On 8/29/26 00:20, 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.
> From: Denis V. Lunev <[email protected]>
>
> 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 <[email protected]>
> Cc: Fiona Ebner <[email protected]>
> Cc: Kevin Wolf <[email protected]>
> ---
>
> 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.
>
> block/block-backend.c | 1 +
> tests/qemu-iotests/tests/inactive-node-nbd | 16 ++++++++++++++++
> tests/qemu-iotests/tests/inactive-node-nbd.out | 8 ++++++++
> 3 files changed, 25 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 37ba7e9fc4..55497c4551 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -625,6 +625,7 @@ BlockDriverState *bdrv_next(BdrvNextIterator *it)
> return bs;
> }
> it->phase = BDRV_NEXT_MONITOR_OWNED;
> + it->bs = NULL;
> }
>
> /* Then return the monitor-owned BDSes without a BB attached. Ignore all
> diff --git a/tests/qemu-iotests/tests/inactive-node-nbd
> b/tests/qemu-iotests/tests/inactive-node-nbd
> index 664157bfd0..ef876ae6be 100755
> --- a/tests/qemu-iotests/tests/inactive-node-nbd
> +++ b/tests/qemu-iotests/tests/inactive-node-nbd
> @@ -47,6 +47,7 @@ def node_is_active(_vm, node_name):
> return node['active']
>
> with iotests.FilePath('disk.img') as path, \
> + iotests.FilePath('detached.img') as detached_path, \
> iotests.FilePath('snap.qcow2') as snap_path, \
> iotests.FilePath('snap2.qcow2') as snap2_path, \
> iotests.FilePath('target.img') as target_path, \
> @@ -58,6 +59,7 @@ with iotests.FilePath('disk.img') as path, \
> iotests.log('Preparing disk...')
> iotests.qemu_img_create('-f', iotests.imgfmt, path, img_size)
> iotests.qemu_img_create('-f', iotests.imgfmt, target_path, img_size)
> + iotests.qemu_img_create('-f', iotests.imgfmt, detached_path, img_size)
>
> iotests.qemu_img_create('-f', 'qcow2', '-b', path, '-F', iotests.imgfmt,
> snap_path)
> @@ -68,6 +70,9 @@ with iotests.FilePath('disk.img') as path, \
> vm.add_blockdev(f'file,node-name=disk-file,filename={path}')
> vm.add_blockdev(f'{iotests.imgfmt},file=disk-file,node-name=disk-fmt,'
> 'active=off')
> + vm.add_blockdev(f'file,node-name=detached-file,filename={detached_path}')
> + vm.add_blockdev(f'{iotests.imgfmt},file=detached-file,'
> + 'node-name=detached-fmt')
> vm.add_blockdev(f'file,node-name=target-file,filename={target_path}')
>
> vm.add_blockdev(f'{iotests.imgfmt},file=target-file,node-name=target-fmt')
> vm.add_blockdev(f'file,node-name=snap-file,filename={snap_path}')
> @@ -297,6 +302,17 @@ with iotests.FilePath('disk.img') as path, \
> iotests.log('snap2-fmt active: %s' % node_is_active(vm, 'snap2-fmt'))
> iotests.log('target-fmt active: %s' % node_is_active(vm, 'target-fmt'))
>
> + iotests.log('\n=== Inactivating all nodes at once ===')
> +
> + # detached-fmt has no parent and no BlockBackend, so nothing can reach it
> + # by recursion. It is only inactivated if bdrv_next() actually returns
> it.
> + vm.qmp_log('stop')
> + vm.qmp_log('blockdev-set-active', active=False)
> +
> + iotests.log('detached-fmt active: %s' % node_is_active(vm,
> 'detached-fmt'))
> + iotests.log('detached-file active: %s'
> + % node_is_active(vm, 'detached-file'))
> +
> iotests.log('\nShutting down...')
> vm.shutdown()
> log = vm.get_log()
> diff --git a/tests/qemu-iotests/tests/inactive-node-nbd.out
> b/tests/qemu-iotests/tests/inactive-node-nbd.out
> index a458b4fc05..48451dc0c5 100644
> --- a/tests/qemu-iotests/tests/inactive-node-nbd.out
> +++ b/tests/qemu-iotests/tests/inactive-node-nbd.out
> @@ -235,5 +235,13 @@ snap-fmt active: True
> snap2-fmt active: True
> target-fmt active: True
>
> +=== Inactivating all nodes at once ===
> +{"execute": "stop", "arguments": {}}
> +{"return": {}}
> +{"execute": "blockdev-set-active", "arguments": {"active": false}}
> +{"return": {}}
> +detached-fmt active: False
> +detached-file active: False
> +
> Shutting down...
>
ping
