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 <[email protected]> > > CC: Kevin Wolf <[email protected]> > > CC: Hanna Reitz <[email protected]> > > --- > > 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
