Re: [PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Kevin Wolf <kwolf@redhat.com>]

Am 16.07.2026 um 19:54 hat Denis V. Lunev geschrieben:
> On 7/16/26 17:35, Denis V. Lunev wrote:
> > This email originated from an IP that might not be authorized by the domain it was sent from.
> > Do not click links or open attachments unless it is an email you expected to receive.
> > qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
> > plain write to bs->file, unconditionally. A read-only node can still
> > be dirty, inherited from an earlier writable session, and that write
> > then hits a missing BLK_PERM_WRITE and asserts in
> > bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
> > close -- closing is expected, the dirty bit on a read-only node
> > is not.
> >
> > Skip the clear for read-only nodes, same as read access already does.
> > Any other still-dirty node keeps the unguarded write: it is expected
> > to hold write permission, and a missing one there is a bug worth
> > seeing.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Hanna Reitz <hreitz@redhat.com>
> > ---
> >  block/qcow2.c              |  6 +++++-
> >  tests/qemu-iotests/039     | 11 +++++++++++
> >  tests/qemu-iotests/039.out |  3 +++
> >  3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 2ab6ddd7b0..adb177e6b6 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
> >                       strerror(-ret));
> >      }
> >  
> > -    if (result == 0) {
> > +    /*
> > +     * A read-only node cannot resolve an inherited dirty bit here;
> > +     * leave it dirty, same as plain read access already does.
> > +     */
> > +    if (result == 0 && !bdrv_is_read_only(bs)) {
> >          qcow2_mark_clean(bs);
> >      }
> >  
> > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> > index e43e7026ce..94a8bfe754 100755
> > --- a/tests/qemu-iotests/039
> > +++ b/tests/qemu-iotests/039
> > @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
> >  # The dirty bit must be set
> >  _qcow2_dump_header | grep incompatible_features
> >  
> > +echo
> > +echo "== Read-only open must not crash on close =="
> > +
> > +# We must not try to write the QCOW2 header to a read-only image.
> > +$QEMU_IMG info --image-opts \
> > +    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
> > +    > /dev/null
> > +
> > +# The dirty bit must still be set: this open never wrote any guest data
> > +_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 8fdbcc528a..c66361128f 100644
> > --- a/tests/qemu-iotests/039.out
> > +++ b/tests/qemu-iotests/039.out
> > @@ -24,6 +24,9 @@ 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 open must not crash on close ==
> > +incompatible_features     [0]
> > +
> >  == Repairing the image file must succeed ==
> >  ERROR cluster 5 refcount=0 reference=1
> >  Rebuilding refcount structure
> Actually the problem is more subtle. The following command line results in
> 
> qemu-storage-daemon \                                                   
>   --blockdev
> '{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
> \                    
>   --blockdev
> '{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
> \                                                   
>   --chardev socket,id=qmp,path=master.sock,server=on,wait=off \         
>   --monitor chardev=qmp                                                 
>                                                          
> via query-named-block-nodes:
> 
>   node-name=fmt0     drv=qcow2  ro=True
>   node-name=file0    drv=file   ro=False
> 
> which would come to exactly configuration being asserted in the test
> and this setup is pretty legal at my opinion. In production we have
> had something lengthier with more snapshots.

Yes, I don't see a problem with it.

However, I also don't get a crash or anything with your QSD command line
and a dirty image with this patch applied, so what is the part that is
still incorrect?

What is the final result that you're seeing here?

> File descriptor in turn is opened in a correct way - with O_RDONLY
> that is why the problem was hidden for a lot of time.
> 
> Would it be more reasonable to disallow to set dirty rather than
> to ignore is open question to me. Playing with permissions and
> changing bdrv_is_read_only() seems more dangerous.

I don't follow, sorry. The patch fixes a code path that marks the image
as clean. Setting the dirty flag is the opposite operation and happens
here before we even opened the image. How would you disallow that?

> Also worth to note that bdrv_co_write_req_prepare() has wrong check.
> It should check file descriptor rather than administrative permission.

That doesn't sounds right. bdrv_co_write_req_prepare() is a generic
block layer function, the file descriptor is a file-posix implementation
detail.

> This looks toooooooooo complex to me :(
> 
> Den

Kevin