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

[Eric Sandeen]

On 3/18/11 8:48 AM, Chris Mason wrote:

...

 
> 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?

Actually I don't think sync helps that either :)

But as long as coreutils is looking for the "last extent" flag, I think
it works out ok.  Without that fix, I -think- we just returned
fewer extents than were requested, but without the last/EOF flag
returned, so hopefully the application keeps asking for more.

I recently found another similar behavior that still exists...
and fixed up a tool in xfstests to handle it:

http://git.kernel.org/?p=fs/xfs/xfstests-dev.git;a=commitdiff;h=e423b5c584300c738cee95badc13b01bf38c5dc8

I wonder how bad sync will be, in practice.  Thinking about my
own usecases, I doubt I point "cp" at modified-but-not-written
source (source vs. dest I mean) files very often... 

The testcase in coreutils should definitely remove the fdatasync
hack, as that just makes the test pass without worrying about things
in the real world.  :)  A better test suite might be to generate
random holey / delalloc files and copy those around.

We should write an xfstests test to do the same thing, if we can
determine whether we have a "cp" that knows about fiemap?

-Eric

> 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