Re: [PATCH for 9.0] migration: Skip only empty block devices

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

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 <armbru@redhat.com>
> > > Suggested: Kevin Wolf <kwolf@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]