* [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable
@ 2026-08-24 13:37 Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, Eric Blake, Markus Armbruster, qemu-stable
Today I have faced real data corrupt from our customer with the
situation very close to the one addressed in the patch
"qcow2: do not try to clear the dirty bit on a read-only node" and
that is interesting. I was really unsure that design is correct
but with today case I can say that correct thing was done.
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. Afterwards qemu-img check reports
ERROR cluster N refcount=1 reference=2
ERROR cluster N refcount=0 reference=1
ERROR OFLAG_COPIED data cluster: l2_entry=<host>|COPIED refcount=0
and 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.
Two properties of the damage are worth recording, because they are what
told us this was not a race:
- aliasing is exactly two-way, never three or more, which is a single
monotonic sweep of the allocator rather than a window hit repeatedly;
- it stops dead at the host cluster that was the image end at the
moment the volume was made writable. Everything allocated after that
point is intact.
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().
v1:
https://lore.kernel.org/qemu-devel/20260731220039.1765584-1-den@openvz.org/
v2:
https://lore.kernel.org/qemu-devel/20260811173857.396571-1-den@openvz.org/
v3:
https://lore.kernel.org/qemu-devel/20260819120558.3870413-1-den@openvz.org/
Notes for review
----------------
- The repair still 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. Rejecting the reopen instead would be cheap, but block-commit
depends on it succeeding, so repairing in place is the only option.
- .bdrv_reopen_commit_post() runs after the transaction is committed
and cannot roll anything back, so a negative return value reports an
unusable node rather than rejecting the reopen. That is unusual, and
it is the only channel available: there is no failable hook once the
node holds BLK_PERM_WRITE.
Changes in v4
-------------
- patch 5: block-stream and change-backing-file document the repair as
well, they reopen read-write too. (Andrey)
- patch 5: the message says why a failed repair does not mark the image
corrupt. (Andrey)
- patch 5: each of those notes is one or two lines now.
- Cc: qemu-stable restored, QAPI maintainers added to patch 5.
Changes in v3
-------------
- patch 1: the same bug aborts QEMU, not only fails a reopen, when the
file node below is writable; the message says so and iotests 039
covers the read-write to read-only direction as well. (Andrey)
- patch 2: new, a reopen of a node whose driver is gone segfaults in
bdrv_reopen_queue_child() and asserts in bdrv_reopen_prepare(), which
is the crash a failed repair would reach through commit_clean();
bdrv_reopen_prepare() reports it instead, iotests 060 covers it.
(Andrey)
- patch 3: bdrv_reopen_multiple() documents that a failure means either
nothing changed or the reopen went through and left an unusable tree;
no caller changes. (Andrey)
- patch 3: an implementation which fails must set an error, asserted,
and the loop skips a node whose driver is already gone.
- patch 4: the pre-reopen flags are recorded as int old_flags rather
than a bool, with a bdrv_reopen_was_writable() accessor for drivers.
(Andrey)
- patch 5: the failure names the node and keeps the errno of the check.
(Andrey)
- patch 5: a failed repair drops the driver instead of calling
qcow2_signal_corruption(), which would write the corrupt bit into the
header for what may be a transient ENOSPC.
- patch 5: blockdev-reopen and block-commit document the failure and the
state it leaves. (Andrey)
- patch 5: iotests 040 covers a commit onto a dirty base through both
commit_start() and commit_active_start(), and the failed repair.
(Andrey)
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
Denis V. Lunev (5):
qcow2: do not clear the dirty bit when reopening a read-only node
block: reject a reopen of an unusable node instead of crashing
block: let bdrv_reopen_commit_post() report a failure
block: remember the flags a reopen starts from
qcow2: repair a dirty image when it becomes writable
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 | 122 +++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
tests/qemu-iotests/060 | 30 ++++++++
tests/qemu-iotests/060.out | 13 ++++
11 files changed, 439 insertions(+), 15 deletions(-)
base-commit: fa19879df1658f96ac07365fca8835b7decd6995
--
2.53.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/5] qcow2: do not clear the dirty bit when reopening a read-only node
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
@ 2026-08-24 13:37 ` Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 2/5] block: reject a reopen of an unusable node instead of crashing Denis V. Lunev
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, qemu-stable
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 7292dd036c..1543255eba 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2102,9 +2102,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 94a8bfe754..3d0c073d65 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 c66361128f..ce8ee57721 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 related [flat|nested] 15+ messages in thread
* [PATCH v4 2/5] block: reject a reopen of an unusable node instead of crashing
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
@ 2026-08-24 13:37 ` Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 3/5] block: let bdrv_reopen_commit_post() report a failure Denis V. Lunev
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, qemu-stable
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 f0a6042e61..e39f15816a 100644
--- a/block.c
+++ b/block.c
@@ -4486,6 +4486,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;
@@ -4881,9 +4886,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 5cd21a6f68..ce49fc34ec 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 a37bf446e9..ad1912a43b 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 related [flat|nested] 15+ messages in thread
* [PATCH v4 3/5] block: let bdrv_reopen_commit_post() report a failure
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 2/5] block: reject a reopen of an unusable node instead of crashing Denis V. Lunev
@ 2026-08-24 13:37 ` Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 4/5] block: remember the flags a reopen starts from Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
4 siblings, 0 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, qemu-stable
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 e39f15816a..b29202c8d5 100644
--- a/block.c
+++ b/block.c
@@ -4582,6 +4582,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().
*
@@ -4658,15 +4662,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 1543255eba..553a94d003 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2145,7 +2145,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();
@@ -2163,6 +2163,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 147c08155f..035e54d434 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -239,8 +239,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 related [flat|nested] 15+ messages in thread
* [PATCH v4 4/5] block: remember the flags a reopen starts from
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
` (2 preceding siblings ...)
2026-08-24 13:37 ` [PATCH v4 3/5] block: let bdrv_reopen_commit_post() report a failure Denis V. Lunev
@ 2026-08-24 13:37 ` Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
4 siblings, 0 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, qemu-stable
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 b29202c8d5..6280a13610 100644
--- a/block.c
+++ b/block.c
@@ -2193,14 +2193,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));
}
/*
@@ -2214,6 +2218,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();
@@ -4473,6 +4483,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 895ea17541..eb2dd8aff1 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -358,6 +358,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;
BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 035e54d434..4ea1a78494 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1346,6 +1346,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 related [flat|nested] 15+ messages in thread
* [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
` (3 preceding siblings ...)
2026-08-24 13:37 ` [PATCH v4 4/5] block: remember the flags a reopen starts from Denis V. Lunev
@ 2026-08-24 13:37 ` Denis V. Lunev
2026-08-25 9:37 ` Markus Armbruster
4 siblings, 1 reply; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-24 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz, Eric Blake, Markus Armbruster, qemu-stable
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 | 122 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/040.out | 4 +-
6 files changed, 260 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 553a94d003..e91523699f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2147,8 +2147,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 199efc1e00..940249a5e5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1852,6 +1852,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".
@@ -1891,6 +1894,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)
#
@@ -2902,6 +2908,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.
@@ -4989,6 +4998,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
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 3d0c073d65..3f37c36ca8 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 ce8ee57721..cc6ca3ab95 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 5c18e413ec..452c87f9cd 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -951,6 +951,128 @@ 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.assertEqual(self.incompatible_features(), QCOW2_INCOMPAT_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 incompatible_features(self) -> int:
+ with open(dirty_base, 'rb') as img:
+ img.seek(QCOW2_INCOMPAT_FEATURES_OFFSET)
+ return struct.unpack('>Q', img.read(8))[0]
+
+ 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.assertEqual(self.incompatible_features(), 0)
+ 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.assertEqual(self.incompatible_features(), 0)
+ 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.assertEqual(self.incompatible_features(), QCOW2_INCOMPAT_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 1bb1dc5f0e..f3cbf73a01 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 related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-24 13:37 ` [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
@ 2026-08-25 9:37 ` Markus Armbruster
2026-08-26 13:49 ` Denis V. Lunev
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2026-08-25 9:37 UTC (permalink / raw)
To: Denis V. Lunev
Cc: qemu-devel, qemu-block, Andrey Drobyshev, Kevin Wolf, Hanna Reitz,
Eric Blake, qemu-stable
"Denis V. Lunev" <den@openvz.org> writes:
> 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
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 199efc1e00..940249a5e5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1852,6 +1852,9 @@
##
# @change-backing-file:
#
# Change the backing file in the image file metadata. This does not
# cause QEMU to reopen the image file to reparse the backing filename
# (it may, however, perform a reopen to change permissions from r/o ->
> # 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.
Pardon my ignorance: what makes a qcow2 image dirty?
Can you give me an idea of what other requests could be blocked?
Double-checking: "that reopen" is the one to change permissions,
i.e. the parenthesis above. Correct?
> +#
> # @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".
> @@ -1891,6 +1894,9 @@
##
# @block-commit:
#
# Live commit of data from overlay image nodes into backing nodes -
# i.e., writes data between 'top' and 'base' into 'base'.
#
# If top == base, that is an error. If top has no overlays on top of
# it, or if it is in use by a writer, the job will not be completed by
# itself. The user needs to complete the job with the `job-complete`
# command after getting the ready event. (Since 2.0)
#
# If the base image is smaller than top, then the base image will be
# resized to be the same size as top. If top is smaller than the base
# image, the base will not be truncated. If you want the base image
> # 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)
> #
> @@ -2902,6 +2908,9 @@
##
# @block-stream:
#
# Copy data from a backing file into a block device.
[...]
> # 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.
> @@ -4989,6 +4998,16 @@
##
# @blockdev-reopen:
#
# Reopens one or more block devices using the given set of options.
# Any option not specified will be reset to its default value
# regardless of its previous status. If an option cannot be changed
# or a particular driver does not support reopening then the command
# will return an error. All devices in the list are reopened in one
> # 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.
Could that be a problem?
> +# 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
These documentation updates suggest the patch affects commands
change-backing-file, block-commit, block-stream, and blockdev-reopen.
Is that correct?
Would it make sense to list them in the commit message?
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-25 9:37 ` Markus Armbruster
@ 2026-08-26 13:49 ` Denis V. Lunev
2026-08-26 15:24 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-26 13:49 UTC (permalink / raw)
To: Markus Armbruster, Denis V. Lunev
Cc: qemu-devel, qemu-block, Andrey Drobyshev, Kevin Wolf, Hanna Reitz,
Eric Blake, qemu-stable
On 8/25/26 11:37, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> 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
> [...]
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 199efc1e00..940249a5e5 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1852,6 +1852,9 @@
> ##
> # @change-backing-file:
> #
> # Change the backing file in the image file metadata. This does not
> # cause QEMU to reopen the image file to reparse the backing filename
> # (it may, however, perform a reopen to change permissions from r/o ->
>> # 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.
> Pardon my ignorance: what makes a qcow2 image dirty?
usual obvious reasons are SIGKILL to qemu process (f.e. from OOM)
or node crash.
> Can you give me an idea of what other requests could be blocked?
Before the patch reopen was smooth - we have just opened the
file again. After this patch in a very unlikely corner case
potentially lengthy procedure has been added - full image
consistency check. Guest IO is stuck until the check will be
completed.
The case is unfortunately real for production.
> Double-checking: "that reopen" is the one to change permissions,
> i.e. the parenthesis above. Correct?
yes
>> +#
>> # @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".
>> @@ -1891,6 +1894,9 @@
> ##
> # @block-commit:
> #
> # Live commit of data from overlay image nodes into backing nodes -
> # i.e., writes data between 'top' and 'base' into 'base'.
> #
> # If top == base, that is an error. If top has no overlays on top of
> # it, or if it is in use by a writer, the job will not be completed by
> # itself. The user needs to complete the job with the `job-complete`
> # command after getting the ready event. (Since 2.0)
> #
> # If the base image is smaller than top, then the base image will be
> # resized to be the same size as top. If top is smaller than the base
> # image, the base will not be truncated. If you want the base image
>> # 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)
>> #
>> @@ -2902,6 +2908,9 @@
> ##
> # @block-stream:
> #
> # Copy data from a backing file into a block device.
>
> [...]
>
>> # 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.
>> @@ -4989,6 +4998,16 @@
> ##
> # @blockdev-reopen:
> #
> # Reopens one or more block devices using the given set of options.
> # Any option not specified will be reset to its default value
> # regardless of its previous status. If an option cannot be changed
> # or a particular driver does not support reopening then the command
> # will return an error. All devices in the list are reopened in one
>> # 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.
> Could that be a problem?
Error reply as itself is not a problem. The situation as a whole
is a real pain in the ass.
The problem is that such images exists in production and there
is not way to handle this without downtime.
Under this patch we are trying to fix things hard and this is
correct thing to do - on RW image we are doing exactly the
same thing. If automation is unable to fix - the guest denies
starting.
Here we report an error and render guest as unusable. This
case is expected to be extremely rare but technically possible.
This is a problem.
>> +# 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
> These documentation updates suggest the patch affects commands
> change-backing-file, block-commit, block-stream, and blockdev-reopen.
> Is that correct?
>
> Would it make sense to list them in the commit message?
That is simple thing. Will add :-)
Thank you,
Den
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-26 13:49 ` Denis V. Lunev
@ 2026-08-26 15:24 ` Markus Armbruster
2026-08-26 16:37 ` Denis V. Lunev
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2026-08-26 15:24 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
"Denis V. Lunev" <den@virtuozzo.com> writes:
> On 8/25/26 11:37, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>>
>>> 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
>> [...]
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 199efc1e00..940249a5e5 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1852,6 +1852,9 @@
>> ##
>> # @change-backing-file:
>> #
>> # Change the backing file in the image file metadata. This does not
>> # cause QEMU to reopen the image file to reparse the backing filename
>> # (it may, however, perform a reopen to change permissions from r/o ->
>>> # 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.
>>
>> Pardon my ignorance: what makes a qcow2 image dirty?
> usual obvious reasons are SIGKILL to qemu process (f.e. from OOM)
> or node crash.
So, you have to do some cleaning work before you can use it again, just
like a dirty filesystem. Correct?
Back to change-backing-file. It operates on an open image. Cleaning
happens when that image is read-only and dirty. Possible because you
can open dirty images read-only, and that doesn't clean them. Correct?
>> Can you give me an idea of what other requests could be blocked?
> Before the patch reopen was smooth - we have just opened the
> file again. After this patch in a very unlikely corner case
> potentially lengthy procedure has been added - full image
> consistency check. Guest IO is stuck until the check will be
> completed.
>
> The case is unfortunately real for production.
>
>> Double-checking: "that reopen" is the one to change permissions,
>> i.e. the parenthesis above. Correct?
> yes
>
>>> +#
>>> # @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".
>>> @@ -1891,6 +1894,9 @@
>> ##
>> # @block-commit:
>> #
>> # Live commit of data from overlay image nodes into backing nodes -
>> # i.e., writes data between 'top' and 'base' into 'base'.
>> #
>> # If top == base, that is an error. If top has no overlays on top of
>> # it, or if it is in use by a writer, the job will not be completed by
>> # itself. The user needs to complete the job with the `job-complete`
>> # command after getting the ready event. (Since 2.0)
>> #
>> # If the base image is smaller than top, then the base image will be
>> # resized to be the same size as top. If top is smaller than the base
>> # image, the base will not be truncated. If you want the base image
>>> # 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
Reopend, I presume?
>>> +# 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)
>>> #
Like change-backing-file, block-commit operates on open images. It
copies down into a base image. If the base image is read-only, it is
reopened, and cleaning happens when it's dirty. Correct?
Can it happen in any other way?
>>> @@ -2902,6 +2908,9 @@
>> ##
>> # @block-stream:
>> #
>> # Copy data from a backing file into a block device.
>>
>> [...]
Whereas block-commit copies down into a base image, block-stream copies
up from a base image. If the image copied to is read-only, it is
reopened, and cleaning happens when it's dirty. Correct?
Can it happen in any other way?
>>> # 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.
>>> @@ -4989,6 +4998,16 @@
>> ##
>> # @blockdev-reopen:
>> #
>> # Reopens one or more block devices using the given set of options.
>> # Any option not specified will be reset to its default value
>> # regardless of its previous status. If an option cannot be changed
>> # or a particular driver does not support reopening then the command
>> # will return an error. All devices in the list are reopened in one
>>> # transaction, so if one of them fails then the whole transaction is
>>> # cancelled.
blockdev-reopen also operates on open images. Cleaning happens when
reopening a dirty read-only image read/write. Correct?
Can it happen in any other way?
>>> #
>>> +# 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.
>>
>> Could that be a problem?
> Error reply as itself is not a problem. The situation as a whole
> is a real pain in the ass.
>
> The problem is that such images exists in production and there
> is not way to handle this without downtime.
>
> Under this patch we are trying to fix things hard and this is
> correct thing to do - on RW image we are doing exactly the
> same thing. If automation is unable to fix - the guest denies
> starting.
>
> Here we report an error and render guest as unusable. This
> case is expected to be extremely rare but technically possible.
>
> This is a problem.
I'll come back to this as soon as I understand when exactly cleaning may
happen.
>>> +# 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
>>
>> These documentation updates suggest the patch affects commands
>> change-backing-file, block-commit, block-stream, and blockdev-reopen.
>> Is that correct?
>>
>> Would it make sense to list them in the commit message?
> That is simple thing. Will add :-)
>
> Thank you,
> Den
Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-26 15:24 ` Markus Armbruster
@ 2026-08-26 16:37 ` Denis V. Lunev
2026-08-27 9:15 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-26 16:37 UTC (permalink / raw)
To: Markus Armbruster
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
On 8/26/26 17:24, Markus Armbruster wrote:
> "Denis V. Lunev" <den@virtuozzo.com> writes:
>
>> On 8/25/26 11:37, Markus Armbruster wrote:
>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>
>>>> 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
>>> [...]
>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 199efc1e00..940249a5e5 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1852,6 +1852,9 @@
>>> ##
>>> # @change-backing-file:
>>> #
>>> # Change the backing file in the image file metadata. This does not
>>> # cause QEMU to reopen the image file to reparse the backing filename
>>> # (it may, however, perform a reopen to change permissions from r/o ->
>>>> # 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.
>>> Pardon my ignorance: what makes a qcow2 image dirty?
>> usual obvious reasons are SIGKILL to qemu process (f.e. from OOM)
>> or node crash.
> So, you have to do some cleaning work before you can use it again, just
> like a dirty filesystem. Correct?
Correct. QEMU allows right now to open dirty images in read-only
mode and it is OK to be used until we switch to RW. In this
case real write to metadata corrupts image.
> Back to change-backing-file. It operates on an open image. Cleaning
> happens when that image is read-only and dirty. Possible because you
> can open dirty images read-only, and that doesn't clean them. Correct?
Correct.
>>> Can you give me an idea of what other requests could be blocked?
>> Before the patch reopen was smooth - we have just opened the
>> file again. After this patch in a very unlikely corner case
>> potentially lengthy procedure has been added - full image
>> consistency check. Guest IO is stuck until the check will be
>> completed.
>>
>> The case is unfortunately real for production.
>>
>>> Double-checking: "that reopen" is the one to change permissions,
>>> i.e. the parenthesis above. Correct?
>> yes
>>
>>>> +#
>>>> # @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".
>>>> @@ -1891,6 +1894,9 @@
>>> ##
>>> # @block-commit:
>>> #
>>> # Live commit of data from overlay image nodes into backing nodes -
>>> # i.e., writes data between 'top' and 'base' into 'base'.
>>> #
>>> # If top == base, that is an error. If top has no overlays on top of
>>> # it, or if it is in use by a writer, the job will not be completed by
>>> # itself. The user needs to complete the job with the `job-complete`
>>> # command after getting the ready event. (Since 2.0)
>>> #
>>> # If the base image is smaller than top, then the base image will be
>>> # resized to be the same size as top. If top is smaller than the base
>>> # image, the base will not be truncated. If you want the base image
>>>> # 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
> Reopend, I presume?
The meaning here is repaired. The meaning here is the following:
"The base is reopened read-write and if it is dirty it should be
repaired before any single write is made. Guest stalls until
repair is complete."
>>>> +# 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)
>>>> #
> Like change-backing-file, block-commit operates on open images. It
> copies down into a base image. If the base image is read-only, it is
> reopened, and cleaning happens when it's dirty. Correct?
Correct.
> Can it happen in any other way?
No at the best knowledge from me and Andrey.
>>>> @@ -2902,6 +2908,9 @@
>>> ##
>>> # @block-stream:
>>> #
>>> # Copy data from a backing file into a block device.
>>>
>>> [...]
> Whereas block-commit copies down into a base image, block-stream copies
> up from a base image. If the image copied to is read-only, it is
> reopened, and cleaning happens when it's dirty. Correct?
Correct.
> Can it happen in any other way?
No at the best knowledge from me and Andrey.
>>>> # 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.
>>>> @@ -4989,6 +4998,16 @@
>>> ##
>>> # @blockdev-reopen:
>>> #
>>> # Reopens one or more block devices using the given set of options.
>>> # Any option not specified will be reset to its default value
>>> # regardless of its previous status. If an option cannot be changed
>>> # or a particular driver does not support reopening then the command
>>> # will return an error. All devices in the list are reopened in one
>>>> # transaction, so if one of them fails then the whole transaction is
>>>> # cancelled.
> blockdev-reopen also operates on open images. Cleaning happens when
> reopening a dirty read-only image read/write. Correct?
Correct.
> Can it happen in any other way?
No.
>>>> #
>>>> +# 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.
>>> Could that be a problem?
>> Error reply as itself is not a problem. The situation as a whole
>> is a real pain in the ass.
>>
>> The problem is that such images exists in production and there
>> is not way to handle this without downtime.
>>
>> Under this patch we are trying to fix things hard and this is
>> correct thing to do - on RW image we are doing exactly the
>> same thing. If automation is unable to fix - the guest denies
>> starting.
>>
>> Here we report an error and render guest as unusable. This
>> case is expected to be extremely rare but technically possible.
>>
>> This is a problem.
> I'll come back to this as soon as I understand when exactly cleaning may
> happen.
Right. Thanks.
You are asking very good questions which are quite important
and interesting :-) Simple "deny" policy is very bad from
operations point of view.
>>>> +# 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
>>> These documentation updates suggest the patch affects commands
>>> change-backing-file, block-commit, block-stream, and blockdev-reopen.
>>> Is that correct?
>>>
>>> Would it make sense to list them in the commit message?
>> That is simple thing. Will add :-)
>>
>> Thank you,
>> Den
> Thanks!
>
Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-26 16:37 ` Denis V. Lunev
@ 2026-08-27 9:15 ` Markus Armbruster
2026-08-27 16:06 ` Denis V. Lunev
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2026-08-27 9:15 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
"Denis V. Lunev" <den@virtuozzo.com> writes:
> On 8/26/26 17:24, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@virtuozzo.com> writes:
>>
>>> On 8/25/26 11:37, Markus Armbruster wrote:
>>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>>
>>>>> 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
>>>> [...]
>>>>
>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>> index 199efc1e00..940249a5e5 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -1852,6 +1852,9 @@
>>>> ##
>>>> # @change-backing-file:
>>>> #
>>>> # Change the backing file in the image file metadata. This does not
>>>> # cause QEMU to reopen the image file to reparse the backing filename
>>>> # (it may, however, perform a reopen to change permissions from r/o ->
>>>>> # 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.
>>>>
>>>> Pardon my ignorance: what makes a qcow2 image dirty?
>>> usual obvious reasons are SIGKILL to qemu process (f.e. from OOM)
>>> or node crash.
>>
>> So, you have to do some cleaning work before you can use it again, just
>> like a dirty filesystem. Correct?
> Correct. QEMU allows right now to open dirty images in read-only
> mode and it is OK to be used until we switch to RW. In this
> case real write to metadata corrupts image.
Feels... adventurous?
>> Back to change-backing-file. It operates on an open image. Cleaning
>> happens when that image is read-only and dirty. Possible because you
>> can open dirty images read-only, and that doesn't clean them. Correct?
>
> Correct.
>
>>>> Can you give me an idea of what other requests could be blocked?
>>> Before the patch reopen was smooth - we have just opened the
>>> file again. After this patch in a very unlikely corner case
>>> potentially lengthy procedure has been added - full image
>>> consistency check. Guest IO is stuck until the check will be
>>> completed.
>>>
>>> The case is unfortunately real for production.
>>>
>>>> Double-checking: "that reopen" is the one to change permissions,
>>>> i.e. the parenthesis above. Correct?
>>> yes
>>>
>>>>> +#
>>>>> # @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".
>>>>> @@ -1891,6 +1894,9 @@
>>>> ##
>>>> # @block-commit:
>>>> #
>>>> # Live commit of data from overlay image nodes into backing nodes -
>>>> # i.e., writes data between 'top' and 'base' into 'base'.
>>>> #
>>>> # If top == base, that is an error. If top has no overlays on top of
>>>> # it, or if it is in use by a writer, the job will not be completed by
>>>> # itself. The user needs to complete the job with the `job-complete`
>>>> # command after getting the ready event. (Since 2.0)
>>>> #
>>>> # If the base image is smaller than top, then the base image will be
>>>> # resized to be the same size as top. If top is smaller than the base
>>>> # image, the base will not be truncated. If you want the base image
>>>>> # 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
>>
>> Reopend, I presume?
> The meaning here is repaired. The meaning here is the following:
> "The base is reopened read-write and if it is dirty it should be
> repaired before any single write is made. Guest stalls until
> repair is complete."
If it is dirty, it *will* (not should) be repaired before commit can
start to write. Correct?
Is the base reopened read-only after the commit completed?
>>>>> +# 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)
>>>>> #
>>
>> Like change-backing-file, block-commit operates on open images. It
>> copies down into a base image. If the base image is read-only, it is
>> reopened, and cleaning happens when it's dirty. Correct?
> Correct.
>
>> Can it happen in any other way?
> No at the best knowledge from me and Andrey.
Got it.
>>>>> @@ -2902,6 +2908,9 @@
>>>> ##
>>>> # @block-stream:
>>>> #
>>>> # Copy data from a backing file into a block device.
>>>>
>>>> [...]
>>
>> Whereas block-commit copies down into a base image, block-stream copies
>> up from a base image. If the image copied to is read-only, it is
>> reopened, and cleaning happens when it's dirty. Correct?
> Correct.
Is the reopened back to read-only afterwards?
>> Can it happen in any other way?
> No at the best knowledge from me and Andrey.
Got it.
>>>>> # 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.
>>>>> @@ -4989,6 +4998,16 @@
>>>> ##
>>>> # @blockdev-reopen:
>>>> #
>>>> # Reopens one or more block devices using the given set of options.
>>>> # Any option not specified will be reset to its default value
>>>> # regardless of its previous status. If an option cannot be changed
>>>> # or a particular driver does not support reopening then the command
>>>> # will return an error. All devices in the list are reopened in one
>>>>> # transaction, so if one of them fails then the whole transaction is
>>>>> # cancelled.
>>
>> blockdev-reopen also operates on open images. Cleaning happens when
>> reopening a dirty read-only image read/write. Correct?
> Correct.
>
>> Can it happen in any other way?
> No.
Got it.
>>>>> #
>>>>> +# 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.
>>>>
>>>> Could that be a problem?
>>> Error reply as itself is not a problem. The situation as a whole
>>> is a real pain in the ass.
Ideally, a command does not change user-visible state when it fails.
blockdev-reopen is designed to be a single transaction: success means
all the images were reopened as directed, failure means none of the
images were reopened. "So an error does not always mean that nothing
has changed" indicates we're not actually implementing this design.
Could that be a problem?
>>> The problem is that such images exists in production and there
>>> is not way to handle this without downtime.
Yes.
>>> Under this patch we are trying to fix things hard and this is
>>> correct thing to do - on RW image we are doing exactly the
>>> same thing. If automation is unable to fix - the guest denies
>>> starting.
We must not (re)open a dirty image read/write without cleaning it,
because writing to risks corruption, i.e. data loss.
When we open a dirty image read/write, we can clean it without
inconveniencing the guest, because the guest cannot access it until
after open completes and we connect the newly open image. Correct?
When we reopen a read/only dirty image read/write, cleaning it *can*
affect the guest, as discussed above.
As far as I can tell, all the trouble discussed above ultimately comes
from letting the guest work with read-only dirty images. Why is that
useful?
What are the use cases for opening dirty images read-only?
>>> Here we report an error and render guest as unusable. This
>>> case is expected to be extremely rare but technically possible.
>>>
>>> This is a problem.
>>
>> I'll come back to this as soon as I understand when exactly cleaning may
>> happen.
> Right. Thanks.
>
> You are asking very good questions which are quite important
> and interesting :-) Simple "deny" policy is very bad from
> operations point of view.
I try!
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-27 9:15 ` Markus Armbruster
@ 2026-08-27 16:06 ` Denis V. Lunev
2026-08-31 12:31 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-27 16:06 UTC (permalink / raw)
To: Markus Armbruster
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
On 8/27/26 11:15, Markus Armbruster wrote:
> "Denis V. Lunev" <den@virtuozzo.com> writes:
>
>> On 8/26/26 17:24, Markus Armbruster wrote:
>>> "Denis V. Lunev" <den@virtuozzo.com> writes:
>>>
>>>> On 8/25/26 11:37, Markus Armbruster wrote:
>>>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>>>
>>>>>> 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
>>>>> [...]
>>>>>
>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>>> index 199efc1e00..940249a5e5 100644
>>>>>> --- a/qapi/block-core.json
>>>>>> +++ b/qapi/block-core.json
>>>>>> @@ -1852,6 +1852,9 @@
>>>>> ##
>>>>> # @change-backing-file:
>>>>> #
>>>>> # Change the backing file in the image file metadata. This does not
>>>>> # cause QEMU to reopen the image file to reparse the backing filename
>>>>> # (it may, however, perform a reopen to change permissions from r/o ->
>>>>>> # 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.
>>>>> Pardon my ignorance: what makes a qcow2 image dirty?
>>>> usual obvious reasons are SIGKILL to qemu process (f.e. from OOM)
>>>> or node crash.
>>> So, you have to do some cleaning work before you can use it again, just
>>> like a dirty filesystem. Correct?
>> Correct. QEMU allows right now to open dirty images in read-only
>> mode and it is OK to be used until we switch to RW. In this
>> case real write to metadata corrupts image.
> Feels... adventurous?
>
>>> Back to change-backing-file. It operates on an open image. Cleaning
>>> happens when that image is read-only and dirty. Possible because you
>>> can open dirty images read-only, and that doesn't clean them. Correct?
>> Correct.
>>
>>>>> Can you give me an idea of what other requests could be blocked?
>>>> Before the patch reopen was smooth - we have just opened the
>>>> file again. After this patch in a very unlikely corner case
>>>> potentially lengthy procedure has been added - full image
>>>> consistency check. Guest IO is stuck until the check will be
>>>> completed.
>>>>
>>>> The case is unfortunately real for production.
>>>>
>>>>> Double-checking: "that reopen" is the one to change permissions,
>>>>> i.e. the parenthesis above. Correct?
>>>> yes
>>>>
>>>>>> +#
>>>>>> # @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".
>>>>>> @@ -1891,6 +1894,9 @@
>>>>> ##
>>>>> # @block-commit:
>>>>> #
>>>>> # Live commit of data from overlay image nodes into backing nodes -
>>>>> # i.e., writes data between 'top' and 'base' into 'base'.
>>>>> #
>>>>> # If top == base, that is an error. If top has no overlays on top of
>>>>> # it, or if it is in use by a writer, the job will not be completed by
>>>>> # itself. The user needs to complete the job with the `job-complete`
>>>>> # command after getting the ready event. (Since 2.0)
>>>>> #
>>>>> # If the base image is smaller than top, then the base image will be
>>>>> # resized to be the same size as top. If top is smaller than the base
>>>>> # image, the base will not be truncated. If you want the base image
>>>>>> # 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
>>> Reopend, I presume?
>> The meaning here is repaired. The meaning here is the following:
>> "The base is reopened read-write and if it is dirty it should be
>> repaired before any single write is made. Guest stalls until
>> repair is complete."
> If it is dirty, it *will* (not should) be repaired before commit can
> start to write. Correct?
Absolutely.
> Is the base reopened read-only after the commit completed?
Not mandatory. For pivot cases we are committing top to
base and through out top. Base stays RW.
>>>>>> +# 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)
>>>>>> #
>>> Like change-backing-file, block-commit operates on open images. It
>>> copies down into a base image. If the base image is read-only, it is
>>> reopened, and cleaning happens when it's dirty. Correct?
>> Correct.
>>
>>> Can it happen in any other way?
>> No at the best knowledge from me and Andrey.
> Got it.
>
>>>>>> @@ -2902,6 +2908,9 @@
>>>>> ##
>>>>> # @block-stream:
>>>>> #
>>>>> # Copy data from a backing file into a block device.
>>>>>
>>>>> [...]
>>> Whereas block-commit copies down into a base image, block-stream copies
>>> up from a base image. If the image copied to is read-only, it is
>>> reopened, and cleaning happens when it's dirty. Correct?
>> Correct.
> Is the reopened back to read-only afterwards?
That is I was not tracked, but for commit image could stay RW
after op and from design point of view I believe that answer
is enough.
>>> Can it happen in any other way?
>> No at the best knowledge from me and Andrey.
> Got it.
>
>>>>>> # 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.
>>>>>> @@ -4989,6 +4998,16 @@
>>>>> ##
>>>>> # @blockdev-reopen:
>>>>> #
>>>>> # Reopens one or more block devices using the given set of options.
>>>>> # Any option not specified will be reset to its default value
>>>>> # regardless of its previous status. If an option cannot be changed
>>>>> # or a particular driver does not support reopening then the command
>>>>> # will return an error. All devices in the list are reopened in one
>>>>>> # transaction, so if one of them fails then the whole transaction is
>>>>>> # cancelled.
>>> blockdev-reopen also operates on open images. Cleaning happens when
>>> reopening a dirty read-only image read/write. Correct?
>> Correct.
>>
>>> Can it happen in any other way?
>> No.
> Got it.
>
>>>>>> #
>>>>>> +# 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.
>>>>> Could that be a problem?
>>>> Error reply as itself is not a problem. The situation as a whole
>>>> is a real pain in the ass.
> Ideally, a command does not change user-visible state when it fails.
>
> blockdev-reopen is designed to be a single transaction: success means
> all the images were reopened as directed, failure means none of the
> images were reopened. "So an error does not always mean that nothing
> has changed" indicates we're not actually implementing this design.
> Could that be a problem?
This is real pain and this could be a problem for me.
As fair as can be. I thought about other option - return
success from the reopen itself - we are succeeded on
switch and rely to signal corruption workflow, which
sends out of stream QAPI event when this condition is
detected.
This could be an option and may be I am over-designing
with this QAPI change. Anyway, this is the place than
other different thinking person is very welcome.
>>>> The problem is that such images exists in production and there
>>>> is not way to handle this without downtime.
> Yes.
>
>>>> Under this patch we are trying to fix things hard and this is
>>>> correct thing to do - on RW image we are doing exactly the
>>>> same thing. If automation is unable to fix - the guest denies
>>>> starting.
> We must not (re)open a dirty image read/write without cleaning it,
> because writing to risks corruption, i.e. data loss.
>
> When we open a dirty image read/write, we can clean it without
> inconveniencing the guest, because the guest cannot access it until
> after open completes and we connect the newly open image. Correct?
correct.
> When we reopen a read/only dirty image read/write, cleaning it *can*
> affect the guest, as discussed above.
>
> As far as I can tell, all the trouble discussed above ultimately comes
> from letting the guest work with read-only dirty images. Why is that
> useful?
>
> What are the use cases for opening dirty images read-only?
Read only images usually comes in image chains (snapshots, backing
stores). How RO image becomes dirty is very good question. May
be this was due to QEMU stop/node crash during running commit.
Why this is needed? VM should continue to start with dirty
RO image. Doing maintenance at start? That is also problematic.
At this moment we can face shared lock on base image (so called
golden image scenario).
>>>> Here we report an error and render guest as unusable. This
>>>> case is expected to be extremely rare but technically possible.
>>>>
>>>> This is a problem.
>>> I'll come back to this as soon as I understand when exactly cleaning may
>>> happen.
>> Right. Thanks.
>>
>> You are asking very good questions which are quite important
>> and interesting :-) Simple "deny" policy is very bad from
>> operations point of view.
> I try!
>
> [...]
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-27 16:06 ` Denis V. Lunev
@ 2026-08-31 12:31 ` Markus Armbruster
2026-08-31 19:01 ` Denis V. Lunev
0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2026-08-31 12:31 UTC (permalink / raw)
To: Denis V. Lunev
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
"Denis V. Lunev" <den@virtuozzo.com> writes:
> On 8/27/26 11:15, Markus Armbruster wrote:
[...]
>> We must not (re)open a dirty image read/write without cleaning it,
>> because writing to risks corruption, i.e. data loss.
>>
>> When we open a dirty image read/write, we can clean it without
>> inconveniencing the guest, because the guest cannot access it until
>> after open completes and we connect the newly open image. Correct?
> correct.
>
>> When we reopen a read/only dirty image read/write, cleaning it *can*
>> affect the guest, as discussed above.
>>
>> As far as I can tell, all the trouble discussed above ultimately comes
>> from letting the guest work with read-only dirty images. Why is that
>> useful?
>>
>> What are the use cases for opening dirty images read-only?
> Read only images usually comes in image chains (snapshots, backing
> stores). How RO image becomes dirty is very good question. May
> be this was due to QEMU stop/node crash during running commit.
>
> Why this is needed? VM should continue to start with dirty
> RO image. Doing maintenance at start? That is also problematic.
> At this moment we can face shared lock on base image (so called
> golden image scenario).
We can "do maintenance" during initial open proactively, or during
reopen read/write as needed.
In both cases, we risk delays that can make the guest hang.
Cleaning as needed might avoid some delays. It can also shift delays
from QEMU startup (sometimes bad) to guest operation (commonly worse).
Cleaning as needed in its current state appears to violate
blockdev-reopen's contract: it breaks the transaction. This feels like
a regression.
I think cleaning as needed poses challenges to management applications.
I figure sophisticated ones can make a reasonable choice between "clean
offline before you pass to QEMU" and "don't, and manage the delay on
reopen". For less sophisticated ones, and also human users, all this
feels like a trap.
Have we considered dirty image open to require an "I'm sophisticated"
flag?
Anyway, this is how far I can take this. Now the block layer
maintainers need to chime in.
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-31 12:31 ` Markus Armbruster
@ 2026-08-31 19:01 ` Denis V. Lunev
2026-08-31 22:03 ` Denis V. Lunev
0 siblings, 1 reply; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-31 19:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
On 8/31/26 14:31, Markus Armbruster wrote:
> "Denis V. Lunev" <den@virtuozzo.com> writes:
>
>> On 8/27/26 11:15, Markus Armbruster wrote:
> [...]
>
>>> We must not (re)open a dirty image read/write without cleaning it,
>>> because writing to risks corruption, i.e. data loss.
>>>
>>> When we open a dirty image read/write, we can clean it without
>>> inconveniencing the guest, because the guest cannot access it until
>>> after open completes and we connect the newly open image. Correct?
>> correct.
>>
>>> When we reopen a read/only dirty image read/write, cleaning it *can*
>>> affect the guest, as discussed above.
>>>
>>> As far as I can tell, all the trouble discussed above ultimately comes
>>> from letting the guest work with read-only dirty images. Why is that
>>> useful?
>>>
>>> What are the use cases for opening dirty images read-only?
>> Read only images usually comes in image chains (snapshots, backing
>> stores). How RO image becomes dirty is very good question. May
>> be this was due to QEMU stop/node crash during running commit.
>>
>> Why this is needed? VM should continue to start with dirty
>> RO image. Doing maintenance at start? That is also problematic.
>> At this moment we can face shared lock on base image (so called
>> golden image scenario).
> We can "do maintenance" during initial open proactively, or during
> reopen read/write as needed.
>
> In both cases, we risk delays that can make the guest hang.
>
> Cleaning as needed might avoid some delays. It can also shift delays
> from QEMU startup (sometimes bad) to guest operation (commonly worse).
>
> Cleaning as needed in its current state appears to violate
> blockdev-reopen's contract: it breaks the transaction. This feels like
> a regression.
>
> I think cleaning as needed poses challenges to management applications.
> I figure sophisticated ones can make a reasonable choice between "clean
> offline before you pass to QEMU" and "don't, and manage the delay on
> reopen". For less sophisticated ones, and also human users, all this
> feels like a trap.
>
> Have we considered dirty image open to require an "I'm sophisticated"
> flag?
>
> Anyway, this is how far I can take this. Now the block layer
> maintainers need to chime in.
>
> [...]
The problem is that we are opening RO and could not change the
image. That is the worst part and not just by the fact that
we opening RO (we can try to reopen RW) but by the fact that
another QEMU has already opened that image and we could not
take the lock. Reopen here may not happen ever.
Both ways are weird, the question is what is worse? :-)
Den
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-31 19:01 ` Denis V. Lunev
@ 2026-08-31 22:03 ` Denis V. Lunev
0 siblings, 0 replies; 15+ messages in thread
From: Denis V. Lunev @ 2026-08-31 22:03 UTC (permalink / raw)
To: Markus Armbruster
Cc: Denis V. Lunev, qemu-devel, qemu-block, Andrey Drobyshev,
Kevin Wolf, Hanna Reitz, Eric Blake, qemu-stable
On 8/31/26 21:01, Denis V. Lunev wrote:
> On 8/31/26 14:31, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@virtuozzo.com> writes:
>>
>>> On 8/27/26 11:15, Markus Armbruster wrote:
>> [...]
>>
>>>> We must not (re)open a dirty image read/write without cleaning it,
>>>> because writing to risks corruption, i.e. data loss.
>>>>
>>>> When we open a dirty image read/write, we can clean it without
>>>> inconveniencing the guest, because the guest cannot access it until
>>>> after open completes and we connect the newly open image. Correct?
>>> correct.
>>>
>>>> When we reopen a read/only dirty image read/write, cleaning it *can*
>>>> affect the guest, as discussed above.
>>>>
>>>> As far as I can tell, all the trouble discussed above ultimately comes
>>>> from letting the guest work with read-only dirty images. Why is that
>>>> useful?
>>>>
>>>> What are the use cases for opening dirty images read-only?
>>> Read only images usually comes in image chains (snapshots, backing
>>> stores). How RO image becomes dirty is very good question. May
>>> be this was due to QEMU stop/node crash during running commit.
>>>
>>> Why this is needed? VM should continue to start with dirty
>>> RO image. Doing maintenance at start? That is also problematic.
>>> At this moment we can face shared lock on base image (so called
>>> golden image scenario).
>> We can "do maintenance" during initial open proactively, or during
>> reopen read/write as needed.
>>
>> In both cases, we risk delays that can make the guest hang.
>>
>> Cleaning as needed might avoid some delays. It can also shift delays
>> from QEMU startup (sometimes bad) to guest operation (commonly worse).
>>
>> Cleaning as needed in its current state appears to violate
>> blockdev-reopen's contract: it breaks the transaction. This feels like
>> a regression.
>>
>> I think cleaning as needed poses challenges to management applications.
>> I figure sophisticated ones can make a reasonable choice between "clean
>> offline before you pass to QEMU" and "don't, and manage the delay on
>> reopen". For less sophisticated ones, and also human users, all this
>> feels like a trap.
>>
>> Have we considered dirty image open to require an "I'm sophisticated"
>> flag?
>>
>> Anyway, this is how far I can take this. Now the block layer
>> maintainers need to chime in.
>>
>> [...]
> The problem is that we are opening RO and could not change the
> image. That is the worst part and not just by the fact that
> we opening RO (we can try to reopen RW) but by the fact that
> another QEMU has already opened that image and we could not
> take the lock. Reopen here may not happen ever.
>
> Both ways are weird, the question is what is worse? :-)
>
> Den
There are 2 additional note, which I would like to share.
1. Denying start of VM with dirty RO image is no-go for
any migration of such VM over shared storage. Nothing
could be done with such a VM once node would need to be
upgraded and thus to be moved. That would be real hell
for any operation.
2. Though this could be mitigated with an addition of
forced image check through QMP and that makes a lot of
sense. Fixing image integrity for alive VM even with
several seconds pause could be a real life saver. This
functionality smells useful.
Thus we could
* make a check of RO images on start and deny if the fix
could not be made for whatever reason
* and add qmp-image-check as a command for specific node
as command
This could be correct API wise.
Thanks,
Den
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-08-31 22:04 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-24 13:37 [PATCH v4 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 2/5] block: reject a reopen of an unusable node instead of crashing Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 3/5] block: let bdrv_reopen_commit_post() report a failure Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 4/5] block: remember the flags a reopen starts from Denis V. Lunev
2026-08-24 13:37 ` [PATCH v4 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
2026-08-25 9:37 ` Markus Armbruster
2026-08-26 13:49 ` Denis V. Lunev
2026-08-26 15:24 ` Markus Armbruster
2026-08-26 16:37 ` Denis V. Lunev
2026-08-27 9:15 ` Markus Armbruster
2026-08-27 16:06 ` Denis V. Lunev
2026-08-31 12:31 ` Markus Armbruster
2026-08-31 19:01 ` Denis V. Lunev
2026-08-31 22:03 ` Denis V. Lunev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.