* [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable
@ 2026-09-16 15:24 Denis V. Lunev
2026-09-16 15:24 ` [PATCH hci-8.0 1/5] qcow2: do not clear the dirty bit when reopening a read-only node #VSTOR-140073 Denis V. Lunev
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
A customer volume was cross-mapped onto itself by this bug, so hci-8.0
wants the fix even though the upstream series has not been merged yet.
The problem
-----------
qcow2_do_open() repairs an image carrying QCOW2_INCOMPAT_DIRTY, but only
for a node that is writable from the start:
if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
(s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
A node opened read-only skips it, correctly, since it resolves nothing
and writes nothing. Nothing revisits the question when that same node
later becomes writable, and bdrv_reopen() is not an exotic way to get
there: commit_active_start() and commit_start() both reopen the base
read-write for the duration of the job, so an ordinary block-commit onto
a read-only backing file is enough.
With lazy refcounts the refcount blocks are not written again once the
dirty bit is set, so an image left behind by a killed QEMU has an
on-disk refcount block that accounts for the metadata clusters and
nothing else. Every data cluster reads as free. s->free_cluster_index
starts at 0, so the first allocation after such a reopen starts at the
front of the image and hands out clusters that L2 entries still point
at.
The result is aliasing: two guest offsets mapped onto one host cluster,
so the guest reads back data belonging to some other offset. Nothing
about this fails. No I/O error is reported, the corrupt bit stays clear,
and a clean close clears the dirty bit as well, so no later open will
repair the image either. The only runtime witness, if something
eventually frees one of those clusters, is a bare
qcow2_free_clusters failed: Invalid argument
on stderr, with the guest none the wiser.
How it looked in production
---------------------------
A VM was killed while its storage was unavailable, leaving a 100 GiB
volume dirty. It came back with a snapshot-revert overlay on top, so the
volume itself was now the read-only backing file, and the overlay was
committed into it two and a half hours later. 8082 host clusters ended
up referenced by two L2 entries each, roughly 8 GiB of guest data
cross-mapped. The guest filesystem began failing metadata verification
on buffers holding fragments of unrelated files.
qemu-img check -r all makes the metadata self-consistent again, and then
honestly reports no errors, but it cannot un-alias anything. The guest
data stays wrong.
Reproducer
----------
Under a second, no guest and no block job required:
qemu-img create -f qcow2 -o compat=1.1,lazy_refcounts=on base.qcow2 1G
qemu-io -f qcow2 -c "write -P 0xaa 0 100M" -c flush \
-c "sigraise 9" base.qcow2
qemu-io -r -f qcow2 base.qcow2 \
<<< $'reopen -w\nwrite -P 0xbb 900M 100M\nquit'
qemu-io -r -f qcow2 -c "read -v 0 16" base.qcow2
# 00000000: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb
The flush is load-bearing: it writes out the L2 cache but not the
refcount blocks, which is exactly the asymmetry the bug needs. Guest
fsyncs supply it in production, so a long-running VM is the ideal
victim. qemu-img commit of an overlay reaches the same state through
commit_active_start().
The fix
-------
Patch 5 repairs the image in qcow2_reopen_commit_post(), the earliest
point at which the node is writable. That callback runs after the reopen
transaction has been committed and cannot reject it, so patch 3 gives it
a return value to report a node it has had to give up on, and patch 4
records the flags the reopen started from so the driver can tell a node
that has just become writable from one that was writable all along.
Patch 2 stops a reopen of a driverless node from crashing, which is how
a failed repair comes back through commit_clean(). Patch 1 is the
read-only header write that fails or asserts on a dirty image.
The repair runs under bdrv_drain_all(), so a block-commit onto a large
dirty base stalls guest I/O for the length of a full metadata scan. That
is the cost of the alternative being silent data loss; rejecting the
reopen would be cheap, but block-commit depends on it succeeding.
Port notes
----------
This is the hci-8.0 port of the v4 series posted upstream on 2026-08-24:
https://lore.kernel.org/qemu-devel/20260824133729.1141990-1-den@openvz.org/
It is still under review there, so the code may yet change. All five
patches cherry-pick onto hci-8.0 with no prerequisites and no conflicts,
and the diff is identical to the upstream one.
One thing differs, in patch 5: hci-8.0 creates qcow2 images with zstd
compression by default, so a fresh v3 image already carries
QCOW2_INCOMPAT_COMPRESSION. The iotests 040 helper therefore masks the
dirty bit out of the incompatible features rather than comparing the
whole word, which upstream can afford to do.
Built and tested on dev-cl04: iotests 039, 040 and 060 pass, and the
qcow2 auto group is otherwise unchanged. vz-mt-vendor fails there only
because that build was configured --disable-spice and so has no
qxl-vga device for the test to instantiate.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Denis V. Lunev (5):
qcow2: do not clear the dirty bit when reopening a read-only node
#VSTOR-140073
block: reject a reopen of an unusable node instead of crashing
#VSTOR-140073
block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073
block: remember the flags a reopen starts from #VSTOR-140073
qcow2: repair a dirty image when it becomes writable #VSTOR-140073
block.c | 55 ++++++++++++--
block/qcow2.c | 33 ++++++++-
include/block/block-common.h | 1 +
include/block/block_int-common.h | 11 ++-
qapi/block-core.json | 19 +++++
tests/qemu-iotests/039 | 110 +++++++++++++++++++++++++++
tests/qemu-iotests/039.out | 56 ++++++++++++++
tests/qemu-iotests/040 | 123 +++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
tests/qemu-iotests/060 | 30 ++++++++
tests/qemu-iotests/060.out | 13 ++++
11 files changed, 440 insertions(+), 15 deletions(-)
--
2.53.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH hci-8.0 1/5] qcow2: do not clear the dirty bit when reopening a read-only node #VSTOR-140073 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev @ 2026-09-16 15:24 ` Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 2/5] block: reject a reopen of an unusable node instead of crashing #VSTOR-140073 Denis V. Lunev ` (3 subsequent siblings) 4 siblings, 0 replies; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw) To: svt-core; +Cc: andrey.drobyshev, den From: Denis V. Lunev <den@openvz.org> qcow2_reopen_prepare() clears the dirty bit whenever the node is reopened read-only, with an unguarded header write. A read-only node can still be dirty, inherited from an earlier writable session, and it holds no BLK_PERM_WRITE to resolve that. A read-only to read-only reopen of a dirty image therefore fails outright: $ qemu-io -r -f qcow2 dirty.qcow2 <<< $'reopen -r\nquit' qemu-io: failed while preparing to reopen image 'dirty.qcow2' Where the file node below is writable the write is not refused early, and bdrv_co_write_req_prepare() aborts on its BLK_PERM_WRITE assertion instead. Clear it only for a node that is writable now, the predicate qcow2_do_open() already uses for the repair. bdrv_is_writable() also excludes an inactive node, whose header must not be touched either. Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Hanna Reitz <hreitz@redhat.com> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> Cc: qemu-stable@nongnu.org --- block/qcow2.c | 8 +++--- tests/qemu-iotests/039 | 50 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/039.out | 20 +++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 34498078f5e..3e0a219e156 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2190,9 +2190,11 @@ qcow2_reopen_prepare(BDRVReopenState *state,BlockReopenQueue *queue, goto fail; } - ret = qcow2_mark_clean(state->bs); - if (ret < 0) { - goto fail; + if (bdrv_is_writable(state->bs)) { + ret = qcow2_mark_clean(state->bs); + if (ret < 0) { + goto fail; + } } } diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 index 94a8bfe754e..3d0c073d65d 100755 --- a/tests/qemu-iotests/039 +++ b/tests/qemu-iotests/039 @@ -95,6 +95,40 @@ $QEMU_IMG info --image-opts \ # The dirty bit must still be set: this open never wrote any guest data _qcow2_dump_header | grep incompatible_features +echo +echo "== Read-only reopen must not clear the dirty bit ==" + +# A read-only node cannot write the header, and must keep the dirty bit +$QEMU_IO -r -c "reopen -r" -c "read -P 0x5a 0 512" "$TEST_IMG" \ + | _filter_qemu_io + +# The dirty bit must still be set +_qcow2_dump_header | grep incompatible_features + +echo +echo "== Read-only reopen must not write through a writable file node ==" + +# The write the header update needs is refused by the permission system +echo "{'execute': 'qmp_capabilities'} + {'execute': 'blockdev-reopen', + 'arguments': {'options': [{'node-name': 'drive', + 'driver': 'qcow2', + 'read-only': true, + 'file': 'prot'}]}} + {'execute': 'quit'}" \ + | $QEMU -qmp stdio -nographic -nodefaults \ + -blockdev "{'node-name': 'prot', + 'driver': 'file', + 'filename': '$TEST_IMG'}" \ + -blockdev "{'node-name': 'drive', + 'driver': 'qcow2', + 'file': 'prot', + 'read-only': true}" \ + | _filter_qmp + +# The dirty bit must still be set +_qcow2_dump_header | grep incompatible_features + echo echo "== Repairing the image file must succeed ==" @@ -108,6 +142,22 @@ echo "== Data should still be accessible after repair ==" $QEMU_IO -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io +echo +echo "== A read-write to read-only reopen must clear the dirty bit ==" + +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size + +# The kill keeps the close from clearing the bit, so the header shows what +# the reopen did with it +_NO_VALGRIND \ +$QEMU_IO -c "write -P 0x5a 0 512" \ + -c "reopen -r" \ + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ + | _filter_qemu_io + +# The dirty bit must not be set +_qcow2_dump_header | grep incompatible_features + echo echo "== Opening a dirty image read/write should repair it ==" diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out index c66361128f6..ce8ee577216 100644 --- a/tests/qemu-iotests/039.out +++ b/tests/qemu-iotests/039.out @@ -27,6 +27,19 @@ incompatible_features [0] == Read-only open must not crash on close == incompatible_features [0] +== Read-only reopen must not clear the dirty bit == +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +incompatible_features [0] + +== Read-only reopen must not write through a writable file node == +QMP_VERSION +{"return": {}} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} +{"return": {}} +incompatible_features [0] + == Repairing the image file must succeed == ERROR cluster 5 refcount=0 reference=1 Rebuilding refcount structure @@ -45,6 +58,13 @@ incompatible_features [] read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== A read-write to read-only reopen must clear the dirty bit == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) +incompatible_features [] + == Opening a dirty image read/write should repair it == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 wrote 512/512 bytes at offset 0 -- 2.53.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH hci-8.0 2/5] block: reject a reopen of an unusable node instead of crashing #VSTOR-140073 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 1/5] qcow2: do not clear the dirty bit when reopening a read-only node #VSTOR-140073 Denis V. Lunev @ 2026-09-16 15:24 ` Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 3/5] block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073 Denis V. Lunev ` (2 subsequent siblings) 4 siblings, 0 replies; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw) To: svt-core; +Cc: andrey.drobyshev, den From: Denis V. Lunev <den@openvz.org> qcow2_signal_corruption() drops bs->drv, so a node can lose its driver at any time and a reopen has to expect that. Both ends of the path assume otherwise: $ qemu-io -c "read 0 64k" -c "reopen -r" corrupt.qcow2 qcow2: Marking image as corrupt: Cluster allocation offset 0x1200 unaligned (L2 offset: 0x40000, L2 index: 0); ... Segmentation fault bdrv_reopen_queue_child() dereferences bs->drv while descending into the children the node opened itself, and bdrv_reopen_prepare() asserts on it. commit_clean() reopens the base of a commit job back to read-only, so a base which goes corrupt under the job arrives here too. There is nothing to reopen for such a node, so stop descending into its children and let bdrv_reopen_prepare() report what every caller of bdrv_reopen() already handles. bdrv_reopen_commit() and bdrv_reopen_abort() keep their assertion, only a prepared entry reaches them. Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Hanna Reitz <hreitz@redhat.com> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> Cc: qemu-stable@nongnu.org --- block.c | 14 +++++++++++++- tests/qemu-iotests/060 | 30 ++++++++++++++++++++++++++++++ tests/qemu-iotests/060.out | 13 +++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 7b27cb4edf3..44afa191930 100644 --- a/block.c +++ b/block.c @@ -4526,6 +4526,11 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs, !qdict_haskey(options, "backing.driver"); } + /* An unusable node is rejected by bdrv_reopen_prepare(), do not descend */ + if (!bs->drv) { + return bs_queue; + } + QLIST_FOREACH(child, &bs->children, next) { QDict *new_child_options = NULL; bool child_keep_old = keep_old_opts; @@ -4921,9 +4926,16 @@ bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, bool drv_prepared = false; assert(reopen_state != NULL); - assert(reopen_state->bs->drv != NULL); GLOBAL_STATE_CODE(); + drv = reopen_state->bs->drv; + if (drv == NULL) { + GRAPH_RDLOCK_GUARD_MAINLOOP(); + + error_setg(errp, "Block node '%s' has no driver left to reopen", + bdrv_get_device_or_node_name(reopen_state->bs)); + return -ENOMEDIUM; + } /* This function and each driver's bdrv_reopen_prepare() remove * entries from reopen_state->options as they are processed, so diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060 index 5cd21a6f686..ce49fc34ecc 100755 --- a/tests/qemu-iotests/060 +++ b/tests/qemu-iotests/060 @@ -486,6 +486,36 @@ echo # Image should not have been marked corrupt _img_info --format-specific | grep 'corrupt:' +echo +echo "=== Testing the reopen of an image corrupted at runtime ===" +echo + +_make_test_img 64M +poke_file "$TEST_IMG" "$l1_offset" "\x00\x00\x00\x00\x2a\x2a\x2a\x2a" + +# The read leaves the node unusable, the reopen must report that +echo "{'execute': 'qmp_capabilities'} + {'execute': 'human-monitor-command', + 'arguments': {'command-line': 'qemu-io drive \"read 0 512\"'}} + {'execute': 'blockdev-reopen', + 'arguments': {'options': [{'node-name': 'drive', + 'driver': 'qcow2', + 'read-only': true, + 'file': { + 'driver': 'file', + 'filename': '$TEST_IMG' + }}]}} + {'execute': 'quit'}" \ + | $QEMU -qmp stdio -nographic -nodefaults \ + -blockdev "{'node-name': 'drive', + 'driver': 'qcow2', + 'file': { + 'driver': 'file', + 'filename': '$TEST_IMG' + }}" \ + 2>&1 \ + | _filter_qmp | _filter_qemu_io + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out index a37bf446e94..ad1912a43be 100644 --- a/tests/qemu-iotests/060.out +++ b/tests/qemu-iotests/060.out @@ -436,4 +436,17 @@ qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0); fur {"return": {}} corrupt: false + +=== Testing the reopen of an image corrupted at runtime === + +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +QMP_VERSION +{"return": {}} +qcow2: Marking image as corrupt: L2 table offset 0x2a2a2a00 unaligned (L1 index: 0); further corruption events will be suppressed +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2 table offset 0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": true}} +read failed: Input/output error +{"return": ""} +{"error": {"class": "GenericError", "desc": "Block node 'drive' has no driver left to reopen"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} +{"return": {}} *** done -- 2.53.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH hci-8.0 3/5] block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 1/5] qcow2: do not clear the dirty bit when reopening a read-only node #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 2/5] block: reject a reopen of an unusable node instead of crashing #VSTOR-140073 Denis V. Lunev @ 2026-09-16 15:24 ` Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 4/5] block: remember the flags a reopen starts from #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 Denis V. Lunev 4 siblings, 0 replies; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw) To: svt-core; +Cc: andrey.drobyshev, den From: Denis V. Lunev <den@openvz.org> The callback runs after bdrv_reopen_multiple() has committed the transaction, so it cannot reject the reopen. It can still find that the node it has just made writable is unusable, and has no way to say so: bdrv_reopen() returns success and the caller carries on. Give it a return value and an Error argument. The reopen stays committed, the error only reports that the node is gone. Every queued node still gets its callback, the first error is the one reported. A callback may leave its node without a driver, and so may the I/O of a later bdrv_reopen_prepare(), so do not assume that the nodes still ahead of it in the queue have one. qcow2 is the only implementation and does not fail yet. An error therefore means one of two things now, either that the reopen was denied and nothing changed, or that it went through and left a tree which cannot be used. Nothing is undone in the second case: the node is beyond repair by another reopen, and a caller which reacts to the error by reopening anything is making it worse. bdrv_reopen_multiple() says so, nothing else changes. Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Hanna Reitz <hreitz@redhat.com> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> Cc: qemu-stable@nongnu.org --- block.c | 24 +++++++++++++++++++++--- block/qcow2.c | 4 +++- include/block/block_int-common.h | 9 +++++++-- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index 44afa191930..0978ab2a0ea 100644 --- a/block.c +++ b/block.c @@ -4622,6 +4622,10 @@ void bdrv_reopen_queue_free(BlockReopenQueue *bs_queue) * If all devices prepare successfully, then the changes are committed * to all devices. * + * A failure means either that the reopen was denied and nothing changed, + * or that it went through and a driver then found the node unusable. In + * the second case nothing is undone and the tree is no longer usable. + * * All affected nodes must be drained between bdrv_reopen_queue() and * bdrv_reopen_multiple(). * @@ -4698,15 +4702,29 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) tran_commit(tran); bdrv_graph_wrunlock(); + ret = 0; QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) { BlockDriverState *bs = bs_entry->state.bs; + Error *local_err = NULL; + int commit_ret; - if (bs->drv->bdrv_reopen_commit_post) { - bs->drv->bdrv_reopen_commit_post(&bs_entry->state); + if (!bs->drv || !bs->drv->bdrv_reopen_commit_post) { + continue; + } + + commit_ret = bs->drv->bdrv_reopen_commit_post(&bs_entry->state, + &local_err); + assert(commit_ret >= 0 || local_err); + + if (commit_ret < 0 && ret == 0) { + /* Committed already, so report the first failure and go on */ + error_propagate(errp, local_err); + ret = commit_ret; + } else { + error_free(local_err); } } - ret = 0; goto cleanup; abort: diff --git a/block/qcow2.c b/block/qcow2.c index 3e0a219e156..ddad39a3084 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2233,7 +2233,7 @@ static void qcow2_reopen_commit(BDRVReopenState *state) g_free(state->opaque); } -static void qcow2_reopen_commit_post(BDRVReopenState *state) +static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp) { GRAPH_RDLOCK_GUARD_MAINLOOP(); @@ -2251,6 +2251,8 @@ static void qcow2_reopen_commit_post(BDRVReopenState *state) bdrv_get_node_name(state->bs)); } } + + return 0; } static void qcow2_reopen_abort(BDRVReopenState *state) diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 50848de07f4..7afbe8c4ae5 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -262,8 +262,13 @@ struct BlockDriver { BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp); void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)( BDRVReopenState *reopen_state); - void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)( - BDRVReopenState *reopen_state); + /* + * Runs once the reopen is committed, so it cannot reject it. Returns 0, + * or a negative errno with @errp set to report that the node it has + * just reopened is unusable, which it may leave without a driver. + */ + int GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)( + BDRVReopenState *reopen_state, Error **errp); void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)( BDRVReopenState *reopen_state); void (*bdrv_join_options)(QDict *options, QDict *old_options); -- 2.53.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH hci-8.0 4/5] block: remember the flags a reopen starts from #VSTOR-140073 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev ` (2 preceding siblings ...) 2026-09-16 15:24 ` [PATCH hci-8.0 3/5] block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073 Denis V. Lunev @ 2026-09-16 15:24 ` Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 Denis V. Lunev 4 siblings, 0 replies; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw) To: svt-core; +Cc: andrey.drobyshev, den From: Denis V. Lunev <den@openvz.org> bdrv_reopen_commit() updates bs->open_flags, so by the time .bdrv_reopen_commit_post() runs a driver can no longer tell whether the node has just become writable or was writable all along. Only the transition is worth reacting to. Record the flags while the queue is built, next to the other pre-reopen state BDRVReopenState already keeps, and let a driver ask about them the way it asks about the node itself. The predicate which bdrv_is_writable_after_reopen() spells out gets a name for that, and stays private to block.c. Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Hanna Reitz <hreitz@redhat.com> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> Cc: qemu-stable@nongnu.org --- block.c | 17 ++++++++++++++--- include/block/block-common.h | 1 + include/block/block_int-common.h | 2 ++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 0978ab2a0ea..236a30618dc 100644 --- a/block.c +++ b/block.c @@ -2188,14 +2188,18 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs) return bs->open_flags; } +/* An inactive node may not be written to, even though it is not read-only */ +static bool bdrv_flags_writable(int flags) +{ + return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR; +} + /* Returns whether the image file can be written to after the reopen queue @q * has been successfully applied, or right now if @q is NULL. */ static bool bdrv_is_writable_after_reopen(BlockDriverState *bs, BlockReopenQueue *q) { - int flags = bdrv_reopen_get_flags(q, bs); - - return (flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) == BDRV_O_RDWR; + return bdrv_flags_writable(bdrv_reopen_get_flags(q, bs)); } /* @@ -2209,6 +2213,12 @@ bool bdrv_is_writable(BlockDriverState *bs) return bdrv_is_writable_after_reopen(bs, NULL); } +bool bdrv_reopen_was_writable(const BDRVReopenState *state) +{ + GLOBAL_STATE_CODE(); + return bdrv_flags_writable(state->old_flags); +} + static char *bdrv_child_user_desc(BdrvChild *c) { GLOBAL_STATE_CODE(); @@ -4513,6 +4523,7 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs, bs_entry->state.options = options; bs_entry->state.explicit_options = explicit_options; bs_entry->state.flags = flags; + bs_entry->state.old_flags = bs->open_flags; /* * If keep_old_opts is false then it means that unspecified diff --git a/include/block/block-common.h b/include/block/block-common.h index 3c5b6aa3f58..b4558fb245f 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -359,6 +359,7 @@ typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; typedef struct BDRVReopenState { BlockDriverState *bs; int flags; + int old_flags; /* bs->open_flags is updated on commit */ BlockdevDetectZeroesOptions detect_zeroes; bool backing_missing; bool mig_ign_bitmaps; diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 7afbe8c4ae5..4062d28fca6 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1400,6 +1400,8 @@ char *create_tmp_file(Error **errp); void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix, QDict *options); +bool bdrv_reopen_was_writable(const BDRVReopenState *state); + int bdrv_check_qiov_request(int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t qiov_offset, -- 2.53.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev ` (3 preceding siblings ...) 2026-09-16 15:24 ` [PATCH hci-8.0 4/5] block: remember the flags a reopen starts from #VSTOR-140073 Denis V. Lunev @ 2026-09-16 15:24 ` Denis V. Lunev 2026-09-16 16:26 ` Andrey Drobyshev 4 siblings, 1 reply; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 15:24 UTC (permalink / raw) To: svt-core; +Cc: andrey.drobyshev, den From: Denis V. Lunev <den@openvz.org> A dirty image must be repaired before anything allocates a cluster in it. qcow2_do_open() does that, but only for a node that is writable from the start. A node opened read-only skips it, and nothing revisits the question once that node becomes writable, which block-commit does routinely: commit_active_start() and commit_start() reopen the base read-write for the duration of the job. With lazy refcounts the on-disk refcount block then still accounts for the metadata clusters only, so the allocator restarts at the front of the image and hands out clusters that L2 entries point at. Two guest offsets end up sharing one host cluster. Nothing fails, the corrupt bit stays clear, and a clean close clears the dirty bit, so no later open repairs the image either. The bit also stays set for the whole writable session, so a node which is merely writable says nothing. Refusing the reopen instead is simpler and keeps it atomic, but it leaves nowhere to go: the base belongs to a chain the VM has open, so the qemu-img check -r such an error would ask for cannot take the write lock it needs. The repair does the trick in most cases anyway. Do the repair in qcow2_reopen_commit_post(), the earliest point where the node is writable. An inactive node is skipped: bdrv_activate() calls qcow2_do_open() again through qcow2_co_invalidate_cache(). commit_post cannot reject the reopen, so a failed repair takes the driver away from the node instead, which is what stops writes from aliasing live clusters. qcow2_signal_corruption() does that as well, but it also sends BLOCK_IMAGE_CORRUPTED and sets the corrupt bit, which qcow2_do_open() honours by refusing every later read-write open. The image is dirty and unrepaired, not corrupt, and qemu-img check -r still fixes it, so neither belongs here. Return the error and skip the bitmaps. Signed-off-by: Denis V. Lunev <den@openvz.org> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> CC: Kevin Wolf <kwolf@redhat.com> CC: Hanna Reitz <hreitz@redhat.com> CC: Eric Blake <eblake@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> Cc: qemu-stable@nongnu.org --- block/qcow2.c | 21 +++++++ qapi/block-core.json | 19 ++++++ tests/qemu-iotests/039 | 60 ++++++++++++++++++ tests/qemu-iotests/039.out | 36 +++++++++++ tests/qemu-iotests/040 | 123 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/040.out | 4 +- 6 files changed, 261 insertions(+), 2 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index ddad39a3084..be346f0339b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2235,8 +2235,29 @@ static void qcow2_reopen_commit(BDRVReopenState *state) static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp) { + ERRP_GUARD(); + BDRVQcow2State *s = state->bs->opaque; + GRAPH_RDLOCK_GUARD_MAINLOOP(); + if (!bdrv_reopen_was_writable(state) && bdrv_is_writable(state->bs) && + (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { + BdrvCheckResult result = {0}; + int ret; + + ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); + if (ret < 0 || result.check_errors || !state->bs->drv) { + ret = ret < 0 ? ret : -EIO; + /* No write may reach an image whose refcounts are unaccounted */ + state->bs->drv = NULL; + error_setg_errno(errp, -ret, "Could not repair dirty image '%s'", + bdrv_get_device_or_node_name(state->bs)); + error_append_hint(errp, "The image is left dirty and this node " + "holds it open until the node is removed\n"); + return ret; + } + } + if (state->flags & BDRV_O_RDWR) { Error *local_err = NULL; diff --git a/qapi/block-core.json b/qapi/block-core.json index c036a3a8858..6fb8c31210a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1843,6 +1843,9 @@ # r/w -> r/o, if needed). The new backing file string is written into # the image file metadata, and the QEMU internal strings are updated. # +# A dirty qcow2 image is repaired during that reopen, which blocks +# other requests and can fail the command. +# # @image-node-name: The name of the block driver state node of the # image to modify. The "device" argument is used to verify # "image-node-name" is in the chain described by "device". @@ -1882,6 +1885,9 @@ # size to match the size of the smaller top, you can safely truncate # it yourself once the commit operation successfully completes. # +# The base is opened read-write. A dirty qcow2 base is repaired +# first, which blocks other requests and can fail the command. +# # @job-id: identifier for the newly-created block job. If omitted, # the device name will be used. (Since 2.7) # @@ -2919,6 +2925,9 @@ # On successful completion the image file is updated to drop the # backing file and the `BLOCK_JOB_COMPLETED` event is emitted. # +# The top image is opened read-write. A dirty qcow2 image is repaired +# first, which blocks other requests and can fail the command. +# # In case @device is a filter node, `block-stream` modifies the first # non-filter overlay node below it to point to the new backing node # instead of modifying @device itself. @@ -5070,6 +5079,16 @@ # transaction, so if one of them fails then the whole transaction is # cancelled. # +# An error is also returned when a device cannot be used once it has +# been reopened. Such a reopen is not undone, so an error does not +# always mean that nothing has changed. A node the driver gave up on +# serves nothing at all: it keeps its image open, makes +# `query-named-block-nodes` fail for as long as it is in the graph, +# and `blockdev-del` removes it only once nothing refers to it. +# +# Reopening a dirty qcow2 image read-write repairs it first, which +# blocks other requests and can fail the command. +# # The command receives a list of block devices to reopen. For each # one of them, the top-level @node-name option (from `BlockdevOptions`) # must be specified and is used to select the block device to be diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 index 3d0c073d65d..3f37c36ca86 100755 --- a/tests/qemu-iotests/039 +++ b/tests/qemu-iotests/039 @@ -33,6 +33,7 @@ status=1 # failure is the default! _cleanup() { _cleanup_test_img + rm -f "$TEST_DIR/blkdebug.conf" } trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -176,6 +177,65 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io # The dirty bit must not be set _qcow2_dump_header | grep incompatible_features +echo +echo "== Reopening a dirty image read/write should repair it ==" + +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size + +_NO_VALGRIND \ +$QEMU_IO -c "write -P 0x5a 0 512" \ + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ + | _filter_qemu_io + +# The dirty bit must be set +_qcow2_dump_header | grep incompatible_features + +# Without the repair this write would alias the cluster at offset 0 +$QEMU_IO -r -c "reopen -w" \ + -c "write -P 0xb1 1M 512" \ + -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io + +_check_test_img + +echo +echo "== A read/write reopen must not check the image ==" + +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size + +_NO_VALGRIND \ +$QEMU_IO -c "write -P 0x5a 0 512" \ + -c "reopen -o l2-cache-size=1M" \ + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ + | _filter_qemu_io + +# The dirty bit must still be set, it belongs to the running session +_qcow2_dump_header | grep incompatible_features + +echo +echo "== A failed repair must fail the reopen ==" + +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size + +_NO_VALGRIND \ +$QEMU_IO -c "write -P 0x5a 0 512" \ + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ + | _filter_qemu_io + +cat > "$TEST_DIR/blkdebug.conf" <<EOF +[inject-error] +event = "none" +iotype = "write" +errno = "5" +EOF + +# The repair cannot write, so the reopen itself must report the failure +$QEMU_IO -r -c "reopen -w" -c "read -P 0x5a 0 512" \ + "blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG" 2>&1 \ + | _filter_testdir | _filter_qemu_io | _filter_generated_node_ids + +# The corrupt bit needs a write of its own, so the image is only left dirty +_qcow2_dump_header | grep incompatible_features + echo echo "== Creating an image file with lazy_refcounts=off ==" diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out index ce8ee577216..cc6ca3ab95b 100644 --- a/tests/qemu-iotests/039.out +++ b/tests/qemu-iotests/039.out @@ -79,6 +79,42 @@ wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) incompatible_features [] +== Reopening a dirty image read/write should repair it == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) +incompatible_features [0] +ERROR cluster 5 refcount=0 reference=1 +Rebuilding refcount structure +Repairing cluster 1 refcount=1 reference=0 +Repairing cluster 2 refcount=1 reference=0 +wrote 512/512 bytes at offset 1048576 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +No errors were found on the image. + +== A read/write reopen must not check the image == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) +incompatible_features [0] + +== A failed repair must fail the reopen == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) +ERROR cluster 5 refcount=0 reference=1 +Rebuilding refcount structure +qemu-io: ERROR writing refblock: Input/output error +qemu-io: Could not repair dirty image 'NODE_NAME': Input/output error +The image is left dirty and this node holds it open until the node is removed +read failed: No medium found +incompatible_features [0] + == Creating an image file with lazy_refcounts=off == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 wrote 512/512 bytes at offset 0 diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 5c18e413ec9..b25c76183a1 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -951,6 +951,129 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase): self.vm.qmp('block-job-complete', device='commit') self.vm.event_wait('BLOCK_JOB_COMPLETED') +QCOW2_INCOMPAT_FEATURES_OFFSET = 72 +QCOW2_INCOMPAT_DIRTY = 1 << 0 + +image_size = 4 * 1024 * 1024 +dirty_base = os.path.join(iotests.test_dir, 'dirty-base.img') +mid = os.path.join(iotests.test_dir, 'dirty-mid.img') +top = os.path.join(iotests.test_dir, 'dirty-top.img') + + +class TestCommitDirtyBase(iotests.QMPTestCase): + def setUp(self) -> None: + if iotests.imgfmt != 'qcow2': + self.case_skip('the dirty bit is a qcow2 feature') + iotests.qemu_img_create('-f', iotests.imgfmt, '-o', + 'compat=1.1,lazy_refcounts=on', dirty_base, + str(image_size)) + # Killing the process leaves the refcounts of the written cluster stale + iotests.qemu_io_popen('-t', 'writethrough', + '-c', 'write -P 0x5a 0 512', + '-c', 'sigraise 9', dirty_base).communicate() + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', dirty_base, + '-F', iotests.imgfmt, mid) + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid, + '-F', iotests.imgfmt, top) + # The commit has to allocate for this, which is where the stale + # refcounts hand out the cluster holding the data written above + qemu_io('-c', 'write -P 0xb1 1M 512', mid) + + self.vm = iotests.VM() + self.vm.launch() + self.vm.cmd('blockdev-add', driver='file', filename=dirty_base, + node_name='base-file') + + self.assertTrue(self.is_dirty()) + + def tearDown(self) -> None: + if self.vm.is_running(): + self.vm.shutdown() + for image in (dirty_base, mid, top): + os.remove(image) + + def add_chain(self, base_file: str) -> None: + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file=base_file, + node_name='base', read_only=True) + self.vm.cmd('blockdev-add', driver='file', filename=mid, + node_name='mid-file') + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='mid-file', + node_name='mid', backing='base') + self.vm.cmd('blockdev-add', driver='file', filename=top, + node_name='top-file') + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='top-file', + node_name='top', backing='mid') + + def check_base(self) -> None: + result = iotests.qemu_img_check(dirty_base) + self.assertEqual(result['check-errors'], 0) + self.assertEqual(result.get('corruptions', 0), 0) + # Without the repair the commit would have aliased this cluster + qemu_io('-c', 'read -P 0x5a 0 512', '-c', 'read -P 0xb1 1M 512', + dirty_base) + + def is_dirty(self) -> bool: + with open(dirty_base, 'rb') as img: + img.seek(QCOW2_INCOMPAT_FEATURES_OFFSET) + features = struct.unpack('>Q', img.read(8))[0] + return bool(features & QCOW2_INCOMPAT_DIRTY) + + def test_commit_repairs_base(self) -> None: + self.add_chain('base-file') + + self.vm.cmd('block-commit', job_id='job0', device='top', + top_node='mid', base_node='base') + self.wait_until_completed(drive='job0') + + self.vm.shutdown() + self.assertFalse(self.is_dirty()) + self.check_base() + + def test_active_commit_repairs_base(self) -> None: + self.add_chain('base-file') + + # Without top-node the whole chain commits, through + # commit_active_start() rather than commit_start() + self.vm.cmd('block-commit', job_id='job0', device='top', + base_node='base') + self.complete_and_wait(drive='job0') + + self.vm.shutdown() + self.assertFalse(self.is_dirty()) + self.check_base() + + def test_failed_repair_fails_the_commit(self) -> None: + self.vm.cmd('blockdev-add', driver='blkdebug', image='base-file', + node_name='base-blkdebug', + inject_error=[{'event': 'none', 'iotype': 'write', + 'errno': 5}]) + self.add_chain('base-blkdebug') + + result = self.vm.qmp('block-commit', job_id='job0', device='top', + top_node='mid', base_node='base') + self.assert_qmp(result, 'error/class', 'GenericError') + self.assertIn("Could not repair dirty image 'base'", + result['error']['desc']) + + # The base is left in the graph, and nothing can be queried while + # it is there + result = self.vm.qmp('query-named-block-nodes', flat=True) + self.assert_qmp(result, 'error/desc', 'Block device base is ejected') + + # It only goes away once nothing refers to it + result = self.vm.qmp('blockdev-del', node_name='base') + self.assert_qmp(result, 'error/desc', + "Node 'base' is busy: node is used as backing hd of " + "'mid'") + + # Marking the image corrupt needs a write of its own, which fails too + self.assertTrue(self.is_dirty()) + + # Nothing can use the base any more, and that is what is reported + result = self.vm.qmp('block-commit', job_id='job1', device='top', + top_node='mid', base_node='base') + self.assert_qmp(result, 'error/desc', 'Device has no medium') + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2', 'qed'], supported_protocols=['file']) diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out index 1bb1dc5f0ea..f3cbf73a01d 100644 --- a/tests/qemu-iotests/040.out +++ b/tests/qemu-iotests/040.out @@ -1,5 +1,5 @@ -................................................................. +.................................................................... ---------------------------------------------------------------------- -Ran 65 tests +Ran 68 tests OK -- 2.53.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 2026-09-16 15:24 ` [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 Denis V. Lunev @ 2026-09-16 16:26 ` Andrey Drobyshev 2026-09-16 16:28 ` Denis V. Lunev 0 siblings, 1 reply; 8+ messages in thread From: Andrey Drobyshev @ 2026-09-16 16:26 UTC (permalink / raw) To: Denis V. Lunev, svt-core Other commits appear to be fine, but this commit has several issues: - Major: a failed repair makes a readable image unreadable. state->bs->drv = NULL at block/qcow2.c:2251 blocks reads as well as writes. The base of a running chain, or the active layer in the stream_start() case, answers every guest read with ENOMEDIUM from then on, for an image whose L1/L2 metadata is intact and was serving reads a moment earlier. Test 039's own expected output shows it: read failed: No medium found. Only allocation has to be stopped. A per-node gate in BDRVQcow2State that fails pwritev/pwrite_zeroes/pdiscard/truncate with -EIO (and skips qcow2_mark_clean() in qcow2_inactivate() so the dirty bit survives close) keeps the guest running and the job simply fails. The current approach also makes query-named-block-nodes/query-block fail wholesale while the node exists, and in the commit case the node cannot be deleted until the VM is restarted. - Major: use-after-free through the cache-clean coroutine. With bs->drv == NULL, bdrv_close() (block.c:5259) never calls qcow2_close(), so cache_clean_timer_del_and_wait() never runs, and bdrv_close() then frees bs->opaque. The coroutine at block/qcow2.c:895 is still sleeping on s->cache_clean_timer_wake (600 s default on Linux) and wakes on freed memory. Pre-existing for qcow2_signal_corruption(fatal), but this patch adds a second path and is the right place to stop the coroutine before dropping the driver. The same skipped close also leaks the L1 table, refcount table, both caches, crypto and bitmap state. - Major: the check runs under bdrv_drain_all_begin(). bdrv_reopen_queue() drains every node (block.c:4593), and bdrv_check() polls from the main loop. A full refcount scan plus rebuild of a large image stalls all guest I/O on every disk and the monitor for its whole duration, as a side effect of block-commit/block-stream/change-backing-file. The QAPI text discloses it, but the cover letter should quantify it and argue why this beats failing the command. - Medium (downstream only): the new condition ignores s->keep_dirty, unlike qcow2_do_open() at block/qcow2.c:2025. A node opened with __vz_keep-dirty=on, then reopened read-write, gets checked and its dirty bit cleared, breaking that option's contract. Add !s->keep_dirty in the hci branch. - Low: after the failure the node keeps BDRV_O_RDWR, BLK_PERM_WRITE on bs->file and the write lock, so the qemu-img check -r the hint recommends is impossible until the node is gone, which for a commit base means VM shutdown. Say so in the hint or the docs. - Low: test 040 asserts exact error strings owned by other code ("Node 'base' is busy: node is used as backing hd of 'mid'", "Block device base is ejected"), which makes it brittle. On 9/16/26 6:24 PM, Denis V. Lunev wrote: > From: Denis V. Lunev <den@openvz.org> > > A dirty image must be repaired before anything allocates a cluster in > it. qcow2_do_open() does that, but only for a node that is writable > from the start. A node opened read-only skips it, and nothing revisits > the question once that node becomes writable, which block-commit does > routinely: commit_active_start() and commit_start() reopen the base > read-write for the duration of the job. > > With lazy refcounts the on-disk refcount block then still accounts for > the metadata clusters only, so the allocator restarts at the front of > the image and hands out clusters that L2 entries point at. Two guest > offsets end up sharing one host cluster. Nothing fails, the corrupt bit > stays clear, and a clean close clears the dirty bit, so no later open > repairs the image either. The bit also stays set for the whole writable > session, so a node which is merely writable says nothing. > > Refusing the reopen instead is simpler and keeps it atomic, but it > leaves nowhere to go: the base belongs to a chain the VM has open, so > the qemu-img check -r such an error would ask for cannot take the write > lock it needs. The repair does the trick in most cases anyway. > > Do the repair in qcow2_reopen_commit_post(), the earliest point where > the node is writable. An inactive node is skipped: bdrv_activate() calls > qcow2_do_open() again through qcow2_co_invalidate_cache(). > > commit_post cannot reject the reopen, so a failed repair takes the > driver away from the node instead, which is what stops writes from > aliasing live clusters. qcow2_signal_corruption() does that as well, > but it also sends BLOCK_IMAGE_CORRUPTED and sets the corrupt bit, > which qcow2_do_open() honours by refusing every later read-write open. > The image is dirty and unrepaired, not corrupt, and qemu-img check -r > still fixes it, so neither belongs here. Return the error and skip > the bitmaps. > > Signed-off-by: Denis V. Lunev <den@openvz.org> > Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Hanna Reitz <hreitz@redhat.com> > CC: Eric Blake <eblake@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> > Cc: qemu-stable@nongnu.org > --- > block/qcow2.c | 21 +++++++ > qapi/block-core.json | 19 ++++++ > tests/qemu-iotests/039 | 60 ++++++++++++++++++ > tests/qemu-iotests/039.out | 36 +++++++++++ > tests/qemu-iotests/040 | 123 +++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/040.out | 4 +- > 6 files changed, 261 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index ddad39a3084..be346f0339b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2235,8 +2235,29 @@ static void qcow2_reopen_commit(BDRVReopenState *state) > > static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp) > { > + ERRP_GUARD(); > + BDRVQcow2State *s = state->bs->opaque; > + > GRAPH_RDLOCK_GUARD_MAINLOOP(); > > + if (!bdrv_reopen_was_writable(state) && bdrv_is_writable(state->bs) && > + (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { > + BdrvCheckResult result = {0}; > + int ret; > + > + ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); > + if (ret < 0 || result.check_errors || !state->bs->drv) { > + ret = ret < 0 ? ret : -EIO; > + /* No write may reach an image whose refcounts are unaccounted */ > + state->bs->drv = NULL; > + error_setg_errno(errp, -ret, "Could not repair dirty image '%s'", > + bdrv_get_device_or_node_name(state->bs)); > + error_append_hint(errp, "The image is left dirty and this node " > + "holds it open until the node is removed\n"); > + return ret; > + } > + } > + > if (state->flags & BDRV_O_RDWR) { > Error *local_err = NULL; > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index c036a3a8858..6fb8c31210a 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1843,6 +1843,9 @@ > # r/w -> r/o, if needed). The new backing file string is written into > # the image file metadata, and the QEMU internal strings are updated. > # > +# A dirty qcow2 image is repaired during that reopen, which blocks > +# other requests and can fail the command. > +# > # @image-node-name: The name of the block driver state node of the > # image to modify. The "device" argument is used to verify > # "image-node-name" is in the chain described by "device". > @@ -1882,6 +1885,9 @@ > # size to match the size of the smaller top, you can safely truncate > # it yourself once the commit operation successfully completes. > # > +# The base is opened read-write. A dirty qcow2 base is repaired > +# first, which blocks other requests and can fail the command. > +# > # @job-id: identifier for the newly-created block job. If omitted, > # the device name will be used. (Since 2.7) > # > @@ -2919,6 +2925,9 @@ > # On successful completion the image file is updated to drop the > # backing file and the `BLOCK_JOB_COMPLETED` event is emitted. > # > +# The top image is opened read-write. A dirty qcow2 image is repaired > +# first, which blocks other requests and can fail the command. > +# > # In case @device is a filter node, `block-stream` modifies the first > # non-filter overlay node below it to point to the new backing node > # instead of modifying @device itself. > @@ -5070,6 +5079,16 @@ > # transaction, so if one of them fails then the whole transaction is > # cancelled. > # > +# An error is also returned when a device cannot be used once it has > +# been reopened. Such a reopen is not undone, so an error does not > +# always mean that nothing has changed. A node the driver gave up on > +# serves nothing at all: it keeps its image open, makes > +# `query-named-block-nodes` fail for as long as it is in the graph, > +# and `blockdev-del` removes it only once nothing refers to it. > +# > +# Reopening a dirty qcow2 image read-write repairs it first, which > +# blocks other requests and can fail the command. > +# > # The command receives a list of block devices to reopen. For each > # one of them, the top-level @node-name option (from `BlockdevOptions`) > # must be specified and is used to select the block device to be > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 > index 3d0c073d65d..3f37c36ca86 100755 > --- a/tests/qemu-iotests/039 > +++ b/tests/qemu-iotests/039 > @@ -33,6 +33,7 @@ status=1 # failure is the default! > _cleanup() > { > _cleanup_test_img > + rm -f "$TEST_DIR/blkdebug.conf" > } > trap "_cleanup; exit \$status" 0 1 2 3 15 > > @@ -176,6 +177,65 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io > # The dirty bit must not be set > _qcow2_dump_header | grep incompatible_features > > +echo > +echo "== Reopening a dirty image read/write should repair it ==" > + > +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size > + > +_NO_VALGRIND \ > +$QEMU_IO -c "write -P 0x5a 0 512" \ > + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ > + | _filter_qemu_io > + > +# The dirty bit must be set > +_qcow2_dump_header | grep incompatible_features > + > +# Without the repair this write would alias the cluster at offset 0 > +$QEMU_IO -r -c "reopen -w" \ > + -c "write -P 0xb1 1M 512" \ > + -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io > + > +_check_test_img > + > +echo > +echo "== A read/write reopen must not check the image ==" > + > +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size > + > +_NO_VALGRIND \ > +$QEMU_IO -c "write -P 0x5a 0 512" \ > + -c "reopen -o l2-cache-size=1M" \ > + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ > + | _filter_qemu_io > + > +# The dirty bit must still be set, it belongs to the running session > +_qcow2_dump_header | grep incompatible_features > + > +echo > +echo "== A failed repair must fail the reopen ==" > + > +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size > + > +_NO_VALGRIND \ > +$QEMU_IO -c "write -P 0x5a 0 512" \ > + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ > + | _filter_qemu_io > + > +cat > "$TEST_DIR/blkdebug.conf" <<EOF > +[inject-error] > +event = "none" > +iotype = "write" > +errno = "5" > +EOF > + > +# The repair cannot write, so the reopen itself must report the failure > +$QEMU_IO -r -c "reopen -w" -c "read -P 0x5a 0 512" \ > + "blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG" 2>&1 \ > + | _filter_testdir | _filter_qemu_io | _filter_generated_node_ids > + > +# The corrupt bit needs a write of its own, so the image is only left dirty > +_qcow2_dump_header | grep incompatible_features > + > echo > echo "== Creating an image file with lazy_refcounts=off ==" > > diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out > index ce8ee577216..cc6ca3ab95b 100644 > --- a/tests/qemu-iotests/039.out > +++ b/tests/qemu-iotests/039.out > @@ -79,6 +79,42 @@ wrote 512/512 bytes at offset 0 > 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > incompatible_features [] > > +== Reopening a dirty image read/write should repair it == > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 > +wrote 512/512 bytes at offset 0 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) > +incompatible_features [0] > +ERROR cluster 5 refcount=0 reference=1 > +Rebuilding refcount structure > +Repairing cluster 1 refcount=1 reference=0 > +Repairing cluster 2 refcount=1 reference=0 > +wrote 512/512 bytes at offset 1048576 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +read 512/512 bytes at offset 0 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +No errors were found on the image. > + > +== A read/write reopen must not check the image == > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 > +wrote 512/512 bytes at offset 0 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) > +incompatible_features [0] > + > +== A failed repair must fail the reopen == > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 > +wrote 512/512 bytes at offset 0 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) > +ERROR cluster 5 refcount=0 reference=1 > +Rebuilding refcount structure > +qemu-io: ERROR writing refblock: Input/output error > +qemu-io: Could not repair dirty image 'NODE_NAME': Input/output error > +The image is left dirty and this node holds it open until the node is removed > +read failed: No medium found > +incompatible_features [0] > + > == Creating an image file with lazy_refcounts=off == > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 > wrote 512/512 bytes at offset 0 > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > index 5c18e413ec9..b25c76183a1 100755 > --- a/tests/qemu-iotests/040 > +++ b/tests/qemu-iotests/040 > @@ -951,6 +951,129 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase): > self.vm.qmp('block-job-complete', device='commit') > self.vm.event_wait('BLOCK_JOB_COMPLETED') > > +QCOW2_INCOMPAT_FEATURES_OFFSET = 72 > +QCOW2_INCOMPAT_DIRTY = 1 << 0 > + > +image_size = 4 * 1024 * 1024 > +dirty_base = os.path.join(iotests.test_dir, 'dirty-base.img') > +mid = os.path.join(iotests.test_dir, 'dirty-mid.img') > +top = os.path.join(iotests.test_dir, 'dirty-top.img') > + > + > +class TestCommitDirtyBase(iotests.QMPTestCase): > + def setUp(self) -> None: > + if iotests.imgfmt != 'qcow2': > + self.case_skip('the dirty bit is a qcow2 feature') > + iotests.qemu_img_create('-f', iotests.imgfmt, '-o', > + 'compat=1.1,lazy_refcounts=on', dirty_base, > + str(image_size)) > + # Killing the process leaves the refcounts of the written cluster stale > + iotests.qemu_io_popen('-t', 'writethrough', > + '-c', 'write -P 0x5a 0 512', > + '-c', 'sigraise 9', dirty_base).communicate() > + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', dirty_base, > + '-F', iotests.imgfmt, mid) > + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid, > + '-F', iotests.imgfmt, top) > + # The commit has to allocate for this, which is where the stale > + # refcounts hand out the cluster holding the data written above > + qemu_io('-c', 'write -P 0xb1 1M 512', mid) > + > + self.vm = iotests.VM() > + self.vm.launch() > + self.vm.cmd('blockdev-add', driver='file', filename=dirty_base, > + node_name='base-file') > + > + self.assertTrue(self.is_dirty()) > + > + def tearDown(self) -> None: > + if self.vm.is_running(): > + self.vm.shutdown() > + for image in (dirty_base, mid, top): > + os.remove(image) > + > + def add_chain(self, base_file: str) -> None: > + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file=base_file, > + node_name='base', read_only=True) > + self.vm.cmd('blockdev-add', driver='file', filename=mid, > + node_name='mid-file') > + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='mid-file', > + node_name='mid', backing='base') > + self.vm.cmd('blockdev-add', driver='file', filename=top, > + node_name='top-file') > + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='top-file', > + node_name='top', backing='mid') > + > + def check_base(self) -> None: > + result = iotests.qemu_img_check(dirty_base) > + self.assertEqual(result['check-errors'], 0) > + self.assertEqual(result.get('corruptions', 0), 0) > + # Without the repair the commit would have aliased this cluster > + qemu_io('-c', 'read -P 0x5a 0 512', '-c', 'read -P 0xb1 1M 512', > + dirty_base) > + > + def is_dirty(self) -> bool: > + with open(dirty_base, 'rb') as img: > + img.seek(QCOW2_INCOMPAT_FEATURES_OFFSET) > + features = struct.unpack('>Q', img.read(8))[0] > + return bool(features & QCOW2_INCOMPAT_DIRTY) > + > + def test_commit_repairs_base(self) -> None: > + self.add_chain('base-file') > + > + self.vm.cmd('block-commit', job_id='job0', device='top', > + top_node='mid', base_node='base') > + self.wait_until_completed(drive='job0') > + > + self.vm.shutdown() > + self.assertFalse(self.is_dirty()) > + self.check_base() > + > + def test_active_commit_repairs_base(self) -> None: > + self.add_chain('base-file') > + > + # Without top-node the whole chain commits, through > + # commit_active_start() rather than commit_start() > + self.vm.cmd('block-commit', job_id='job0', device='top', > + base_node='base') > + self.complete_and_wait(drive='job0') > + > + self.vm.shutdown() > + self.assertFalse(self.is_dirty()) > + self.check_base() > + > + def test_failed_repair_fails_the_commit(self) -> None: > + self.vm.cmd('blockdev-add', driver='blkdebug', image='base-file', > + node_name='base-blkdebug', > + inject_error=[{'event': 'none', 'iotype': 'write', > + 'errno': 5}]) > + self.add_chain('base-blkdebug') > + > + result = self.vm.qmp('block-commit', job_id='job0', device='top', > + top_node='mid', base_node='base') > + self.assert_qmp(result, 'error/class', 'GenericError') > + self.assertIn("Could not repair dirty image 'base'", > + result['error']['desc']) > + > + # The base is left in the graph, and nothing can be queried while > + # it is there > + result = self.vm.qmp('query-named-block-nodes', flat=True) > + self.assert_qmp(result, 'error/desc', 'Block device base is ejected') > + > + # It only goes away once nothing refers to it > + result = self.vm.qmp('blockdev-del', node_name='base') > + self.assert_qmp(result, 'error/desc', > + "Node 'base' is busy: node is used as backing hd of " > + "'mid'") > + > + # Marking the image corrupt needs a write of its own, which fails too > + self.assertTrue(self.is_dirty()) > + > + # Nothing can use the base any more, and that is what is reported > + result = self.vm.qmp('block-commit', job_id='job1', device='top', > + top_node='mid', base_node='base') > + self.assert_qmp(result, 'error/desc', 'Device has no medium') > + > if __name__ == '__main__': > iotests.main(supported_fmts=['qcow2', 'qed'], > supported_protocols=['file']) > diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out > index 1bb1dc5f0ea..f3cbf73a01d 100644 > --- a/tests/qemu-iotests/040.out > +++ b/tests/qemu-iotests/040.out > @@ -1,5 +1,5 @@ > -................................................................. > +.................................................................... > ---------------------------------------------------------------------- > -Ran 65 tests > +Ran 68 tests > > OK ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 2026-09-16 16:26 ` Andrey Drobyshev @ 2026-09-16 16:28 ` Denis V. Lunev 0 siblings, 0 replies; 8+ messages in thread From: Denis V. Lunev @ 2026-09-16 16:28 UTC (permalink / raw) To: Andrey Drobyshev, Denis V. Lunev, svt-core pls answer to mainstream first properly, I'll send there and here Den On 9/16/26 18:26, Andrey Drobyshev wrote: > Other commits appear to be fine, but this commit has several issues: > > - Major: a failed repair makes a readable image unreadable. > state->bs->drv = NULL at block/qcow2.c:2251 blocks reads as well as > writes. The base of a running chain, or the active layer in the > stream_start() case, answers every guest read with ENOMEDIUM from then > on, for an image whose L1/L2 metadata is intact and was serving reads a > moment earlier. Test 039's own expected output shows it: read failed: No > medium found. Only allocation has to be stopped. A per-node gate in > BDRVQcow2State that fails pwritev/pwrite_zeroes/pdiscard/truncate with > -EIO (and skips qcow2_mark_clean() in qcow2_inactivate() so the dirty > bit survives close) keeps the guest running and the job simply fails. > The current approach also makes query-named-block-nodes/query-block fail > wholesale while the node exists, and in the commit case the node cannot > be deleted until the VM is restarted. > > - Major: use-after-free through the cache-clean coroutine. With > bs->drv == NULL, bdrv_close() (block.c:5259) never calls qcow2_close(), > so cache_clean_timer_del_and_wait() never runs, and bdrv_close() then > frees bs->opaque. The coroutine at block/qcow2.c:895 is still sleeping > on s->cache_clean_timer_wake (600 s default on Linux) and wakes on freed > memory. Pre-existing for qcow2_signal_corruption(fatal), but this patch > adds a second path and is the right place to stop the coroutine before > dropping the driver. The same skipped close also leaks the L1 table, > refcount table, both caches, crypto and bitmap state. > > - Major: the check runs under bdrv_drain_all_begin(). > bdrv_reopen_queue() drains every node (block.c:4593), and bdrv_check() > polls from the main loop. A full refcount scan plus rebuild of a large > image stalls all guest I/O on every disk and the monitor for its whole > duration, as a side effect of > block-commit/block-stream/change-backing-file. The QAPI text discloses > it, but the cover letter should quantify it and argue why this beats > failing the command. > > - Medium (downstream only): the new condition ignores s->keep_dirty, > unlike qcow2_do_open() at block/qcow2.c:2025. A node opened with > __vz_keep-dirty=on, then reopened read-write, gets checked and its dirty > bit cleared, breaking that option's contract. Add !s->keep_dirty in the > hci branch. > > - Low: after the failure the node keeps BDRV_O_RDWR, BLK_PERM_WRITE on > bs->file and the write lock, so the qemu-img check -r the hint > recommends is impossible until the node is gone, which for a commit base > means VM shutdown. Say so in the hint or the docs. > > - Low: test 040 asserts exact error strings owned by other code ("Node > 'base' is busy: node is used as backing hd of 'mid'", "Block device base > is ejected"), which makes it brittle. > > On 9/16/26 6:24 PM, Denis V. Lunev wrote: >> From: Denis V. Lunev <den@openvz.org> >> >> A dirty image must be repaired before anything allocates a cluster in >> it. qcow2_do_open() does that, but only for a node that is writable >> from the start. A node opened read-only skips it, and nothing revisits >> the question once that node becomes writable, which block-commit does >> routinely: commit_active_start() and commit_start() reopen the base >> read-write for the duration of the job. >> >> With lazy refcounts the on-disk refcount block then still accounts for >> the metadata clusters only, so the allocator restarts at the front of >> the image and hands out clusters that L2 entries point at. Two guest >> offsets end up sharing one host cluster. Nothing fails, the corrupt bit >> stays clear, and a clean close clears the dirty bit, so no later open >> repairs the image either. The bit also stays set for the whole writable >> session, so a node which is merely writable says nothing. >> >> Refusing the reopen instead is simpler and keeps it atomic, but it >> leaves nowhere to go: the base belongs to a chain the VM has open, so >> the qemu-img check -r such an error would ask for cannot take the write >> lock it needs. The repair does the trick in most cases anyway. >> >> Do the repair in qcow2_reopen_commit_post(), the earliest point where >> the node is writable. An inactive node is skipped: bdrv_activate() calls >> qcow2_do_open() again through qcow2_co_invalidate_cache(). >> >> commit_post cannot reject the reopen, so a failed repair takes the >> driver away from the node instead, which is what stops writes from >> aliasing live clusters. qcow2_signal_corruption() does that as well, >> but it also sends BLOCK_IMAGE_CORRUPTED and sets the corrupt bit, >> which qcow2_do_open() honours by refusing every later read-write open. >> The image is dirty and unrepaired, not corrupt, and qemu-img check -r >> still fixes it, so neither belongs here. Return the error and skip >> the bitmaps. >> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Hanna Reitz <hreitz@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com> >> Cc: qemu-stable@nongnu.org >> --- >> block/qcow2.c | 21 +++++++ >> qapi/block-core.json | 19 ++++++ >> tests/qemu-iotests/039 | 60 ++++++++++++++++++ >> tests/qemu-iotests/039.out | 36 +++++++++++ >> tests/qemu-iotests/040 | 123 +++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/040.out | 4 +- >> 6 files changed, 261 insertions(+), 2 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index ddad39a3084..be346f0339b 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2235,8 +2235,29 @@ static void qcow2_reopen_commit(BDRVReopenState *state) >> >> static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp) >> { >> + ERRP_GUARD(); >> + BDRVQcow2State *s = state->bs->opaque; >> + >> GRAPH_RDLOCK_GUARD_MAINLOOP(); >> >> + if (!bdrv_reopen_was_writable(state) && bdrv_is_writable(state->bs) && >> + (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { >> + BdrvCheckResult result = {0}; >> + int ret; >> + >> + ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); >> + if (ret < 0 || result.check_errors || !state->bs->drv) { >> + ret = ret < 0 ? ret : -EIO; >> + /* No write may reach an image whose refcounts are unaccounted */ >> + state->bs->drv = NULL; >> + error_setg_errno(errp, -ret, "Could not repair dirty image '%s'", >> + bdrv_get_device_or_node_name(state->bs)); >> + error_append_hint(errp, "The image is left dirty and this node " >> + "holds it open until the node is removed\n"); >> + return ret; >> + } >> + } >> + >> if (state->flags & BDRV_O_RDWR) { >> Error *local_err = NULL; >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index c036a3a8858..6fb8c31210a 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1843,6 +1843,9 @@ >> # r/w -> r/o, if needed). The new backing file string is written into >> # the image file metadata, and the QEMU internal strings are updated. >> # >> +# A dirty qcow2 image is repaired during that reopen, which blocks >> +# other requests and can fail the command. >> +# >> # @image-node-name: The name of the block driver state node of the >> # image to modify. The "device" argument is used to verify >> # "image-node-name" is in the chain described by "device". >> @@ -1882,6 +1885,9 @@ >> # size to match the size of the smaller top, you can safely truncate >> # it yourself once the commit operation successfully completes. >> # >> +# The base is opened read-write. A dirty qcow2 base is repaired >> +# first, which blocks other requests and can fail the command. >> +# >> # @job-id: identifier for the newly-created block job. If omitted, >> # the device name will be used. (Since 2.7) >> # >> @@ -2919,6 +2925,9 @@ >> # On successful completion the image file is updated to drop the >> # backing file and the `BLOCK_JOB_COMPLETED` event is emitted. >> # >> +# The top image is opened read-write. A dirty qcow2 image is repaired >> +# first, which blocks other requests and can fail the command. >> +# >> # In case @device is a filter node, `block-stream` modifies the first >> # non-filter overlay node below it to point to the new backing node >> # instead of modifying @device itself. >> @@ -5070,6 +5079,16 @@ >> # transaction, so if one of them fails then the whole transaction is >> # cancelled. >> # >> +# An error is also returned when a device cannot be used once it has >> +# been reopened. Such a reopen is not undone, so an error does not >> +# always mean that nothing has changed. A node the driver gave up on >> +# serves nothing at all: it keeps its image open, makes >> +# `query-named-block-nodes` fail for as long as it is in the graph, >> +# and `blockdev-del` removes it only once nothing refers to it. >> +# >> +# Reopening a dirty qcow2 image read-write repairs it first, which >> +# blocks other requests and can fail the command. >> +# >> # The command receives a list of block devices to reopen. For each >> # one of them, the top-level @node-name option (from `BlockdevOptions`) >> # must be specified and is used to select the block device to be >> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 >> index 3d0c073d65d..3f37c36ca86 100755 >> --- a/tests/qemu-iotests/039 >> +++ b/tests/qemu-iotests/039 >> @@ -33,6 +33,7 @@ status=1 # failure is the default! >> _cleanup() >> { >> _cleanup_test_img >> + rm -f "$TEST_DIR/blkdebug.conf" >> } >> trap "_cleanup; exit \$status" 0 1 2 3 15 >> >> @@ -176,6 +177,65 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io >> # The dirty bit must not be set >> _qcow2_dump_header | grep incompatible_features >> >> +echo >> +echo "== Reopening a dirty image read/write should repair it ==" >> + >> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size >> + >> +_NO_VALGRIND \ >> +$QEMU_IO -c "write -P 0x5a 0 512" \ >> + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ >> + | _filter_qemu_io >> + >> +# The dirty bit must be set >> +_qcow2_dump_header | grep incompatible_features >> + >> +# Without the repair this write would alias the cluster at offset 0 >> +$QEMU_IO -r -c "reopen -w" \ >> + -c "write -P 0xb1 1M 512" \ >> + -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io >> + >> +_check_test_img >> + >> +echo >> +echo "== A read/write reopen must not check the image ==" >> + >> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size >> + >> +_NO_VALGRIND \ >> +$QEMU_IO -c "write -P 0x5a 0 512" \ >> + -c "reopen -o l2-cache-size=1M" \ >> + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ >> + | _filter_qemu_io >> + >> +# The dirty bit must still be set, it belongs to the running session >> +_qcow2_dump_header | grep incompatible_features >> + >> +echo >> +echo "== A failed repair must fail the reopen ==" >> + >> +_make_test_img -o "compat=1.1,lazy_refcounts=on" $size >> + >> +_NO_VALGRIND \ >> +$QEMU_IO -c "write -P 0x5a 0 512" \ >> + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \ >> + | _filter_qemu_io >> + >> +cat > "$TEST_DIR/blkdebug.conf" <<EOF >> +[inject-error] >> +event = "none" >> +iotype = "write" >> +errno = "5" >> +EOF >> + >> +# The repair cannot write, so the reopen itself must report the failure >> +$QEMU_IO -r -c "reopen -w" -c "read -P 0x5a 0 512" \ >> + "blkdebug:$TEST_DIR/blkdebug.conf:$TEST_IMG" 2>&1 \ >> + | _filter_testdir | _filter_qemu_io | _filter_generated_node_ids >> + >> +# The corrupt bit needs a write of its own, so the image is only left dirty >> +_qcow2_dump_header | grep incompatible_features >> + >> echo >> echo "== Creating an image file with lazy_refcounts=off ==" >> >> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out >> index ce8ee577216..cc6ca3ab95b 100644 >> --- a/tests/qemu-iotests/039.out >> +++ b/tests/qemu-iotests/039.out >> @@ -79,6 +79,42 @@ wrote 512/512 bytes at offset 0 >> 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> incompatible_features [] >> >> +== Reopening a dirty image read/write should repair it == >> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 >> +wrote 512/512 bytes at offset 0 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) >> +incompatible_features [0] >> +ERROR cluster 5 refcount=0 reference=1 >> +Rebuilding refcount structure >> +Repairing cluster 1 refcount=1 reference=0 >> +Repairing cluster 2 refcount=1 reference=0 >> +wrote 512/512 bytes at offset 1048576 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +read 512/512 bytes at offset 0 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +No errors were found on the image. >> + >> +== A read/write reopen must not check the image == >> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 >> +wrote 512/512 bytes at offset 0 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) >> +incompatible_features [0] >> + >> +== A failed repair must fail the reopen == >> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 >> +wrote 512/512 bytes at offset 0 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) >> +ERROR cluster 5 refcount=0 reference=1 >> +Rebuilding refcount structure >> +qemu-io: ERROR writing refblock: Input/output error >> +qemu-io: Could not repair dirty image 'NODE_NAME': Input/output error >> +The image is left dirty and this node holds it open until the node is removed >> +read failed: No medium found >> +incompatible_features [0] >> + >> == Creating an image file with lazy_refcounts=off == >> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 >> wrote 512/512 bytes at offset 0 >> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 >> index 5c18e413ec9..b25c76183a1 100755 >> --- a/tests/qemu-iotests/040 >> +++ b/tests/qemu-iotests/040 >> @@ -951,6 +951,129 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase): >> self.vm.qmp('block-job-complete', device='commit') >> self.vm.event_wait('BLOCK_JOB_COMPLETED') >> >> +QCOW2_INCOMPAT_FEATURES_OFFSET = 72 >> +QCOW2_INCOMPAT_DIRTY = 1 << 0 >> + >> +image_size = 4 * 1024 * 1024 >> +dirty_base = os.path.join(iotests.test_dir, 'dirty-base.img') >> +mid = os.path.join(iotests.test_dir, 'dirty-mid.img') >> +top = os.path.join(iotests.test_dir, 'dirty-top.img') >> + >> + >> +class TestCommitDirtyBase(iotests.QMPTestCase): >> + def setUp(self) -> None: >> + if iotests.imgfmt != 'qcow2': >> + self.case_skip('the dirty bit is a qcow2 feature') >> + iotests.qemu_img_create('-f', iotests.imgfmt, '-o', >> + 'compat=1.1,lazy_refcounts=on', dirty_base, >> + str(image_size)) >> + # Killing the process leaves the refcounts of the written cluster stale >> + iotests.qemu_io_popen('-t', 'writethrough', >> + '-c', 'write -P 0x5a 0 512', >> + '-c', 'sigraise 9', dirty_base).communicate() >> + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', dirty_base, >> + '-F', iotests.imgfmt, mid) >> + iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid, >> + '-F', iotests.imgfmt, top) >> + # The commit has to allocate for this, which is where the stale >> + # refcounts hand out the cluster holding the data written above >> + qemu_io('-c', 'write -P 0xb1 1M 512', mid) >> + >> + self.vm = iotests.VM() >> + self.vm.launch() >> + self.vm.cmd('blockdev-add', driver='file', filename=dirty_base, >> + node_name='base-file') >> + >> + self.assertTrue(self.is_dirty()) >> + >> + def tearDown(self) -> None: >> + if self.vm.is_running(): >> + self.vm.shutdown() >> + for image in (dirty_base, mid, top): >> + os.remove(image) >> + >> + def add_chain(self, base_file: str) -> None: >> + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file=base_file, >> + node_name='base', read_only=True) >> + self.vm.cmd('blockdev-add', driver='file', filename=mid, >> + node_name='mid-file') >> + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='mid-file', >> + node_name='mid', backing='base') >> + self.vm.cmd('blockdev-add', driver='file', filename=top, >> + node_name='top-file') >> + self.vm.cmd('blockdev-add', driver=iotests.imgfmt, file='top-file', >> + node_name='top', backing='mid') >> + >> + def check_base(self) -> None: >> + result = iotests.qemu_img_check(dirty_base) >> + self.assertEqual(result['check-errors'], 0) >> + self.assertEqual(result.get('corruptions', 0), 0) >> + # Without the repair the commit would have aliased this cluster >> + qemu_io('-c', 'read -P 0x5a 0 512', '-c', 'read -P 0xb1 1M 512', >> + dirty_base) >> + >> + def is_dirty(self) -> bool: >> + with open(dirty_base, 'rb') as img: >> + img.seek(QCOW2_INCOMPAT_FEATURES_OFFSET) >> + features = struct.unpack('>Q', img.read(8))[0] >> + return bool(features & QCOW2_INCOMPAT_DIRTY) >> + >> + def test_commit_repairs_base(self) -> None: >> + self.add_chain('base-file') >> + >> + self.vm.cmd('block-commit', job_id='job0', device='top', >> + top_node='mid', base_node='base') >> + self.wait_until_completed(drive='job0') >> + >> + self.vm.shutdown() >> + self.assertFalse(self.is_dirty()) >> + self.check_base() >> + >> + def test_active_commit_repairs_base(self) -> None: >> + self.add_chain('base-file') >> + >> + # Without top-node the whole chain commits, through >> + # commit_active_start() rather than commit_start() >> + self.vm.cmd('block-commit', job_id='job0', device='top', >> + base_node='base') >> + self.complete_and_wait(drive='job0') >> + >> + self.vm.shutdown() >> + self.assertFalse(self.is_dirty()) >> + self.check_base() >> + >> + def test_failed_repair_fails_the_commit(self) -> None: >> + self.vm.cmd('blockdev-add', driver='blkdebug', image='base-file', >> + node_name='base-blkdebug', >> + inject_error=[{'event': 'none', 'iotype': 'write', >> + 'errno': 5}]) >> + self.add_chain('base-blkdebug') >> + >> + result = self.vm.qmp('block-commit', job_id='job0', device='top', >> + top_node='mid', base_node='base') >> + self.assert_qmp(result, 'error/class', 'GenericError') >> + self.assertIn("Could not repair dirty image 'base'", >> + result['error']['desc']) >> + >> + # The base is left in the graph, and nothing can be queried while >> + # it is there >> + result = self.vm.qmp('query-named-block-nodes', flat=True) >> + self.assert_qmp(result, 'error/desc', 'Block device base is ejected') >> + >> + # It only goes away once nothing refers to it >> + result = self.vm.qmp('blockdev-del', node_name='base') >> + self.assert_qmp(result, 'error/desc', >> + "Node 'base' is busy: node is used as backing hd of " >> + "'mid'") >> + >> + # Marking the image corrupt needs a write of its own, which fails too >> + self.assertTrue(self.is_dirty()) >> + >> + # Nothing can use the base any more, and that is what is reported >> + result = self.vm.qmp('block-commit', job_id='job1', device='top', >> + top_node='mid', base_node='base') >> + self.assert_qmp(result, 'error/desc', 'Device has no medium') >> + >> if __name__ == '__main__': >> iotests.main(supported_fmts=['qcow2', 'qed'], >> supported_protocols=['file']) >> diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out >> index 1bb1dc5f0ea..f3cbf73a01d 100644 >> --- a/tests/qemu-iotests/040.out >> +++ b/tests/qemu-iotests/040.out >> @@ -1,5 +1,5 @@ >> -................................................................. >> +.................................................................... >> ---------------------------------------------------------------------- >> -Ran 65 tests >> +Ran 68 tests >> >> OK ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-09-16 16:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-16 15:24 [PATCH hci-8.0 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 1/5] qcow2: do not clear the dirty bit when reopening a read-only node #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 2/5] block: reject a reopen of an unusable node instead of crashing #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 3/5] block: let bdrv_reopen_commit_post() report a failure #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 4/5] block: remember the flags a reopen starts from #VSTOR-140073 Denis V. Lunev 2026-09-16 15:24 ` [PATCH hci-8.0 5/5] qcow2: repair a dirty image when it becomes writable #VSTOR-140073 Denis V. Lunev 2026-09-16 16:26 ` Andrey Drobyshev 2026-09-16 16:28 ` Denis V. Lunev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox