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

[Pádraig Brady]

On 18/03/11 13:48, Chris Mason wrote:
> 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:
>>>>>
>>>>> 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.

OK thanks for the info. So:

XFS may miss some extents for sparse files before 2.6.36
BTRFS and EXT4 need a sync before fiemap() before 2.6.38
BTRFS can return overlapping extents before 2.6.38

It seems like we should at least detect overlapping extents in our:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/extent-scan.c;hb=HEAD
and return false (don't use fiemap) in this case.
We've already stopped doing the lseeks() but that assumes non overlapping 
extents.

So if we do the above we avoid corruption on BTRFS.
We could also change to only enabling FIEMAP_FLAG_SYNC before 2.6.38?
That still doesn't account for the XFS issue which was only fixed
in a released kernel 5 months ago, and that leans towards
us changing cp to just not use fiemap before 2.6.38 as you suggest.
That allows us to remove the FIEMAP_FLAG_SYNC too.

cheers,
Pádraig.