Re: [PATCH] block: fix bdrv_next() skipping monitor-owned nodes

[Fiona Ebner <f.ebner@proxmox.com>]

Hi Denis,

Am 29.08.26 um 12:20 AM schrieb Denis V. Lunev:
> From: Denis V. Lunev <den@openvz.org>
> 
> it->bs is the cursor into monitor_bdrv_states for the second phase of
> bdrv_next(), so it has to be NULL when that phase starts. Commit
> f6d38c9f6d made the first phase store the node it returns there, to
> unreference the right one when the graph changes underneath. The cursor
> is now left pointing at the last BlockBackend root, so the second phase
> resumes from there instead of from the head of the list and never
> returns a node added before it.
> 
> A skipped node drops out of the vm_stop and migration handover paths:
> not flushed, not inactivated, not snapshotted. bdrv_inactivate_all()
> still reports success, so the node keeps its image lock and the
> migration target cannot open the image. Only detached nodes are hit in
> practice, as these callers also recurse into children.
> 
> Reset the cursor when the second phase starts. old_bs is taken at the
> top of the function, so f6d38c9f6d keeps working.
> 
> Fixes: f6d38c9f6d ("block-backend: fix edge case in bdrv_next() where BDS associated to BB changes")
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Fiona Ebner <f.ebner@proxmox.com>
> Cc: Kevin Wolf <kwolf@redhat.com>

thank you for the fix!

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>

Should also go into stable I suppose?

> ---
> 
> Notes:
>     The iterator was correct from its introduction in 2016 until 2024:
>     it->bs was written only by the second phase, and the node to unreference
>     came from the BlockBackend instead.
> 
>     f6d38c9f6d's own reproducer does not fire for me, 240 runs across 8
>     timings, even with its fix reverted. Tried hard.

The reproducer has a very short time window. I'm still able to trigger
an assertion failure with commit f6d38c9f6d reverted, by putting in some
'query-version' commands in between the 'block-stream' and 'quit'
commands, and running the script in a loop, e.g.:

[I] root@pve9a1 ~# cat ~/block-stream.sh
#!/bin/bash
rm -f /tmp/backing.qcow2
rm -f /tmp/top.qcow2
./qemu-img create /tmp/backing.qcow2 -f qcow2 64M
./qemu-io -c "write -P42 0x0 0x1" /tmp/backing.qcow2
./qemu-img create /tmp/top.qcow2 -f qcow2 64M -b /tmp/backing.qcow2 -F qcow2
./qemu-system-x86_64 --qmp stdio \
--blockdev
qcow2,node-name=node0,file.driver=file,file.filename=/tmp/top.qcow2 \
<<EOF
{"execute": "qmp_capabilities"}
{"execute": "block-stream", "arguments": { "job-id": "stream0",
"device": "node0" } }
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "query-version"}
{"execute": "quit"}
EOF

Best Regards,
Fiona