* [PATCH v3 1/5] qcow2: do not clear the dirty bit when reopening a read-only node
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
@ 2026-08-19 12:05 ` Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 2/5] block: reject a reopen of an unusable node instead of crashing Denis V. Lunev
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-19 12:05 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Andrey Drobyshev, Kevin Wolf,
Hanna Reitz
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>
---
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] 8+ messages in thread* [PATCH v3 2/5] block: reject a reopen of an unusable node instead of crashing
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
@ 2026-08-19 12:05 ` Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 3/5] block: let bdrv_reopen_commit_post() report a failure Denis V. Lunev
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-19 12:05 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Kevin Wolf, Hanna Reitz,
Andrey Drobyshev
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>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
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] 8+ messages in thread* [PATCH v3 3/5] block: let bdrv_reopen_commit_post() report a failure
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 1/5] qcow2: do not clear the dirty bit when reopening a read-only node Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 2/5] block: reject a reopen of an unusable node instead of crashing Denis V. Lunev
@ 2026-08-19 12:05 ` Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 4/5] block: remember the flags a reopen starts from Denis V. Lunev
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-19 12:05 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Kevin Wolf, Hanna Reitz,
Andrey Drobyshev
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>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
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] 8+ messages in thread* [PATCH v3 4/5] block: remember the flags a reopen starts from
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
` (2 preceding siblings ...)
2026-08-19 12:05 ` [PATCH v3 3/5] block: let bdrv_reopen_commit_post() report a failure Denis V. Lunev
@ 2026-08-19 12:05 ` Denis V. Lunev
2026-08-19 12:05 ` [PATCH v3 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
2026-08-21 17:34 ` [PATCH v3 0/5] qcow2: silent corruption when a dirty image " Andrey Drobyshev
5 siblings, 0 replies; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-19 12:05 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Kevin Wolf, Hanna Reitz,
Andrey Drobyshev
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>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
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] 8+ messages in thread* [PATCH v3 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
` (3 preceding siblings ...)
2026-08-19 12:05 ` [PATCH v3 4/5] block: remember the flags a reopen starts from Denis V. Lunev
@ 2026-08-19 12:05 ` Denis V. Lunev
2026-08-21 17:24 ` Andrey Drobyshev
2026-08-21 17:34 ` [PATCH v3 0/5] qcow2: silent corruption when a dirty image " Andrey Drobyshev
5 siblings, 1 reply; 8+ messages in thread
From: Denis V. Lunev @ 2026-08-19 12:05 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Denis V. Lunev, Kevin Wolf, Hanna Reitz,
Andrey Drobyshev
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 leaves only
what qcow2_signal_corruption() does, take the driver away from the node,
rather than let writes alias live clusters. Return the error and skip
the bitmaps.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/qcow2.c | 21 +++++++
qapi/block-core.json | 16 +++++
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, 257 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..9aec081f7b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1891,6 +1891,11 @@
# 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 for the duration of the job. A dirty
+# qcow2 base is repaired first, which reads all of its metadata and
+# holds up every other request while it runs. The command fails if
+# that repair does not succeed.
+#
# @job-id: identifier for the newly-created block job. If omitted,
# the device name will be used. (Since 2.7)
#
@@ -4989,6 +4994,17 @@
# 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
+# reads all of its metadata and holds up every other request while it
+# runs.
+#
# 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] 8+ messages in thread* Re: [PATCH v3 5/5] qcow2: repair a dirty image when it becomes writable
2026-08-19 12:05 ` [PATCH v3 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
@ 2026-08-21 17:24 ` Andrey Drobyshev
0 siblings, 0 replies; 8+ messages in thread
From: Andrey Drobyshev @ 2026-08-21 17:24 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel; +Cc: qemu-block, Kevin Wolf, Hanna Reitz
On 8/19/26 3:05 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
>
> A dirty image must be repaired before anything allocates a cluster in
> it. qcow2_do_open() does that, but only for a node that is writable
> from the start. A node opened read-only skips it, and nothing revisits
> the question once that node becomes writable, which block-commit does
> routinely: commit_active_start() and commit_start() reopen the base
> read-write for the duration of the job.
>
> With lazy refcounts the on-disk refcount block then still accounts for
> the metadata clusters only, so the allocator restarts at the front of
> the image and hands out clusters that L2 entries point at. Two guest
> offsets end up sharing one host cluster. Nothing fails, the corrupt bit
> stays clear, and a clean close clears the dirty bit, so no later open
> repairs the image either. The bit also stays set for the whole writable
> session, so a node which is merely writable says nothing.
>
> Refusing the reopen instead is simpler and keeps it atomic, but it
> leaves nowhere to go: the base belongs to a chain the VM has open, so
> the qemu-img check -r such an error would ask for cannot take the write
> lock it needs. The repair does the trick in most cases anyway.
>
> Do the repair in qcow2_reopen_commit_post(), the earliest point where
> the node is writable. An inactive node is skipped: bdrv_activate() calls
> qcow2_do_open() again through qcow2_co_invalidate_cache().
>
> commit_post cannot reject the reopen, so a failed repair leaves only
> what qcow2_signal_corruption() does, take the driver away from the node,
> rather than let writes alias live clusters. Return the error and skip
> the bitmaps.
>
Nit: qcow2_signal_corruption() also sends qapi event and writes corrupt
bit to the header, and we don't do it here. AFAICT the code is right,
but this claim is a bit misleading. Maybe clarify why no event should
be emitted.
Another nit: block-stream and change-backing file also seem to be doing
RO->RW reopen, but they aren't mentioned. Should their docs also be
updated?
Andrey
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
> block/qcow2.c | 21 +++++++
> qapi/block-core.json | 16 +++++
> 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, 257 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;
Your commit says: "... failed repair leaves only what
qcow2_signal_corruption() does".
> + 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..9aec081f7b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1891,6 +1891,11 @@
> # 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 for the duration of the job. A dirty
> +# qcow2 base is repaired first, which reads all of its metadata and
> +# holds up every other request while it runs. The command fails if
> +# that repair does not succeed.
> +#
> # @job-id: identifier for the newly-created block job. If omitted,
> # the device name will be used. (Since 2.7)
> #
> @@ -4989,6 +4994,17 @@
> # 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
> +# reads all of its metadata and holds up every other request while it
> +# runs.
> +#
> # 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable
2026-08-19 12:05 [PATCH v3 0/5] qcow2: silent corruption when a dirty image becomes writable Denis V. Lunev
` (4 preceding siblings ...)
2026-08-19 12:05 ` [PATCH v3 5/5] qcow2: repair a dirty image when it becomes writable Denis V. Lunev
@ 2026-08-21 17:34 ` Andrey Drobyshev
5 siblings, 0 replies; 8+ messages in thread
From: Andrey Drobyshev @ 2026-08-21 17:34 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel; +Cc: qemu-block, Kevin Wolf, Hanna Reitz
On 8/19/26 3:05 PM, Denis V. Lunev wrote:
> 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/
>
> 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 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>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>
> 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 | 16 ++++
> 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, 436 insertions(+), 15 deletions(-)
>
>
> base-commit: fa19879df1658f96ac07365fca8835b7decd6995
Overall LGTM, with a couple of wording nits in the last commit. For the
series:
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Btw this respin lost 'Cc: qemu-stable@nongnu.org' compared to v2 - but
it's probably needed since we're fixing genuine crash.
Andrey
^ permalink raw reply [flat|nested] 8+ messages in thread