On 8/1/26 1:00 AM, Denis V. Lunev wrote:
> 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.
>
> Do the repair in qcow2_reopen_commit_post(). bdrv_reopen_prepare() runs
> before bdrv_list_refresh_perms(), so it holds no BLK_PERM_WRITE and,
> with auto-read-only, bs->file may still have an O_RDONLY descriptor.
> commit_post cannot reject the reopen, so signal corruption if the
> repair fails rather than let writes alias live clusters. An inactive
> node is skipped: bdrv_activate() calls qcow2_do_open() again through
> qcow2_co_invalidate_cache().
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> ---
> block/qcow2.c | 14 ++++++++++++++
> tests/qemu-iotests/039 | 24 ++++++++++++++++++++++++
> tests/qemu-iotests/039.out | 17 +++++++++++++++++
> 3 files changed, 55 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1543255eba..e660655a0d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2147,8 +2147,22 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>
> static void qcow2_reopen_commit_post(BDRVReopenState *state)
> {
> + BDRVQcow2State *s = state->bs->opaque;
> +
> GRAPH_RDLOCK_GUARD_MAINLOOP();
>
> + if (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) {
> + qcow2_signal_corruption(state->bs, true, -1, -1,
> + "Could not repair dirty image");
> + }
> + }
> +
This looks wrong. What if we blockdev_reopen() a healthy image which is
already RW? Say, we want to tweak L2 cache size. Then we reopen
RW->RW, and yet with your patch we do full scan of metadata. IIUC we're
being drained at this point -> we get guest IO stalled because of this
scan.
The subject says "... when it BECOMES writable". But essentially we're
doing full check anytime it IS writable. That's a completely different
thing.
Andrey
> if (state->flags & BDRV_O_RDWR) {
> Error *local_err = NULL;
>
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index a5be81bc4a..255dc2e7e5 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -137,6 +137,30 @@ $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
> +
> +# The refcounts are only resolved when the node becomes writable. Without
> +# that, this write is allocated on top of the cluster at offset 0 and reading
> +# it back returns the data written here.
> +$QEMU_IO -r -c "reopen -w" \
> + -c "write -P 0xb1 1M 512" \
> + -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
> +
> +# The dirty bit must not be set
> +_qcow2_dump_header | grep incompatible_features
> +_check_test_img
> +
> 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 3c71e5a3dd..62073916a9 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -64,6 +64,23 @@ 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)
> +incompatible_features []
> +No errors were found on the image.
> +
> == Creating an image file with lazy_refcounts=off ==
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> wrote 512/512 bytes at offset 0
