Re: [PATCH] copy: adjust fiemap handling of sparse files

[Chris Mason]

Excerpts from Pádraig Brady's message of 2011-03-18 09:19:44 -0400:
> On 18/03/11 12:04, Chris Mason wrote:
> > Excerpts from Jim Meyering's message of 2011-03-18 07:52:43 -0400:
> >> Pádraig Brady wrote:
> >>> I also now remember this discussion:
> >>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6131
> >>> where FIEMAP_FLAG_SYNC was introduced in cp,
> >>> I think to work around this same bug in BTRFS and EXT4.
> >>> This flag in ineffective though on ext4 loopback
> >>> and thus I needed to add the syncs to the test as ref'd above.
> > 
> > Sorry, I'm not sure I follow the loopback problem?
> 
> Bah humbug. Looks like there is no such issue.
> This actually seems like an issue in a coreutils test script,
> which make it seem like the SYNC done by `filefrag -vs` was ineffective.

Great, that makes much more sense ;)

> 
> > 
> >>>
> >>>>>> Also note the make_holes heuristic variable is no
> >>>>>> longer used in extent_copy due to this patch which
> >>>>>> came after the 8.10
> >>>>>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=275c831a
> >>>>>
> >>>>> i'll worry about it once 8.11 is released ;)
> >>>>> -mike
> >>>>
> >>>> You might be safer to just bypass extent_copy for kernels < 2.6.38
> >>>> I'm 30:70 for doing that in upstream coreutils
> >>>
> >>> So am I right in thinking FIEMAP_FLAG_SYNC is no longer needed with 
> >>> 2.6.38.
> >>> I'm quite worried about implicit syncing in cp actually, where it might
> >>> introduce bad performance regressions.
> >>
> >> Good point.
> >>
> >>> Maybe we could disable this flag if XFS || kernel >= 2.6.38?
> >>> Or maybe we might do as you suggest above and disable extent_copy()
> >>> completely, for kernels before 2.6.38.
> >>> Not an ideal situation at all :(
> >>
> >> If there really is risk of data loss with ext4 on kernels < 2.6.38,
> >> even if it's only on unusual files, then we'll have to consider
> >> using a kernel version check to disable extent_copy.
> >>
> >> Is there a stand-alone ext4 reproducer?
> > 
> > dd if=/dev/zero of=/mnt/foo bs=1M seek=1 count=1
> > 
> > Without a sync the buggy ext4/btrfs will not find the extent, and report
> > only holes.
> 
> OK, FIEMAP_FLAG_SYNC is an effective workaround for that.
> So as mentioned above, we might consider not using this flag for
> XFS || kernel >= 2.6.38

Btrfs before 2.6.38 may have real trouble though, even with the sync.
We were returning overlapping ranges to you, so the destination would
end up bigger than the original.  This could be fixed in cp by making
sure to never seek backwards based on the extents returned.

Example bad results:

logical: [ 0 ... 1MB ] -> physical [ foo .. foo ]
logical  [ 0 ... 2MB ] -> physical [ foo2 .. foo2 ]

100% a btrfs bug, but cp would do the 0 .. 1MB bit and then seek back to
zero and do the 0 .. 2MB bit.  If cp had not seeked backwards you would
have covered for me.  2.6.38 final fixed this problem.

I think even XFS was fixed relatively recently, 2.6.36 and later.
Looking at the commit, the simple dd test above wouldn't have triggered
it.  Actually, looking at this commit even the sync wouldn't save xfs
before 2.6.36, Eric am I reading this right?

So maybe just give in and look for 2.6.38 instead of trying to remember
all the places where individual filesystems didn't suck.  Give the user
a cp --sparse=fiemap-im-really-really-sure to override your assumptions
about our bugs.

commit 9af25465081480a75824fd7a16a37a5cfebeede9
Author: Tao Ma <address@hidden>
Date:   Mon Aug 30 02:44:03 2010 +0000

    xfs: Make fiemap work with sparse files
    
    In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
    to return fi_extent_max extents, but actually it won't work for
    a sparse file. The reason is that in xfs_getbmap we will
    calculate holes and set it in 'out', while out is malloced by
    bmv_count(fi_extent_max+1) which didn't consider holes. So in the
    worst case, if 'out' vector looks like
    [hole, extent, hole, extent, hole, ... hole, extent, hole],
    we will only return half of fi_extent_max extents.
    
    This patch add a new parameter BMV_IF_NO_HOLES for bvm_iflags.
    So with this flags, we don't use our 'out' in xfs_getbmap for
    a hole. The solution is a bit ugly by just don't increasing
    index of 'out' vector. I felt that it is not easy to skip it
    at the very beginning since we have the complicated check and
    some function like xfs_getbmapx_fix_eof_hole to adjust 'out'.
    
    Cc: Dave Chinner <address@hidden>
    Signed-off-by: Tao Ma <address@hidden>
    Signed-off-by: Alex Elder <address@hidden>

-chris