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... > Kevin,
have you had a chance to take a look? Den
