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: > >>> 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.
Great, that makes much more sense ;) > > > > >>> > >>>>>> 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 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 <[email protected]> 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 <[email protected]> Signed-off-by: Tao Ma <[email protected]> Signed-off-by: Alex Elder <[email protected]> -chris
