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

[Christoph Hellwig <hch@lst.de>]

[adding Carlos who was looking into this if I remember correctly]

On Fri, Aug 31, 2018 at 08:36:40AM -0400, Brian Foster wrote:
> So basically ioctl_fibmap() either prioritizes ->fiemap() or looks for
> some special combination of (fiemap && !bmap) to translate the call..

I'd always use fiemap if present, but if there is any good reason
to prefer ->bmap if present that should be ok.  Hopefully we can just
kill of pure ->bmap implementations in the long run..

> > > Granted, grub's blocklist code doesn't seem to check for shared blocks
> > > when it writes grubenv.... yuck, though TBH I don't have the eye budget
> > > to spend on digging through grub2.  Frankly I think FIBMAP comes verrry
> > > close to "this API is unfixably stupid and shouldn't be enabled for new
> > > use cases and should go away some day".
> > 
> > .. and that policy should be: always return an error for the slightest
> > unusual file layout (shared, encrypted, inline, etc).
> 
> ... and then return some error if the associate extent is in some state
> that cannot be described by fibmap..? That sounds like a nice option to
> me. Carlos..?

Yes.

> Maybe it's too late for this, but I think even dropping ->bmap
> completely for the time being on XFS reflink=1 filesystems is preferable
> to the current behavior where we return a perfectly valid result and
> pretend that somehow represents an error to userspace.

Note that a 0 return on ->bmap has traditionally been treated as an
error, including in userspace as block 0 is usually not a valid block
for user data.   (this isn't intended as support for this interface,
just stating history)