On Tue, Mar 12, 2024 at 09:22:06PM +0100, Cédric Le Goater wrote: > On 3/12/24 19:41, Stefan Hajnoczi wrote: > > On Tue, Mar 12, 2024 at 01:04:31PM +0100, Cédric Le Goater wrote: > > > The block .save_setup() handler calls a helper routine > > > init_blk_migration() which builds a list of block devices to take into > > > account for migration. When one device is found to be empty (sectors > > > == 0), the loop exits and all the remaining devices are ignored. This > > > is a regression introduced when bdrv_iterate() was removed. > > > > > > Change that by skipping only empty devices. > > > > > > Cc: Markus Armbruster <arm...@redhat.com> > > > Suggested: Kevin Wolf <kw...@redhat.com> > > > Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()") > > > > It's not clear to me that fea68bb6e9fa introduced the bug. The code is > > still <= 0 there and I don't see anything else that skips empty devices. > > Can you explain the bug in fea68bb6e9fa? > > Let me try. Initially, the code was : > static void init_blk_migration_it(void *opaque, BlockDriverState *bs) > { > BlockDriverState *bs; > BlkMigDevState *bmds; > int64_t sectors; > if (!bdrv_is_read_only(bs)) { > sectors = bdrv_nb_sectors(bs); > if (sectors <= 0) { > return; > ... > } > > static void init_blk_migration(QEMUFile *f) > { > block_mig_state.submitted = 0; > block_mig_state.read_done = 0; > block_mig_state.transferred = 0; > block_mig_state.total_sector_sum = 0; > block_mig_state.prev_progress = -1; > block_mig_state.bulk_completed = 0; > block_mig_state.zero_blocks = migrate_zero_blocks(); > bdrv_iterate(init_blk_migration_it, NULL); > } > > bdrv_iterate() was iterating on all devices and exiting one iteration > early if the device was empty or an error detected. The loop applied > on all devices always, only empty devices and the ones with a failure > were skipped. > It was replaced by : > > static void init_blk_migration(QEMUFile *f) > { > BlockDriverState *bs; > BlkMigDevState *bmds; > int64_t sectors; > block_mig_state.submitted = 0; > block_mig_state.read_done = 0; > block_mig_state.transferred = 0; > block_mig_state.total_sector_sum = 0; > block_mig_state.prev_progress = -1; > block_mig_state.bulk_completed = 0; > block_mig_state.zero_blocks = migrate_zero_blocks(); > for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) { > if (bdrv_is_read_only(bs)) { > continue; > } > sectors = bdrv_nb_sectors(bs); > if (sectors <= 0) { > return; > > ... > } > > The loop and function exit at first failure or first empty device, > skipping the remaining devices. This is a different behavior. > > What we would want today is ignore empty devices, as it done for > the read only ones, return at first failure and let the caller of > init_blk_migration() report. > > This is a short term fix for 9.0 because the block migration code > is deprecated and should be removed in 9.1.
I understand now. I missed that returning from init_blk_migration_it() did not abort iteration. Thank you! Stefan
signature.asc
Description: PGP signature