On 7/24/26 18:54, Kevin Wolf wrote:
> 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?
=== Image setup ===
qemu-img create -f qcow2 -o compat=1.1,lazy_refcounts=on dirty.qcow2 64M
qemu-io -c "write -P 0x5a 0 512" -c "sigraise 9" dirty.qcow2
# expect: "wrote 512/512 bytes..." followed by "Killed" from the shell
qemu-img info --output=json dirty.qcow2 | grep dirty-flag
# expect: "dirty-flag": true
=== Launch daemon ===
./build/storage-daemon/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 \
> master.log 2>&1 &
=== Connect and drive it ===
./build/run qmp-shell master.sock
(QEMU) quit
Expected result:
[1]+ Aborted (core dumped) ./build/storage-daemon/qemu-storage-daemon
Double checked this on current QEMU master.
Here I have just stated that the case inside the test I have added
in the thread starter is not artificial and this is not a mistake.
Such a command line has been generated by libvirt for me.
>> 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.
OK. You say that at the bdrv_co_write_req_prepare() we check
only possible permission and that is ok. That was really
unexpected to me and that is why I have wonders. Designed this
way, no changes going in.
I can live with that :-)
Den
