1. Patchew
  2. QEMU
  3. View series

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

Cédric Le Goater posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240312120431.550054-1-clg@redhat.com
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
migration/block.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[PATCH for 9.0] migration: Skip only empty block devices
Posted by Cédric Le Goater 1 month, 2 weeks ago 
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
Posted by Stefan Hajnoczi 1 month, 2 weeks ago 
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>
Re: [PATCH for 9.0] migration: Skip only empty block devices
Posted by Cédric Le Goater 1 month, 2 weeks ago 
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
Posted by Stefan Hajnoczi 1 month, 2 weeks ago 
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
Re: [PATCH for 9.0] migration: Skip only empty block devices
Posted by Peter Xu 1 month, 2 weeks ago 
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
Posted by Markus Armbruster 1 month, 2 weeks ago 
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
Posted by Peter Xu 1 month, 2 weeks ago 
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
Posted by Kevin Wolf 1 month, 2 weeks ago 
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>
Re: [PATCH for 9.0] migration: Skip only empty block devices
Posted by Cédric Le Goater 1 month, 2 weeks ago 
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;

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.