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

[Carlos Maiolino <cmaiolino@redhat.com>]

On Sun, Sep 02, 2018 at 12:52:47PM -0500, Eric Sandeen wrote:
> On 9/2/18 9:08 AM, Carlos Maiolino wrote:
> > Hi Folks,
> > 
> > On Fri, Aug 31, 2018 at 08:36:40AM -0400, Brian Foster wrote:
> >> On Fri, Aug 31, 2018 at 08:28:13AM +0200, Christoph Hellwig wrote:
> >>> On Thu, Aug 30, 2018 at 11:28:49AM -0700, Darrick J. Wong wrote:
> >>>> I prefer to have FIBMAP return errors to *cough* encourage people to use
> >>>> FIEMAP.  If code are going to abuse the FI[BE]MAP interface they could
> >>>> at least abuse the one that gives it enough context to avoid fs
> >>>> corruption.  (A proper fs driver would be preferable, though very
> >>>> difficult).
> >>>
> >>> I think Carlos was looking into implementing the FIBMAP ioctl
> >>> using ->fiemap.  In that case we could return sensible errors,
> >>> and centralize policy in a single place..
> >>>
> >>
> >> So basically ioctl_fibmap() either prioritizes ->fiemap() or looks for
> >> some special combination of (fiemap && !bmap) to translate the call..
> >>
> >>>> 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, I've been working on using FIEMAP interface to handle FIBMAP, it was mostly
> > working, although it needed some extra tweaks due the fact different
> > filesystems return different blocks inside an extent, when a single block query
> > is made on FIEMAP.
> > 
> > I mean, if you query for a single block which is in the middle of an extent,
> > ext4 returns the address of the specific block inside the extent, while xfs
> > (using iomap fiemap infra), returns the address of the first block in the
> > extent.

Ops, after re-reading my notes about it today, it's quite the opposite. xfs
(with iomap), returns the requested block, while ext4 returns the beginning of
the extent.
> 
> IMHO, with hindsight, FIEMAP really kind of sucks.  The call is a pain to
> set up, and the results are a pain to interpret.
> 
> Preparing a patch for zipl to use FIEMAP, I realized what a truly crappy and
> cumbersome interface it is, particularly if you just want to map a small
> handful of blocks.
> 
> I doubt it'd fly, but I'm half tempted to propose a new block-at-at-time
> FIBMAP2 that doesn't overflow 32 bits, uses flags similar to FIEMAP to control
> behavior and return mapped block state, and can return proper errors.
> 
> FIEMAP is great that it can efficiently map contiguous extents and all, but
> it's so cumbersome to use.  (ISTR that both xfsprogs' and e2fsprogs' use of
> FIEMAP had /multiple/ followon commits fixing the original implementation
> in filefrag and xfs_io.)
> 

I'm still refreshing my memory at the subject, but, IIRC the past discussions,
we will need to support FIBMAP forever, but, internally we can actually use
FIEMAP infra-structure to answer FIBMAP calls. From a user perspective, it
changes nothing.

The idea, is to replace all the ->bmap calls, by calling into bmap directly, and
make bmap() internals to use FIEMAP infra-structure to get the required single
block.

I made it work successfully on systems using only XFS, until I tested on ext4
and the system didn't boot due the difference between iomap and ext4 FIEMAP
implementation I explained above.

Actually, IIRC I put this project by side because we haven't decided which
interface would be better, but looks like the right approach would be to use an
fs-independent solution, which is something I'll try to achieve this week.

Cheers

> -Eric

-- 
Carlos