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.