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

[Cédric Le Goater]

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.

Thanks,

C.