qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] block: Fix copy-on-read crash with partial fina


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH] block: Fix copy-on-read crash with partial final cluster
Date: Fri, 6 Jul 2018 16:20:17 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

On 07/06/2018 11:45 AM, Kevin Wolf wrote:
If the virtual disk size isn't aligned to full clusters,
bdrv_co_do_copy_on_readv() may get pnum == 0 before having the full
cluster completed, which will let it run into an assertion failure:

qemu-io: block/io.c:1203: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < 
pnum' failed.

Check for EOF, assert that we read at least as much as the read request
originally wanted to have (which is true at EOF because otherwise
bdrv_check_byte_request() would already have returned an error) and
return success early even though we couldn't copy the full cluster.

Signed-off-by: Kevin Wolf <address@hidden>
---
  block/io.c                 | 6 ++++++
  tests/qemu-iotests/197     | 9 +++++++++
  tests/qemu-iotests/197.out | 8 ++++++++
  3 files changed, 23 insertions(+)

diff --git a/block/io.c b/block/io.c
index 038449f81f..4c0831149c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1200,6 +1200,12 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
              pnum = MIN(cluster_bytes, max_transfer);
          }
+ /* Stop at EOF if the image ends in the middle of the cluster */
+        if (ret == 0 && pnum == 0) {

Is it any smarter to check for 'ret & BDRV_BLOCK_EOF' instead of pnum == 0?

But as far as I can tell, right now those two conditions are synonymous.

+echo
+echo '=== Partial final cluster ==='
+echo
+
+_make_test_img 1024
+$QEMU_IO -f $IMGFMT -C -c 'read 0 1024' "$TEST_IMG" | _filter_qemu_io

Here, $TEST_IMG has no backing file, and does not have the final cluster allocated; all we have to do is properly read all zeroes. Is it worth also explicitly testing reading of allocated data under COR, and/or the case of one image with another backing image (with same or differing partial cluster sizes), where COR actually has to write the partial cluster? However, the logic in the code appears to cover all of those, whether or not the testsuite does as well.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]