Re: [PATCH v2 4/4] qcow2: repair a dirty image when it becomes writable

On 8/11/26 6:42 PM, 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. That drops
> bs->drv, so return the error and skip the bitmaps. An inactive node is
> skipped: bdrv_activate() calls qcow2_do_open() again through
> qcow2_co_invalidate_cache().
> 
> With lazy refcounts the dirty bit stays set for the whole writable
> session, so a node that is merely writable says nothing. Repair only
> when it has just become writable, which BDRVReopenState remembers:
> a read-write to read-write reopen must not scan a live image.
> 
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
> Cc: [email protected]
> ---
>  block/qcow2.c              | 17 +++++++++++
>  tests/qemu-iotests/039     | 60 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/039.out | 36 +++++++++++++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 553a94d003..bac3e49cf5 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2147,8 +2147,25 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
>  
>  static int qcow2_reopen_commit_post(BDRVReopenState *state, Error **errp)
>  {
> +    BDRVQcow2State *s = state->bs->opaque;
> +
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> +    if (!state->was_writable && bdrv_is_writable(state->bs) &&
> +        (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {

Nit: qcow2_do_open() also guards this with !(flags & BDRV_O_CHECK).
AFAICT we can't reach it here since the only place we open with
BDRV_O_CHECK is qemu-img check, and it never does reopen.  Still, might
consider adding the guard.

> +        BdrvCheckResult result = {0};
> +        int ret;
> +
> +        ret = bdrv_check(state->bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0 || result.check_errors) {
> +            /* This clears bs->drv, there is nothing left to reopen */
> +            qcow2_signal_corruption(state->bs, true, -1, -1,
> +                                    "Could not repair dirty image");
> +            error_setg(errp, "Could not repair dirty image");

Which image?  blockdev-reopen can process multiple nodes.  And we have
no clue which one failed.  How about adding
bdrv_get_device_or_node_name(state->bs)?

> +            return -EIO;

Path qcow2_do_open() -> qcow2_co_check_locked() does preserve the
check's error: ret < 0 ? ret : -EIO; error_setg_errno(-ret).  Here a
-ENOMEM from the check comes back as -EIO.

> +        }
> +    }
> +

There's also a logical issue with this check.  qcow2_signal_corruption()
does 'bs->drv = NULL', which explicitly makes the node unusable.  Now,
block commit does reopen the base node to RW, remembering its previous
state.  If reopen fails, it does rollback reopen to RO.  And on this 2nd
reopen we'll likely end up asserting on:

bdrv_reopen_set_read_only()
  bdrv_reopen()
    bdrv_reopen_multiple()
      bdrv_reopen_prepare()
        assert(reopen_state->bs->drv != NULL)

on this unusable node.

Can we afford making bdrv_reopen_prepare() return an error instead of
crashing?  Or maybe guard this rollback in commit_start() by
!bdrv_is_inserted(base).  IMO this is either a separate commit, or part
of 2nd commit.

And we should probably add a qmp_block_commit test which would cover
exactly this problematic case.


Also the QAPI docs for blockdev-reopen say:

> ... if one of them fails then the whole transaction is cancelled.

That no longer appears to be true.  Probably should amend the docs as well.

Andrey

>      if (state->flags & BDRV_O_RDWR) {
>          Error *local_err = NULL;
>  
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index db52c580d1..dd4f48fa14 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -136,6 +136,66 @@ $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
> +
> +# The dirty bit must not be set
> +_qcow2_dump_header | grep incompatible_features
> +_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
> +
> +rm -f "$TEST_DIR/blkdebug.conf"
> +
>  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..c0e1bf0a09 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -64,6 +64,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)
> +incompatible_features     []
> +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
> +qcow2: Marking image as corrupt: Could not repair dirty image; further corruption events will be suppressed
> +qemu-io: Could not repair dirty image
> +read failed: No medium found
> +
>  == Creating an image file with lazy_refcounts=off ==
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  wrote 512/512 bytes at offset 0