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

[Pádraig Brady]

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:
>>> On 17/03/11 23:00, Pádraig Brady wrote:
>>>> On 17/03/11 19:29, Mike Frysinger wrote:
>>>>> On Thursday, March 17, 2011 05:32:33 Pádraig Brady wrote:
>>>>>> This is the kind of patch appropriate for a distro,
>>>>>> as they may or may not have a fix backported to their kernel.
>>>>>
>>>>> Gentoo, being a source distribution, has no kernel requirement.  people 
>>>>> are
>>>>> free to run on pretty much anything.  we dont have the luxury of saying
>>>>> "upgrade your kernel package and reboot".  especially since we also 
>>>>> support
>>>>> running Gentoo inside of other distros (often times as non-root user) 
>>>>> where
>>>>> people dont have the ability at all to upgrade.
>>>>>
>>>>>> I presume you're referring to coreutils bug 8001.
>>>>>> I didn't realise ext4 was also affected.
>>>>>> Is this specific to the compress flag?
>>>>>> What was the specific fix to btrfs &/or ext4 in 2.6.38?
>>>>>
>>>>> this is the ext4-specific issue i'm avoiding:
>>>>> http://www.spinics.net/lists/linux-ext4/msg23430.html
>>>>
>>>> Ah that issue. That's what the syncing required in this test
>>>> was working around:
>>>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=tests/cp/sparse-fiemap;h=a2460a08;hb=HEAD
>>>> Note Fedora 15 about to be released is using the new fiemap code in cp,
>>>> but is also based on 2.6.38 and so will have the fix soon,
>>>> if it does not already have it.
>>>
>>> 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.

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

cheers,
Pádraig.