Re: [Qemu-devel] semantics of FIEMAP without FIEMAP_FLAG_SYNC (was Re: [PATCH v5 13/14] nbd: Implement NBD_CMD_WRITE_ZEROES on server)

[Dave Chinner]

On Thu, Jul 21, 2016 at 01:31:21PM +0100, Pádraig Brady wrote:
> On 21/07/16 12:43, Dave Chinner wrote:
> > On Wed, Jul 20, 2016 at 03:35:17PM +0200, Niels de Vos wrote:
> >> Oh... And I was surprised to learn that "cp" does use FIEMAP and not
> >> SEEK_HOLE/SEEK_DATA. You give a strong suggestion to fix that.
> >>   http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/extent-scan.c#n87
> > 
> > My understanding was that FIEMAP was disabled almost immediately
> > after the data corruption problems were understood, and it hasn't
> > been turned back on since. I don't see FIEMAP calls in strace when I
> > copy sparse files, even when I use --sparse=auto which is supposed
> > to trigger the sparse source file checks using FIEMAP.
> > 
> > So, yeah, while there's FIEMAP code present, that doesn't mean it's
> > actually used. And, yes, there are comments in coreutils commits in
> > 2011 saying that the FIEMAP workarounds are temporary until
> > SEK_HOLE/DATA are supported, which were added to the kernel in 2011
> > soon after the 'cp-corrupts-data-with-fiemap' debacle came to light.
> > Five years later, and the fiemap code is stil there?
> 
> Yes it's still there, and hasn't caused issues.
> It's used only for sparse files:
> 
>   $ truncate -s1G t.fiemap
>   $ strace -e ioctl cp t.fiemap t2.fiemap
>   ioctl(3, FS_IOC_FIEMAP, 0x7ffff007c910) = 0

So, my tests were done with coreutils 8.25, and a sparse 100M file with
a layout like this:

$ xfs_bmap -vvp testfile
testfile:
 EXT: FILE-OFFSET       BLOCK-RANGE        AG AG-OFFSET            TOTAL FLAGS
   0: [0..20479]:       71768912..71789391  3 (11216720..11237199) 20480 00000
   1: [20480..40959]:   hole                                       20480
   2: [40960..61439]:   71809872..71830351  3 (11257680..11278159) 20480 00000
   3: [61440..81919]:   71830352..71850831  3 (11278160..11298639) 20480 10000
   4: [81920..96223]:   71850832..71865135  3 (11298640..11312943) 14304 00000
   5: [96224..190471]:  60457944..60552191  2 (20089816..20184063) 94248 00000
   6: [190472..204799]: 73101864..73116191  3 (12549672..12563999) 14328 00000
 FLAG Values:
    010000 Unwritten preallocated extent
    001000 Doesn't begin on stripe unit
    000100 Doesn't end   on stripe unit
    000010 Doesn't begin on stripe width
    000001 Doesn't end   on stripe width

i.e. a 10MB hole @ 10MB, a 10MB unwritten extent @ 30MB. And, yes, I
do see a fiemap call with your example above.

IOWs, it seems the "is sparse" heuristic that cp uses is not very
reliable.  That doesn't inspire confidence, and explains why none of
my cp tests on sparse files ove rthe past 5 years have ever shown
FIEMAP calls....

/me is now somewhat afraid to use cp for copies that require data
integrity.

> It's a pity fiemap is racy (on some file systems?) wrt reporting
> of data not yet written out, and is thus fairly useless IMHO
> without the FIEMAP_FLAG_SYNC workaround.

It's racy on *all filesystems* - all the FIEMAP_FLAG_SYNC flag does
is make the race window smaller. The extent map is only coherent
with the data in memory while the inode locks are held by the
kernel. The moment the inode locks are dropped by the kernel in the
syscall, the extent map held in the fiemap structure is considered
stale and no longer coherent with the extent map held on the inode.

Cheers,

Dave.
-- 
Dave Chinner
address@hidden