Re: [PATCH 0/4] fix & merge block_status_above and is_allocated_above

[Kevin Wolf]

Am 16.11.2019 um 17:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> I wanted to understand, what is the real difference between 
> bdrv_block_status_above
> and bdrv_is_allocated_above, IMHO bdrv_is_allocated_above should work through
> bdrv_block_status_above..
> 
> And I found the problem: bdrv_is_allocated_above considers space after EOF as
> UNALLOCATED for intermediate nodes..
> 
> UNALLOCATED is not about allocation at fs level, but about should we go to 
> backing or
> not.. And it seems incorrect for me, as in case of short backing file, we'll 
> read
> zeroes after EOF, instead of going further by backing chain.
> 
> This leads to the following effect:
> 
> ./qemu-img create -f qcow2 base.qcow2 2M
> ./qemu-io -c "write -P 0x1 0 2M" base.qcow2
> 
> ./qemu-img create -f qcow2 -b base.qcow2 mid.qcow2 1M
> ./qemu-img create -f qcow2 -b mid.qcow2 top.qcow2 2M
> 
> Region 1M..2M is shadowed by short middle image, so guest sees zeroes:
> ./qemu-io -c "read -P 0 1M 1M" top.qcow2
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 00.00 sec (22.795 GiB/sec and 23341.5807 ops/sec)
> 
> But after commit guest visible state is changed, which seems wrong for me:
> ./qemu-img commit top.qcow2 -b mid.qcow2
> 
> ./qemu-io -c "read -P 0 1M 1M" mid.qcow2
> Pattern verification failed at offset 1048576, 1048576 bytes
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 00.00 sec (4.981 GiB/sec and 5100.4794 ops/sec)
> 
> ./qemu-io -c "read -P 1 1M 1M" mid.qcow2
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 00.00 sec (3.365 GiB/sec and 3446.1606 ops/sec)
> 
> 
> I don't know, is it a real bug, as I don't know, do we support backing
> file larger than its parent. Still, I'm not sure that this behavior of
> bdrv_is_allocated_above don't lead to other problems.

Actually, this specific problem is completely unrelated to how the block
status functions deal with short backing files because they are only
ever called for backing files of the same length as their overlay.

The problem is that the commit job grows the backing file first without
making sure that the clusters in the new part read as zeros. After this,
the damage is done and bdrv_is_allocated_above() returns correctly that
the blocks are unallocated both in top.qcow2 and in mid.qcow2.

So the simple fix for 4.2 would be the following. Maybe we can find a
way to optimise it later (though probably it's not worth it because
short backing files are an uncommon case anyway).

Kevin


diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..a0c4f51caf 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -159,6 +159,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
         if (ret) {
             goto out;
         }
+        ret = blk_co_pwrite_zeroes(s->base, base_len, len - base_len,
+                                   BDRV_REQ_MAY_UNMAP);
+        if (ret < 0) {
+            goto out;
+        }
     }
 
     buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
diff --git a/block/mirror.c b/block/mirror.c
index f0f2d9dff1..2a34f2fad6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -883,6 +883,12 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
             if (ret < 0) {
                 goto immediate_exit;
             }
+            ret = blk_co_pwrite_zeroes(s->target, base_length,
+                                       s->bdev_length - base_length,
+                                       BDRV_REQ_MAY_UNMAP);
+            if (ret < 0) {
+                goto immediate_exit;
+            }
         }
     }