[PATCH] qemu-{img,nbd}: Don't report zeroed cluster as a hole

[Nir Soffer]

When zeroing a cluster in an image with backing file, qemu-img and
qemu-nbd reported the area as a hole. This does not affect the guest
since the area is read as zero, but breaks code trying to reconstruct
the image chain based on qemu-img map or qemu-nbd block status response.

Here is simpler reproducer:

    # Create a qcow2 image with a raw backing file:
    $ qemu-img create base.raw $((4*64*1024))
    $ qemu-img create -f qcow2 -b base.raw -F raw top.qcow2

    # Write to first 3 clusters of base:
    $ qemu-io -f raw -c "write -P 65 0 64k" base.raw
    $ qemu-io -f raw -c "write -P 66 64k 64k" base.raw
    $ qemu-io -f raw -c "write -P 67 128k 64k" base.raw

    # Write to second cluster of top, hiding second cluster of base:
    $ qemu-io -f qcow2 -c "write -P 69 64k 64k" top.qcow2

    # Write zeroes to third cluster of top, hiding third cluster of base:
    $ qemu-io -f qcow2 -c "write -z 128k 64k" top.qcow2

This creates:

    top:  -D0-
    base: ABC-

How current qemu-img and qemu-nbd report the state:

    $ qemu-img map --output json top.qcow2
    [{ "start": 0, "length": 65536, "depth": 1, "zero": false, "data": true, 
"offset": 0},
    { "start": 65536, "length": 65536, "depth": 0, "zero": false, "data": true, 
"offset": 327680},
    { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": 
false},
    { "start": 196608, "length": 65536, "depth": 1, "zero": true, "data": 
false, "offset": 196608}]

    $ qemu-nbd -r -t -f qcow2 top.qcow2 &
    $ qemu-img map --output json nbd://localhost
    [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, 
"offset": 0},
    { "start": 131072, "length": 131072, "depth": 0, "zero": true, "data": 
false, "offset": 131072}]

    $ nbdinfo --map nbd://localhost
             0      131072    0  allocated
        131072      131072    3  hole,zero

The third extents is reported as a hole in both cases. In qmeu-nbd the
cluster is merged with forth cluster which is actually a hole.

This is incorrect since if it was a hole, the third cluster would be
exposed to the guest. Programs using qemu-nbd output to reconstruct the
image chain on other storage would be confused and copy only the first 2
cluster. The results of this copy will be an image exposing the third
cluster from the base image, corrupting the guest data.

I found that it can be fixed using BDRV_BLOCK_OFFSET_VALID when
reporting the status of the extent. When we have a valid offset, we
report based on BDRV_BLOCK_DATA. Otherwise we report based on
BDRV_BLOCK_ALLOCATED.

With this fix we get:

    $ build/qemu-img map --output json top.qcow2
    [{ "start": 0, "length": 65536, "depth": 1, "zero": false, "data": true, 
"offset": 0},
    { "start": 65536, "length": 65536, "depth": 0, "zero": false, "data": true, 
"offset": 327680},
    { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": true},
    { "start": 196608, "length": 65536, "depth": 1, "zero": true, "data": 
false, "offset": 196608}]

    $ build/qemu-nbd -r -t -f qcow2 top.qcow2 &
    $ qemu-img map --output json nbd://localhost
    [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": true, 
"offset": 0},
    { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": true, 
"offset": 131072},
    { "start": 196608, "length": 65536, "depth": 0, "zero": true, "data": 
false, "offset": 196608}]

    $ nbdinfo --map nbd://localhost
             0      131072    0  allocated
        131072       65536    2  zero
        196608       65536    3  hole,zero

The issue was found by ovirt-imageio functional tests:
https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/client_test.py

I did not update any of the existing tests, and I'm sure many tests are
missing, and the documentation should change to describe the new
behavior. Posting as is for early review.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Resolves: https://bugzilla.redhat.com/1968693
---
 nbd/server.c | 8 ++++++--
 qemu-img.c   | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index b60ebc3ab6..adf37905d5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2127,8 +2127,12 @@ static int blockstatus_to_extents(BlockDriverState *bs, 
uint64_t offset,
             return ret;
         }
 
-        flags = (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE) |
-                (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
+        flags = (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
+
+        if (ret & BDRV_BLOCK_OFFSET_VALID)
+            flags |= (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE);
+        else
+            flags |= (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE);
 
         if (nbd_extent_array_add(ea, num, flags) < 0) {
             return 0;
diff --git a/qemu-img.c b/qemu-img.c
index a5993682aa..6808e12d87 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3039,7 +3039,9 @@ static int get_block_status(BlockDriverState *bs, int64_t 
offset,
     *e = (MapEntry) {
         .start = offset,
         .length = bytes,
-        .data = !!(ret & BDRV_BLOCK_DATA),
+        .data = !!(has_offset
+            ? ret & BDRV_BLOCK_DATA
+            : ret & BDRV_BLOCK_ALLOCATED),
         .zero = !!(ret & BDRV_BLOCK_ZERO),
         .offset = map,
         .has_offset = has_offset,
-- 
2.26.3