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

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

[Cédric Le Goater]

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()")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 migration/block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/block.c b/migration/block.c
index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
         }
 
         sectors = bdrv_nb_sectors(bs);
-        if (sectors <= 0) {
+        if (sectors == 0) {
+            continue;
+        }
+        if (sectors < 0) {
             ret = sectors;
             bdrv_next_cleanup(&it);
             goto out;
-- 
2.44.0

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

[Cédric Le Goater]

On 3/12/24 13:04, 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>

That's better :

Suggested-by: Kevin Wolf <kwolf@redhat.com>

Sorry for the noise,

C.


> Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/block.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
>           }
>   
>           sectors = bdrv_nb_sectors(bs);
> -        if (sectors <= 0) {
> +        if (sectors == 0) {
> +            continue;
> +        }
> +        if (sectors < 0) {
>               ret = sectors;
>               bdrv_next_cleanup(&it);
>               goto out;

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

[Peter Xu]

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>

This should be:

Suggested-by: Kevin Wolf <kwolf@redhat.com>

I think the missed "by" caused Kevin not in the cc list, I added Kevin in.

I'll hold a bit for Kevin to ACK, no repost needed just for above; I can
fix it.

Thanks,

> Fixes: fea68bb6e9fa ("block: Eliminate bdrv_iterate(), use bdrv_next()")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  migration/block.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..2b9054889ad2ba739828594c50cf047703757e96 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -402,7 +402,10 @@ static int init_blk_migration(QEMUFile *f)
>          }
>  
>          sectors = bdrv_nb_sectors(bs);
> -        if (sectors <= 0) {
> +        if (sectors == 0) {
> +            continue;
> +        }
> +        if (sectors < 0) {
>              ret = sectors;
>              bdrv_next_cleanup(&it);
>              goto out;
> -- 
> 2.44.0
> 
> 

-- 
Peter Xu

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

[Stefan Hajnoczi]

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

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?

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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.
	

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Peter Xu]

On Tue, Mar 12, 2024 at 05:34:26PM -0400, Stefan Hajnoczi wrote:
> I understand now. I missed that returning from init_blk_migration_it()
> did not abort iteration. Thank you!

I queued it, thanks both!

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 12, 2024 at 05:34:26PM -0400, Stefan Hajnoczi wrote:
>> I understand now. I missed that returning from init_blk_migration_it()
>> did not abort iteration. Thank you!
>
> I queued it, thanks both!

Thanks for cleaning up the mess I made!

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

[Kevin Wolf]

Am 12.03.2024 um 13:27 hat Peter Xu geschrieben:
> 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>
> 
> This should be:
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> 
> I think the missed "by" caused Kevin not in the cc list, I added Kevin in.
> 
> I'll hold a bit for Kevin to ACK, no repost needed just for above; I can
> fix it.

Thanks.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>