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

[Pádraig Brady]

On 18/03/11 16:32, Eric Sandeen wrote:
> 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
> 

OK I'll assume this is not a big issue for cp as it checks the final flag.

But there is the other issue with BTRFS that I forgot to mention,
where it returns "unwritten" extents for holes, that was also
fixed recently.

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

cp --version >= 8.10 is probably the best way at present,
or perhaps `timeout 2 cp holey dst`

cheers,
Pádraig.