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

["Denis V. Lunev" <den@virtuozzo.com>]

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 <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>
> ---
>
> 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...
>  
Kevin,

have you had a chance to take a look?

Den