Re: [PATCH v2 1/4] qcow2: do not clear the dirty bit when reopening a read-only node

On 8/11/26 6:42 PM, Denis V. Lunev wrote:
> 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'
> 
> 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 <[email protected]>
> CC: Kevin Wolf <[email protected]>
> CC: Hanna Reitz <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
> Cc: [email protected]
> ---
>  block/qcow2.c              |  8 +++++---
>  tests/qemu-iotests/039     | 10 ++++++++++
>  tests/qemu-iotests/039.out |  5 +++++
>  3 files changed, 20 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..db52c580d1 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -95,6 +95,16 @@ $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 "== Repairing the image file must succeed =="
>  
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index c66361128f..3c71e5a3dd 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -27,6 +27,11 @@ 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]
> +
>  == Repairing the image file must succeed ==
>  ERROR cluster 5 refcount=0 reference=1
>  Rebuilding refcount structure

bdrv_is_writable() checks pre-reopen state.  So IMHO it'd also be nice
to check RW->RO reopen and that it does clear the dirty bit.  Smth like:

> _make_test_img -o "compat=1.1,lazy_refcounts=on" $size
> $QEMU_IO -c "write -P 0x5a 0 512" -c "reopen -r" "$TEST_IMG" | _filter_qemu_io
> _qcow2_dump_header | grep incompatible_features

A write operation with lazy refcounts must set dirty bit.  And upon
RW->RO reopen the dirty bit must be cleared.

Still, with or without this case covered:

Reviewed-by: Andrey Drobyshev <[email protected]>