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 <dchinner@redhat.com>]

On Wed, Jul 20, 2016 at 05:19:37AM -0400, Paolo Bonzini wrote:
> Adding ext4 and XFS guys (Lukas and Dave respectively).  As a quick recap, the
> issue here is the semantics of FIEMAP and SEEK_HOLE/SEEK_DATA, which we use in
> "qemu-img map".  This command prints metadata about a virtual disk image---which
> in the case of a raw image amounts to detecting holes and unwritten extents.
> 
> First, it seems like SEEK_HOLE and SEEK_DATA really should be "SEEK_NONZERO" and
> "SEEK_ZERO", on both ext4 and XFS.    You can see that unwritten extents are
> reported by "qemu-img map" as holes:

Correctly so. seek hole/data knows nothing about the underlying
filesystem storage implementation. A "hole" is defined as a region
of zeros, and the filesystem is free call anything it kows for
certain contains zeros as a hole.

FYI, SEEK_HOLE/DATA were named after the the pre-existing Solaris
seek API that uses these definitions for hole and data....

>     $ dd if=/dev/urandom of=test.img bs=1M count=100
>     $ fallocate -z -o 10M -l 10M test.img
>     $ du -h test.img
>     $ qemu-img map --output=json test.img
>     [{ "start": 0, "length": 10485760, "depth": 0, "zero": false, "data": true, "offset": 0},
>     { "start": 10485760, "length": 10485760, "depth": 0, "zero": true, "data": false, "offset": 10485760},
>     { "start": 20971520, "length": 83886080, "depth": 0, "zero": false, "data": true, "offset": 20971520}]

> 
> On the second line, zero=true data=false identifies a hole.  The right output
> would either have zero=true data=true (unwritten extent) or just
> 
>     [{ "start": 0, "length": 104857600, "depth": 0, "zero": false, "data": true, "offset": 0},
>
> since the zero flag is advisory (it doesn't try to detect zeroes beyond what the
> filesystem says).

Not from SEEK_HOLE/SEEK_DATA. All it conveys is "this is the start
of a range of zeros" and "this is the start of a range of data". And
for filesystems that don't specifically implement these seek
operations, zeros are considered data. i.e. SEEK_HOLE will take you
to the end of file, SEEK_DATA returns the current position....

i.e. unwritten extents contain no data, so they are semantically
identical to holes for the purposes of seeking and hence SEEK_DATA
can skip over them.

> The reason why we disabled FIEMAP was a combination of a corruption and performance
> issue.  The data corruption bug was at https://bugs.launchpad.net/qemu/+bug/1368815
> and it was reported on Ubuntu Trusty (kernel 3.13 based on the release notes at
> https://wiki.ubuntu.com/TrustyTahr/ReleaseNotes).  We corrected that by using
> FIEMAP_FLAG_SYNC, based on a similar patch to coreutils.  This turned out to be too
> slow, so we dropped FIEMAP altogether.

Yes, because FIEMAP output is only useful for diagnostic purposes as
it can be stale even before the syscall returns to userspace. i.e.
it can't be used in userspace for optimising file copies....

Finding regions of valid data in a file is what SEEK_HOLE/SEEK_DATA
is for, not FIEMAP. FIEMAP only reports the physical layout of the
file, now where the currently valid data in the file lies.

> However, today I found kernel commit 91dd8c114499 ("ext4: prevent race while walking
> extent tree for fiemap", 2012-11-28) whose commit message says:
> 
>     Moreover the extent currently in delayed allocation might be allocated
>     after we search the extent tree and before we search extent status tree
>     delayed buffers resulting in those delayed buffers being completely
>     missed, even though completely written and allocated.
> 
> This seems pretty much like our data corruption bug; it would mean that
> using FIEMAP_FLAG_SYNC was only working around a bug and delayed allocations
> _should_ be reported as usual by FIEMAP.

What do you think you can do with the delayed allocation regions in
the file?

Indeed, with specualtive delayed allocation, XFS can report delalloc
regions that have no actual resemblence to the layout of dirty data
in the file. SEEK_DATA will iterate the delalloc regions containing
data precisely, however.

> Except that the commit went in kernel 3.8 and as said above Trusty had 3.13.
> So either there are other bugs, or my understanding of the commit is not correct.
> So the questions for Lukas and Dave are:
> 
> 1) is it expected that SEEK_HOLE skips unwritten extents?

There are multiple answers to this, all of which are correct depending
on current context and state:

1. No - some filesystems will report clean unwritten extents as holes.

2. Yes - some filesystems will report clean unwritten extents as
data.

3.  Maybe - if there is written data in memory over the unwritten
extent on disk (i.e. hasn't been flushed to disk, it will be
considered a data region with non-zero data. (FIEMAP will still
report is as unwritten)

> If not, would
> it be acceptable to introduce Linux-specific SEEK_ZERO/SEEK_NONZERO, which
> would be similar to what SEEK_HOLE/SEEK_DATA do now?

To solve what problem? You haven't explained what problem you are
trying to solve yet.

> 2) for FIEMAP do we really need FIEMAP_FLAG_SYNC?  And if not, for what
> filesystems and kernel releases is it really not needed?

I can't answer this question, either, because I don't know what
you want the fiemap information for.

Cheers,

Dave.
-- 
Dave Chinner
dchinner@redhat.com