From 3c2c599c79eea9cebd7280140a5901fe3734e94e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 11 Aug 2023 18:52:29 +0100 Subject: [PATCH 01/15] block/vpc: Avoid dynamic stack allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use autofree heap allocation instead of variable-length array on the stack. Here we don't expect the bitmap size to be enormous, and since we're about to read/write it to disk the overhead of the allocation should be fine. The codebase has very few VLAs, and if we can get rid of them all we can make the compiler error on new additions. This is a defensive measure against security bugs where an on-stack dynamic allocation isn't correctly size-checked (e.g. CVE-2021-3527). Signed-off-by: Philippe Mathieu-Daudé [PMM: expanded commit message] Signed-off-by: Peter Maydell Message-ID: <20230811175229.808139-1-peter.maydell@linaro.org> Reviewed-by: Kevin Wolf Reviewed-by: Francisco Iglesias Signed-off-by: Kevin Wolf --- block/vpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/vpc.c b/block/vpc.c index 3810a601a3..ceb87dd3d8 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -510,7 +510,7 @@ get_image_offset(BlockDriverState *bs, uint64_t offset, bool write, int *err) miss sparse read optimization, but it's not a problem in terms of correctness. */ if (write && (s->last_bitmap_offset != bitmap_offset)) { - uint8_t bitmap[s->bitmap_size]; + g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size); int r; s->last_bitmap_offset = bitmap_offset; @@ -558,7 +558,7 @@ alloc_block(BlockDriverState *bs, int64_t offset) int64_t bat_offset; uint32_t index, bat_value; int ret; - uint8_t bitmap[s->bitmap_size]; + g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size); /* Check if sector_num is valid */ if ((offset < 0) || (offset > bs->total_sectors * BDRV_SECTOR_SIZE)) { From 65c23ef1e4197c53d3836906895062307e48a6c5 Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Thu, 17 Aug 2023 13:25:38 +0200 Subject: [PATCH 02/15] iotests: adapt test output for new qemu_cleanup() behavior Since commit ca2a5e630d ("qemu_cleanup: begin drained section after vm_shutdown()"), there will be an additional pause for jobs during qemu_cleanup(). The reason is that the bdrv_drain_all() call in do_vm_stop() is not inside the drained section used by qemu_cleanup() anymore. I.e., there is a second drained section now that ends before the final one in qemu_cleanup() starts. Thus, job_pause() is called twice during cleanup (via child_job_drained_begin()). Test 185 needs to be adapted directly too, because it waits for a specific number of JOB_STATUS_CHANGE events before the BLOCK_JOB_CANCELLED event. Reported-by: Kevin Wolf Signed-off-by: Fiona Ebner Message-ID: <20230817112538.255111-1-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- tests/qemu-iotests/109.out | 24 ++++++++++++++++++++++++ tests/qemu-iotests/185 | 2 ++ tests/qemu-iotests/185.out | 4 ++++ 3 files changed, 30 insertions(+) diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out index e29280015e..2611d6a40f 100644 --- a/tests/qemu-iotests/109.out +++ b/tests/qemu-iotests/109.out @@ -44,6 +44,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -94,6 +96,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -144,6 +148,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -194,6 +200,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -244,6 +252,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -294,6 +304,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -343,6 +355,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -392,6 +406,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -441,6 +457,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -490,6 +508,8 @@ read 512/512 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -519,6 +539,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} @@ -541,6 +563,8 @@ Images are identical. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "src"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185 index 8b1143dc16..2ae0a85bbf 100755 --- a/tests/qemu-iotests/185 +++ b/tests/qemu-iotests/185 @@ -354,6 +354,8 @@ wait_for_job_and_quit() { QEMU_EVENTS= # Ignore all JOB_STATUS_CHANGE events that came before SHUTDOWN _wait_event $h 'JOB_STATUS_CHANGE' # standby _wait_event $h 'JOB_STATUS_CHANGE' # ready + _wait_event $h 'JOB_STATUS_CHANGE' # standby + _wait_event $h 'JOB_STATUS_CHANGE' # ready _wait_event $h 'JOB_STATUS_CHANGE' # aborting # Filter the offset (depends on when exactly `quit` was issued) _wait_event $h 'BLOCK_JOB_CANCELLED' \ diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out index 70e8dd6c87..7292c26bae 100644 --- a/tests/qemu-iotests/185.out +++ b/tests/qemu-iotests/185.out @@ -137,6 +137,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "mirror", "len": 33554432, "offset": (filtered), "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "mirror"}} @@ -160,6 +162,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "commit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "commit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "commit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "commit", "len": 33554432, "offset": (filtered), "speed": 0, "type": "commit"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "commit"}} From 3480ce69a9c7ee956323c45269d21b6905488904 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 8 Aug 2023 11:58:51 -0400 Subject: [PATCH 03/15] block: minimize bs->reqs_lock section in tracked_request_end() Signed-off-by: Stefan Hajnoczi Message-ID: <20230808155852.2745350-2-stefanha@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/io.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/block/io.c b/block/io.c index 76e7df18d8..4f32d6aa6e 100644 --- a/block/io.c +++ b/block/io.c @@ -593,8 +593,14 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req) qemu_co_mutex_lock(&req->bs->reqs_lock); QLIST_REMOVE(req, list); - qemu_co_queue_restart_all(&req->wait_queue); qemu_co_mutex_unlock(&req->bs->reqs_lock); + + /* + * At this point qemu_co_queue_wait(&req->wait_queue, ...) won't be called + * anymore because the request has been removed from the list, so it's safe + * to restart the queue outside reqs_lock to minimize the critical section. + */ + qemu_co_queue_restart_all(&req->wait_queue); } /** From fa9185fcdfceeb1a02f61a003acd19509e146bde Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 8 Aug 2023 11:58:52 -0400 Subject: [PATCH 04/15] block: change reqs_lock to QemuMutex CoMutex has poor performance when lock contention is high. The tracked requests list is accessed frequently and performance suffers in QEMU multi-queue block layer scenarios. It is not necessary to use CoMutex for the requests lock. The lock is always released across coroutine yield operations. It is held for relatively short periods of time and it is not beneficial to yield when the lock is held by another coroutine. Change the lock type from CoMutex to QemuMutex to improve multi-queue block layer performance. fio randread bs=4k iodepth=64 with 4 IOThreads handling a virtio-blk device with 8 virtqueues improves from 254k to 517k IOPS (+203%). Full benchmark results and configuration details are available here: https://gitlab.com/stefanha/virt-playbooks/-/commit/980c40845d540e3669add1528739503c2e817b57 In the future we may wish to introduce thread-local tracked requests lists to avoid lock contention completely. That would be much more involved though. Signed-off-by: Stefan Hajnoczi Message-ID: <20230808155852.2745350-3-stefanha@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 4 +++- block/io.c | 24 ++++++++++++------------ include/block/block_int-common.h | 2 +- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 0af890f647..b79b1ce7fe 100644 --- a/block.c +++ b/block.c @@ -415,7 +415,7 @@ BlockDriverState *bdrv_new(void) for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { QLIST_INIT(&bs->op_blockers[i]); } - qemu_co_mutex_init(&bs->reqs_lock); + qemu_mutex_init(&bs->reqs_lock); qemu_mutex_init(&bs->dirty_bitmap_mutex); bs->refcnt = 1; bs->aio_context = qemu_get_aio_context(); @@ -5476,6 +5476,8 @@ static void bdrv_delete(BlockDriverState *bs) bdrv_close(bs); + qemu_mutex_destroy(&bs->reqs_lock); + g_free(bs); } diff --git a/block/io.c b/block/io.c index 4f32d6aa6e..525c94b16a 100644 --- a/block/io.c +++ b/block/io.c @@ -591,9 +591,9 @@ static void coroutine_fn tracked_request_end(BdrvTrackedRequest *req) qatomic_dec(&req->bs->serialising_in_flight); } - qemu_co_mutex_lock(&req->bs->reqs_lock); + qemu_mutex_lock(&req->bs->reqs_lock); QLIST_REMOVE(req, list); - qemu_co_mutex_unlock(&req->bs->reqs_lock); + qemu_mutex_unlock(&req->bs->reqs_lock); /* * At this point qemu_co_queue_wait(&req->wait_queue, ...) won't be called @@ -627,9 +627,9 @@ static void coroutine_fn tracked_request_begin(BdrvTrackedRequest *req, qemu_co_queue_init(&req->wait_queue); - qemu_co_mutex_lock(&bs->reqs_lock); + qemu_mutex_lock(&bs->reqs_lock); QLIST_INSERT_HEAD(&bs->tracked_requests, req, list); - qemu_co_mutex_unlock(&bs->reqs_lock); + qemu_mutex_unlock(&bs->reqs_lock); } static bool tracked_request_overlaps(BdrvTrackedRequest *req, @@ -793,9 +793,9 @@ bdrv_wait_serialising_requests(BdrvTrackedRequest *self) return; } - qemu_co_mutex_lock(&bs->reqs_lock); + qemu_mutex_lock(&bs->reqs_lock); bdrv_wait_serialising_requests_locked(self); - qemu_co_mutex_unlock(&bs->reqs_lock); + qemu_mutex_unlock(&bs->reqs_lock); } void coroutine_fn bdrv_make_request_serialising(BdrvTrackedRequest *req, @@ -803,12 +803,12 @@ void coroutine_fn bdrv_make_request_serialising(BdrvTrackedRequest *req, { IO_CODE(); - qemu_co_mutex_lock(&req->bs->reqs_lock); + qemu_mutex_lock(&req->bs->reqs_lock); tracked_request_set_serialising(req, align); bdrv_wait_serialising_requests_locked(req); - qemu_co_mutex_unlock(&req->bs->reqs_lock); + qemu_mutex_unlock(&req->bs->reqs_lock); } int bdrv_check_qiov_request(int64_t offset, int64_t bytes, @@ -3002,7 +3002,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs) goto early_exit; } - qemu_co_mutex_lock(&bs->reqs_lock); + qemu_mutex_lock(&bs->reqs_lock); current_gen = qatomic_read(&bs->write_gen); /* Wait until any previous flushes are completed */ @@ -3012,7 +3012,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs) /* Flushes reach this point in nondecreasing current_gen order. */ bs->active_flush_req = true; - qemu_co_mutex_unlock(&bs->reqs_lock); + qemu_mutex_unlock(&bs->reqs_lock); /* Write back all layers by calling one driver function */ if (bs->drv->bdrv_co_flush) { @@ -3100,11 +3100,11 @@ out: bs->flushed_gen = current_gen; } - qemu_co_mutex_lock(&bs->reqs_lock); + qemu_mutex_lock(&bs->reqs_lock); bs->active_flush_req = false; /* Return value is ignored - it's ok if wait queue is empty */ qemu_co_queue_next(&bs->flush_queue); - qemu_co_mutex_unlock(&bs->reqs_lock); + qemu_mutex_unlock(&bs->reqs_lock); early_exit: bdrv_dec_in_flight(bs); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 74195c3004..7a1e678031 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1231,7 +1231,7 @@ struct BlockDriverState { unsigned int write_gen; /* Current data generation */ /* Protected by reqs_lock. */ - CoMutex reqs_lock; + QemuMutex reqs_lock; QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; CoQueue flush_queue; /* Serializing flush queue */ bool active_flush_req; /* Flush request in flight? */ From b0a6620acfe4c2997935cb3a9baa0d429ab8d61a Mon Sep 17 00:00:00 2001 From: Michael Tokarev Date: Fri, 11 Aug 2023 14:09:46 +0300 Subject: [PATCH 05/15] qemu-img: omit errno value in error message I'm getting io-qcow2-244 test failure on mips* due to output mismatch: Take an internal snapshot: -qemu-img: Could not create snapshot 'test': -95 (Operation not supported) +qemu-img: Could not create snapshot 'test': -122 (Operation not supported) No errors were found on the image. This is because errno values might be different across different architectures. This error message in qemu-img.c is the only one which prints errno directly, all the rest print strerror(errno) only. Fix this error message and the expected output of the 3 test cases too. Signed-off-by: Michael Tokarev Message-ID: <20230811110946.2435067-1-mjt@tls.msk.ru> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- qemu-img.c | 4 ++-- tests/qemu-iotests/080.out | 6 +++--- tests/qemu-iotests/112.out | 6 +++--- tests/qemu-iotests/244.out | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 27f48051b0..0756dbb835 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -3468,8 +3468,8 @@ static int img_snapshot(int argc, char **argv) ret = bdrv_snapshot_create(bs, &sn); if (ret) { - error_report("Could not create snapshot '%s': %d (%s)", - snapshot_name, ret, strerror(-ret)); + error_report("Could not create snapshot '%s': %s", + snapshot_name, strerror(-ret)); } break; diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 45ab01db8e..d8acb3e723 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -33,7 +33,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Snapshot table offset invalid == Hitting snapshot table size limit == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -qemu-img: Could not create snapshot 'test': -27 (File too large) +qemu-img: Could not create snapshot 'test': File too large read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -56,8 +56,8 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Backing file name too long Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -qemu-img: Could not create snapshot 'test': -27 (File too large) -qemu-img: Could not create snapshot 'test': -11 (Resource temporarily unavailable) +qemu-img: Could not create snapshot 'test': File too large +qemu-img: Could not create snapshot 'test': Resource temporarily unavailable == Invalid snapshot L1 table offset == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out index dd3cc4383c..ebf426febc 100644 --- a/tests/qemu-iotests/112.out +++ b/tests/qemu-iotests/112.out @@ -32,7 +32,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount bits: 1 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -qemu-img: Could not create snapshot 'foo': -22 (Invalid argument) +qemu-img: Could not create snapshot 'foo': Invalid argument Leaked cluster 6 refcount=1 reference=0 1 leaked clusters were found on the image. @@ -44,7 +44,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount bits: 2 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -qemu-img: Could not create snapshot 'baz': -22 (Invalid argument) +qemu-img: Could not create snapshot 'baz': Invalid argument Leaked cluster 7 refcount=1 reference=0 1 leaked clusters were found on the image. @@ -75,7 +75,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount bits: 64 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -qemu-img: Could not create snapshot 'foo': -22 (Invalid argument) +qemu-img: Could not create snapshot 'foo': Invalid argument Leaked cluster 5 refcount=18446744073709551615 reference=1 Leaked cluster 6 refcount=1 reference=0 diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out index 5e03add054..4815a489b0 100644 --- a/tests/qemu-iotests/244.out +++ b/tests/qemu-iotests/244.out @@ -41,7 +41,7 @@ write failed: Operation not supported No errors were found on the image. Take an internal snapshot: -qemu-img: Could not create snapshot 'test': -95 (Operation not supported) +qemu-img: Could not create snapshot 'test': Operation not supported No errors were found on the image. === Standalone image with external data file (efficient) === From 7966a36c83132c5eebf04a60d9e475b0aa6e7f88 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Thu, 27 Jul 2023 16:07:05 +0100 Subject: [PATCH 06/15] block/iscsi: Document why we use raw malloc() In block/iscsi.c we use a raw malloc() call, which is unusual given the project standard is to use the glib memory allocation functions. Document why we do so, to avoid it being converted to g_malloc() by mistake. Signed-off-by: Peter Maydell Message-ID: <20230727150705.2664464-1-peter.maydell@linaro.org> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/iscsi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/iscsi.c b/block/iscsi.c index 34f97ab646..5640c8b565 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1058,6 +1058,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, return NULL; } + /* Must use malloc(): this is freed via scsi_free_scsi_task() */ acb->task = malloc(sizeof(struct scsi_task)); if (acb->task == NULL) { error_report("iSCSI: Failed to allocate task for scsi command. %s", From 816248675a21d5b4d82f22a6a19ddd2a7fbfee18 Mon Sep 17 00:00:00 2001 From: Hanna Czenczek Date: Thu, 20 Jul 2023 16:00:24 +0200 Subject: [PATCH 07/15] block: Be more verbose in create fallback For image creation code, we have central fallback code for protocols that do not support creating new images (like NBD or iscsi). So for them, you can only specify existing paths/exports that are overwritten to make clean new images. In such a case, if the given path cannot be opened (assuming a pre-existing image there), we print an error message that tries to describe what is going on: That with this protocol, you cannot create new images, but only overwrite existing ones; and the given path could not be opened as a pre-existing image. However, the current message is confusing, because it does not say that the protocol in question does not support creating new images, but instead that "image creation" is unsupported. This can be interpreted to mean that `qemu-img create` will not work in principle, which is not true. Be more verbose for clarity. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2217204 Signed-off-by: Hanna Czenczek Message-ID: <20230720140024.46836-1-hreitz@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index b79b1ce7fe..1a5cb554a8 100644 --- a/block.c +++ b/block.c @@ -661,8 +661,10 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv, blk = blk_co_new_open(filename, NULL, options, BDRV_O_RDWR | BDRV_O_RESIZE, errp); if (!blk) { - error_prepend(errp, "Protocol driver '%s' does not support image " - "creation, and opening the image failed: ", + error_prepend(errp, "Protocol driver '%s' does not support creating " + "new images, so an existing image must be selected as " + "the target; however, opening the given target as an " + "existing image failed: ", drv->format_name); return -EINVAL; } From a675ca4c620e66c1d45d55037f8fc21286af58cd Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 1 Sep 2023 12:24:30 +0200 Subject: [PATCH 08/15] qemu-img: Update documentation for compressed images Document the 'compression_type' option for qcow2, and mention that streamOptimized vmdk supports compression, too. Reported-by: Richard W.M. Jones Signed-off-by: Kevin Wolf Message-ID: <20230901102430.23856-1-kwolf@redhat.com> Reviewed-by: Richard W.M. Jones Signed-off-by: Kevin Wolf --- docs/tools/qemu-img.rst | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index 15aeddc6d8..ca5a2773cf 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -106,7 +106,11 @@ by the used format or see the format descriptions below for details. .. option:: -c - Indicates that target image must be compressed (qcow format only). + Indicates that target image must be compressed (qcow/qcow2 and vmdk with + streamOptimized subformat only). + + For qcow2, the compression algorithm can be specified with the ``-o + compression_type=...`` option (see below). .. option:: -h @@ -776,7 +780,7 @@ Supported image file formats: QEMU image format, the most versatile format. Use it to have smaller images (useful if your filesystem does not supports holes, for example - on Windows), optional AES encryption, zlib based compression and + on Windows), optional AES encryption, zlib or zstd based compression and support of multiple VM snapshots. Supported options: @@ -794,6 +798,17 @@ Supported image file formats: ``backing_fmt`` Image format of the base image + ``compression_type`` + This option configures which compression algorithm will be used for + compressed clusters on the image. Note that setting this option doesn't yet + cause the image to actually receive compressed writes. It is most commonly + used with the ``-c`` option of ``qemu-img convert``, but can also be used + with the ``compress`` filter driver or backup block jobs with compression + enabled. + + Valid values are ``zlib`` and ``zstd``. For images that use + ``compat=0.10``, only ``zlib`` compression is available. + ``encryption`` If this option is set to ``on``, the image is encrypted with 128-bit AES-CBC. From a8d99c0e6c52191c8a3e01ed79b1a9f186a3a91e Mon Sep 17 00:00:00 2001 From: Dmitry Frolov Date: Thu, 31 Aug 2023 15:59:27 +0300 Subject: [PATCH 09/15] vmdk: Clean up bdrv_open_child() return value check bdrv_open_child() may return NULL. Usually return value is checked for this function. Check for return value is more reliable. Fixes: 24bc15d1f6 ("vmdk: Use BdrvChild instead of BDS for references to extents") Signed-off-by: Dmitry Frolov Message-ID: <20230831125926.796205-1-frolov@swemel.ru> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/vmdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/vmdk.c b/block/vmdk.c index 70066c2b01..58ce290e9c 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1207,7 +1207,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, bs, &child_of_bds, extent_role, false, &local_err); g_free(extent_path); - if (local_err) { + if (!extent_file) { error_propagate(errp, local_err); ret = -EINVAL; goto out; From be2e51c5034ffb2b8d9a4618e8f9b7ff82cb057c Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Fri, 1 Sep 2023 15:46:04 -0300 Subject: [PATCH 10/15] block: Remove bdrv_query_block_node_info The last call site of this function has been removed by commit c04d0ab026 ("qemu-img: Let info print block graph"). Reviewed-by: Claudio Fontana Signed-off-by: Fabiano Rosas Message-ID: <20230901184605.32260-2-farosas@suse.de> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qapi.c | 27 --------------------------- include/block/qapi.h | 3 --- 2 files changed, 30 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index f34f95e0ef..79bf80c503 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -309,33 +309,6 @@ out: aio_context_release(bdrv_get_aio_context(bs)); } -/** - * bdrv_query_block_node_info: - * @bs: block node to examine - * @p_info: location to store node information - * @errp: location to store error information - * - * Store image information about @bs in @p_info. - * - * @p_info will be set only on success. On error, store error in @errp. - */ -void bdrv_query_block_node_info(BlockDriverState *bs, - BlockNodeInfo **p_info, - Error **errp) -{ - BlockNodeInfo *info; - ERRP_GUARD(); - - info = g_new0(BlockNodeInfo, 1); - bdrv_do_query_node_info(bs, info, errp); - if (*errp) { - qapi_free_BlockNodeInfo(info); - return; - } - - *p_info = info; -} - /** * bdrv_query_image_info: * @bs: block node to examine diff --git a/include/block/qapi.h b/include/block/qapi.h index 18d48ddb70..8663971c58 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -36,9 +36,6 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, int bdrv_query_snapshot_info_list(BlockDriverState *bs, SnapshotInfoList **p_list, Error **errp); -void bdrv_query_block_node_info(BlockDriverState *bs, - BlockNodeInfo **p_info, - Error **errp); void bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat, From c3b29ae6b4e2bf70739e49154f24c8e850b34bf7 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Fri, 1 Sep 2023 15:46:05 -0300 Subject: [PATCH 11/15] block: Remove unnecessary variable in bdrv_block_device_info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commit 5d8813593f ("block/qapi: Let bdrv_query_image_info() recurse") removed the loop where we set the 'bs0' variable, so now it is just the same as 'bs'. Signed-off-by: Fabiano Rosas Reviewed-by: Philippe Mathieu-Daudé Message-ID: <20230901184605.32260-3-farosas@suse.de> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- block/qapi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/block/qapi.c b/block/qapi.c index 79bf80c503..1cbb0935ff 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -48,7 +48,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, { ImageInfo **p_image_info; ImageInfo *backing_info; - BlockDriverState *bs0, *backing; + BlockDriverState *backing; BlockDeviceInfo *info; ERRP_GUARD(); @@ -145,7 +145,6 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, info->write_threshold = bdrv_write_threshold_get(bs); - bs0 = bs; p_image_info = &info->image; info->backing_file_depth = 0; @@ -153,7 +152,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, * Skip automatically inserted nodes that the user isn't aware of for * query-block (blk != NULL), but not for query-named-block-nodes */ - bdrv_query_image_info(bs0, p_image_info, flat, blk != NULL, errp); + bdrv_query_image_info(bs, p_image_info, flat, blk != NULL, errp); if (*errp) { qapi_free_BlockDeviceInfo(info); return NULL; From 9e03a5e195a20e7a342bcbf9e49e113e4b4dc23d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 5 Sep 2023 15:06:06 +0200 Subject: [PATCH 12/15] block/meson.build: Restore alphabetical order of files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When commit 5e5733e5999 created block/meson.build, the list of unconditionally added files was in alphabetical order. Later commits added new files in random places. Reorder the list to be alphabetical again. (As for ordering foo.c against foo-*.c, there are both ways used currently; standardise on having foo.c first, even though this is different from the original commit 5e5733e5999.) Signed-off-by: Kevin Wolf Message-ID: <20230905130607.35134-2-kwolf@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Kevin Wolf --- block/meson.build | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/meson.build b/block/meson.build index 529fc172c6..f351b9d0d3 100644 --- a/block/meson.build +++ b/block/meson.build @@ -4,41 +4,41 @@ block_ss.add(files( 'aio_task.c', 'amend.c', 'backup.c', - 'copy-before-write.c', 'blkdebug.c', 'blklogwrites.c', 'blkverify.c', 'block-backend.c', 'block-copy.c', - 'graph-lock.c', 'commit.c', + 'copy-before-write.c', 'copy-on-read.c', - 'preallocate.c', - 'progress_meter.c', 'create.c', 'crypto.c', 'dirty-bitmap.c', 'filter-compress.c', + 'graph-lock.c', 'io.c', 'mirror.c', 'nbd.c', 'null.c', 'plug.c', + 'preallocate.c', + 'progress_meter.c', 'qapi.c', + 'qcow2.c', 'qcow2-bitmap.c', 'qcow2-cache.c', 'qcow2-cluster.c', 'qcow2-refcount.c', 'qcow2-snapshot.c', 'qcow2-threads.c', - 'qcow2.c', 'quorum.c', 'raw-format.c', 'reqlist.c', 'snapshot.c', 'snapshot-access.c', - 'throttle-groups.c', 'throttle.c', + 'throttle-groups.c', 'write-threshold.c', ), zstd, zlib, gnutls) From 9ea473fb7bd01d350a860044b3d8e46f8747e865 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 5 Sep 2023 15:06:07 +0200 Subject: [PATCH 13/15] block: Make more BlockDriver definitions static MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most block driver implementations don't have any reason for their BlockDriver to be public. The only exceptions are bdrv_file, bdrv_raw and bdrv_qcow2, which are actually used in other source files. Make all other BlockDriver definitions static if they aren't yet. Signed-off-by: Kevin Wolf Message-ID: <20230905130607.35134-3-kwolf@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Kevin Wolf --- block/copy-before-write.c | 2 +- block/preallocate.c | 2 +- block/snapshot-access.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/copy-before-write.c b/block/copy-before-write.c index b866e42271..9a0e2b69d9 100644 --- a/block/copy-before-write.c +++ b/block/copy-before-write.c @@ -503,7 +503,7 @@ static void cbw_close(BlockDriverState *bs) s->bcs = NULL; } -BlockDriver bdrv_cbw_filter = { +static BlockDriver bdrv_cbw_filter = { .format_name = "copy-before-write", .instance_size = sizeof(BDRVCopyBeforeWriteState), diff --git a/block/preallocate.c b/block/preallocate.c index 4d82125036..3d0f621003 100644 --- a/block/preallocate.c +++ b/block/preallocate.c @@ -535,7 +535,7 @@ static void preallocate_child_perm(BlockDriverState *bs, BdrvChild *c, } } -BlockDriver bdrv_preallocate_filter = { +static BlockDriver bdrv_preallocate_filter = { .format_name = "preallocate", .instance_size = sizeof(BDRVPreallocateState), diff --git a/block/snapshot-access.c b/block/snapshot-access.c index 67ea339da9..8d4e8932b8 100644 --- a/block/snapshot-access.c +++ b/block/snapshot-access.c @@ -108,7 +108,7 @@ static void snapshot_access_child_perm(BlockDriverState *bs, BdrvChild *c, *nshared = BLK_PERM_ALL; } -BlockDriver bdrv_snapshot_access_drv = { +static BlockDriver bdrv_snapshot_access_drv = { .format_name = "snapshot-access", .bdrv_open = snapshot_access_open, From 8686a689e5bc205bdba0e647c269b86756cbc504 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 5 Sep 2023 16:50:01 +0200 Subject: [PATCH 14/15] vmstate: Mark VMStateInfo.get/put() coroutine_mixed_fn Migration code can run both in coroutine context (the usual case) and non-coroutine context (at least savevm/loadvm for snapshots). This also affects the VMState callbacks, and devices must consider this. Change the callback definition in VMStateInfo to be explicit about it. Signed-off-by: Kevin Wolf Message-ID: <20230905145002.46391-2-kwolf@redhat.com> Acked-by: Peter Xu Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/migration/vmstate.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index d1b8abe08d..e4db910339 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -41,9 +41,11 @@ typedef struct VMStateField VMStateField; */ struct VMStateInfo { const char *name; - int (*get)(QEMUFile *f, void *pv, size_t size, const VMStateField *field); - int (*put)(QEMUFile *f, void *pv, size_t size, const VMStateField *field, - JSONWriter *vmdesc); + int coroutine_mixed_fn (*get)(QEMUFile *f, void *pv, size_t size, + const VMStateField *field); + int coroutine_mixed_fn (*put)(QEMUFile *f, void *pv, size_t size, + const VMStateField *field, + JSONWriter *vmdesc); }; enum VMStateFlags { From 92e2e6a867334a990f8d29f07ca34e3162fdd6ec Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 5 Sep 2023 16:50:02 +0200 Subject: [PATCH 15/15] virtio: Drop out of coroutine context in virtio_load() virtio_load() as a whole should run in coroutine context because it reads from the migration stream and we don't want this to block. However, it calls virtio_set_features_nocheck() and devices don't expect their .set_features callback to run in a coroutine and therefore call functions that may not be called in coroutine context. To fix this, drop out of coroutine context for calling virtio_set_features_nocheck(). Without this fix, the following crash was reported: #0 __pthread_kill_implementation (threadid=, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 #1 0x00007efc738c05d3 in __pthread_kill_internal (signo=6, threadid=) at pthread_kill.c:78 #2 0x00007efc73873d26 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x00007efc738477f3 in __GI_abort () at abort.c:79 #4 0x00007efc7384771b in __assert_fail_base (fmt=0x7efc739dbcb8 "", assertion=assertion@entry=0x560aebfbf5cf "!qemu_in_coroutine()", file=file@entry=0x560aebfcd2d4 "../block/graph-lock.c", line=line@entry=275, function=function@entry=0x560aebfcd34d "void bdrv_graph_rdlock_main_loop(void)") at assert.c:92 #5 0x00007efc7386ccc6 in __assert_fail (assertion=0x560aebfbf5cf "!qemu_in_coroutine()", file=0x560aebfcd2d4 "../block/graph-lock.c", line=275, function=0x560aebfcd34d "void bdrv_graph_rdlock_main_loop(void)") at assert.c:101 #6 0x0000560aebcd8dd6 in bdrv_register_buf () #7 0x0000560aeb97ed97 in ram_block_added.llvm () #8 0x0000560aebb8303f in ram_block_add.llvm () #9 0x0000560aebb834fa in qemu_ram_alloc_internal.llvm () #10 0x0000560aebb2ac98 in vfio_region_mmap () #11 0x0000560aebb3ea0f in vfio_bars_register () #12 0x0000560aebb3c628 in vfio_realize () #13 0x0000560aeb90f0c2 in pci_qdev_realize () #14 0x0000560aebc40305 in device_set_realized () #15 0x0000560aebc48e07 in property_set_bool.llvm () #16 0x0000560aebc46582 in object_property_set () #17 0x0000560aebc4cd58 in object_property_set_qobject () #18 0x0000560aebc46ba7 in object_property_set_bool () #19 0x0000560aeb98b3ca in qdev_device_add_from_qdict () #20 0x0000560aebb1fbaf in virtio_net_set_features () #21 0x0000560aebb46b51 in virtio_set_features_nocheck () #22 0x0000560aebb47107 in virtio_load () #23 0x0000560aeb9ae7ce in vmstate_load_state () #24 0x0000560aeb9d2ee9 in qemu_loadvm_state_main () #25 0x0000560aeb9d45e1 in qemu_loadvm_state () #26 0x0000560aeb9bc32c in process_incoming_migration_co.llvm () #27 0x0000560aebeace56 in coroutine_trampoline.llvm () Cc: qemu-stable@nongnu.org Buglink: https://issues.redhat.com/browse/RHEL-832 Signed-off-by: Kevin Wolf Message-ID: <20230905145002.46391-3-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 309038fd46..969c25f4cf 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2825,8 +2825,9 @@ static int virtio_device_put(QEMUFile *f, void *opaque, size_t size, } /* A wrapper for use as a VMState .get function */ -static int virtio_device_get(QEMUFile *f, void *opaque, size_t size, - const VMStateField *field) +static int coroutine_mixed_fn +virtio_device_get(QEMUFile *f, void *opaque, size_t size, + const VMStateField *field) { VirtIODevice *vdev = VIRTIO_DEVICE(opaque); DeviceClass *dc = DEVICE_CLASS(VIRTIO_DEVICE_GET_CLASS(vdev)); @@ -2853,6 +2854,39 @@ static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val) return bad ? -1 : 0; } +typedef struct VirtioSetFeaturesNocheckData { + Coroutine *co; + VirtIODevice *vdev; + uint64_t val; + int ret; +} VirtioSetFeaturesNocheckData; + +static void virtio_set_features_nocheck_bh(void *opaque) +{ + VirtioSetFeaturesNocheckData *data = opaque; + + data->ret = virtio_set_features_nocheck(data->vdev, data->val); + aio_co_wake(data->co); +} + +static int coroutine_mixed_fn +virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val) +{ + if (qemu_in_coroutine()) { + VirtioSetFeaturesNocheckData data = { + .co = qemu_coroutine_self(), + .vdev = vdev, + .val = val, + }; + aio_bh_schedule_oneshot(qemu_get_current_aio_context(), + virtio_set_features_nocheck_bh, &data); + qemu_coroutine_yield(); + return data.ret; + } else { + return virtio_set_features_nocheck(vdev, val); + } +} + int virtio_set_features(VirtIODevice *vdev, uint64_t val) { int ret; @@ -2906,7 +2940,8 @@ size_t virtio_get_config_size(const VirtIOConfigSizeParams *params, return config_size; } -int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) +int coroutine_mixed_fn +virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) { int i, ret; int32_t config_len; @@ -3023,14 +3058,14 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) * host_features. */ uint64_t features64 = vdev->guest_features; - if (virtio_set_features_nocheck(vdev, features64) < 0) { + if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) { error_report("Features 0x%" PRIx64 " unsupported. " "Allowed features: 0x%" PRIx64, features64, vdev->host_features); return -1; } } else { - if (virtio_set_features_nocheck(vdev, features) < 0) { + if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) { error_report("Features 0x%x unsupported. " "Allowed features: 0x%" PRIx64, features, vdev->host_features);