|
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
[Prev in Thread] | Current Thread | [Next in Thread] |