Re: [PATCH, RFC] xfs: re-enable FIBMAP on reflink; disable for swap

[Eric Sandeen <sandeen@sandeen.net>]

On 8/30/18 7:11 PM, Dave Chinner wrote:
> On Thu, Aug 30, 2018 at 01:51:56PM -0500, Eric Sandeen wrote:
>> On 8/30/18 1:28 PM, Darrick J. Wong wrote:
>>> On Thu, Aug 30, 2018 at 02:02:05PM -0400, Brian Foster wrote:
>>>> On Thu, Aug 30, 2018 at 11:35:46AM -0500, Eric Sandeen wrote:
>>>>> On 8/30/18 11:36 AM, Christoph Hellwig wrote:
>>>>>> On Thu, Aug 30, 2018 at 11:31:40AM -0500, Eric Sandeen wrote:
>>>>>>> That's no reason to uniquely disallow it for reflinked files, though;
>>>>>>> the problem is universal.  It's true for fiemap as well.  So I'm not sure
>>>>>>> that's an argument against the patch?
>>>>>>
>>>>>> fiemap at least tells you an extent is shared, bmap does not.
>>>>>
>>>>> yes, so bmap is clearly the wrong interface to use if you want to
>>>>> write directly to a file's blocks.  But if you know enough to check
>>>>> the fiemap shared flag, you know enough to not use fibmap for that purpose...
>>>>>
>>>>
>>>> FWIW, this patch seems reasonable to me. To Christoph's point, I don't
>>>> think either interface really grants license to write to the underlying
>>>> blocks, so either way it's technically being abused for this purpose.
>>>> Unless there's a clear way to return an error for a particular type of
>>>> file, I think it's reasonable behavior for fibmap to expose the data it
>>>> supports (i.e., block maps) and drop the data it doesn't (reflink
>>>> state).
>>>
>>> But shared block status isn't something that can be dropped lightly.  If
>>> you write to a shared block without realizing it, you'll corrupt every
>>> other file that shares the block.
>>
>> But there is no circumstance under which it is safe to write to a mapped
>> block no matter how you mapped it, tbh.
> 
> <sigh>
> 
> That's what all the break_layouts() code in XFS provides. It's a
> mechanism for applications to prevent the block layout from changing
> unexpected until they - the layout lease owner - give up their
> exclusive access to the file layout.

> Seriously, this has been talked about so much in the past year or
> two in the context of DAX, RDMA, get_user_pages() races in direct
> IO, etc. it pains me to see this discussion rehashing it all over
> again.
> 
> We want applications to do what they need to do safely.  FIBMAP is
> unsafe and, worse, it's unfixable. We need to get apps to move away
> from it to something is actualayl safe.


> Adding a file lease interface to block 3rd party changes to the
> file layout until the app releases the lease is a safe way
> of allowing userspace apps to use FIEMAP to map and identify
> file extents they can write directly to if they need to.
> 
> IOWs, we need to get the FL_LAYOUT flag out into the external file
> lease interface (IIRC Dan Williams posted patches for this a while
> back) and get these "FIBMAP + write()" apps to use "FL_LAYOUT,
> fsync(), FIEMAP, write(), ~FL_LAYOUT".
> 
> We need to make FIBMAP go away by providing a safer, more robust
> solution to the problem people are trying to solve.

Sure.  I get it that it's not a great interface.  I get it that there
are better ways.  When those methods are available, we should explicitly
deprecate FIBMAP.

But until then I can't understand why we'd intentionally break an
otherwise reasonably functional interface in subtle and undetectable
ways for certain classes of files.  We /could/ FIBMAP a reflinked file
exactly as well as we can FIBMAP a non-reflinked file, but we choose
not to.  This choice creates unnecessary problems for existing apps.

Until we deprecate the FIBMAP interface, until there is a better way,
I think we should make it as predictable and complete as
we can, not cripple it intentionally.

But I'm clearly in the minority with that opinion, so I guess I'll
withdraw the patch.

-Eric