migration: Fix block failure cases

[Qemu-devel] [PATCH] migration: Fix block failure cases

Posted by Dr. David Alan Gilbert (git) 7 years, 7 months ago

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This fixes a couple of cases where the block migration capability
doesn't get cleared when a migration failed.

1) When block migration is compiled out:
  (qemu) migrate -d -b  "exec:cat > /dev/null"
  QEMU compiled without old-style (blk/-b, inc/-i) block migration
  Use drive_mirror+NBD instead.
  (qemu) migrate_set_capability xbzrle off
  QEMU compiled without old-style (blk/-b, inc/-i) block migration
  Use drive_mirror+NBD instead.

  This corresponds to https://bugzilla.redhat.com/show_bug.cgi?id=1550022

2) When a migration with a bad protocol is tried:
  (qemu) migrate -d -b "foo:bah"
  Parameter 'uri' expects a valid migration protocol
  (qemu) info migrate_capabilities
  xbzrle: off
  rdma-pin-all: off
  auto-converge: off
  zero-blocks: off
  compress: off
  events: off
  postcopy-ram: off
  x-colo: off
  release-ram: off
  block: on   <<<<<<-----
  return-path: off
  pause-before-switchover: off
  x-multifd: off

Fixes: 2833c59b947
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b913b98803..da0e4a1f56 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1391,11 +1391,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
             return;
         }
         migrate_set_block_enabled(true, &local_err);
+        s->must_remove_block_options = true;
         if (local_err) {
             error_propagate(errp, local_err);
+            block_cleanup_parameters(s);
             return;
         }
-        s->must_remove_block_options = true;
     }
 
     if (has_inc && inc) {
@@ -1417,11 +1418,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
     } else {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
+        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
-        return;
     }
 
     if (local_err) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH] migration: Fix block failure cases

Posted by Peter Xu 7 years, 7 months ago

On Wed, Feb 28, 2018 at 04:49:37PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This fixes a couple of cases where the block migration capability
> doesn't get cleared when a migration failed.
> 
> 1) When block migration is compiled out:
>   (qemu) migrate -d -b  "exec:cat > /dev/null"
>   QEMU compiled without old-style (blk/-b, inc/-i) block migration
>   Use drive_mirror+NBD instead.
>   (qemu) migrate_set_capability xbzrle off
>   QEMU compiled without old-style (blk/-b, inc/-i) block migration
>   Use drive_mirror+NBD instead.
> 
>   This corresponds to https://bugzilla.redhat.com/show_bug.cgi?id=1550022
> 
> 2) When a migration with a bad protocol is tried:
>   (qemu) migrate -d -b "foo:bah"
>   Parameter 'uri' expects a valid migration protocol
>   (qemu) info migrate_capabilities
>   xbzrle: off
>   rdma-pin-all: off
>   auto-converge: off
>   zero-blocks: off
>   compress: off
>   events: off
>   postcopy-ram: off
>   x-colo: off
>   release-ram: off
>   block: on   <<<<<<-----
>   return-path: off
>   pause-before-switchover: off
>   x-multifd: off
> 
> Fixes: 2833c59b947
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b913b98803..da0e4a1f56 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1391,11 +1391,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>              return;
>          }
>          migrate_set_block_enabled(true, &local_err);
> +        s->must_remove_block_options = true;
>          if (local_err) {
>              error_propagate(errp, local_err);
> +            block_cleanup_parameters(s);
>              return;
>          }
> -        s->must_remove_block_options = true;
>      }
>  
>      if (has_inc && inc) {
> @@ -1417,11 +1418,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_outgoing_migration(s, p, &local_err);
>      } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
> +        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");
>          migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>                            MIGRATION_STATUS_FAILED);
> -        return;
>      }
>  
>      if (local_err) {
> -- 
> 2.14.3
> 

Ouch...

Instead it seems to be my fault in 4a84214ebe ("migration: provide
migrate_caps_check()", 2017-07-18).  For now I cannot understand why I
did that before since it's obviously strange if without this
squashed...

diff --git a/migration/migration.c b/migration/migration.c
index 0aa596f867..88ed9375aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 {
     MigrationState *s = migrate_get_current();
     MigrationCapabilityStatusList *cap;
+    bool cap_list[MIGRATION_CAPABILITY__MAX];
 
     if (migration_is_setup_or_active(s->state)) {
         error_setg(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
 
-    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
+    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
+    if (!migrate_caps_check(cap_list, params, errp)) {
         return;
     }

Otherwise I'll get:

(qemu) migrate_set_capability postcopy-ram on                                  
(qemu) migrate_set_capability compress on                                      
Postcopy is not currently compatible with compression                          
(qemu) info migrate_capabilities       
xbzrle: off                            
rdma-pin-all: off                      
auto-converge: off                     
zero-blocks: off                       
compress: on         <------------------------- :(
events: off                            
postcopy-ram: on                       
x-colo: off                            
release-ram: off                       
block: off                             
return-path: off                       
pause-before-switchover: off           
x-multifd: off
 
And it looks very likely that this should solve the block bug too.

(So I think either I got a brain fart last July, or now...)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: Fix block failure cases

Posted by Dr. David Alan Gilbert 7 years, 7 months ago

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Feb 28, 2018 at 04:49:37PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > This fixes a couple of cases where the block migration capability
> > doesn't get cleared when a migration failed.
> > 
> > 1) When block migration is compiled out:
> >   (qemu) migrate -d -b  "exec:cat > /dev/null"
> >   QEMU compiled without old-style (blk/-b, inc/-i) block migration
> >   Use drive_mirror+NBD instead.
> >   (qemu) migrate_set_capability xbzrle off
> >   QEMU compiled without old-style (blk/-b, inc/-i) block migration
> >   Use drive_mirror+NBD instead.
> > 
> >   This corresponds to https://bugzilla.redhat.com/show_bug.cgi?id=1550022
> > 
> > 2) When a migration with a bad protocol is tried:
> >   (qemu) migrate -d -b "foo:bah"
> >   Parameter 'uri' expects a valid migration protocol
> >   (qemu) info migrate_capabilities
> >   xbzrle: off
> >   rdma-pin-all: off
> >   auto-converge: off
> >   zero-blocks: off
> >   compress: off
> >   events: off
> >   postcopy-ram: off
> >   x-colo: off
> >   release-ram: off
> >   block: on   <<<<<<-----
> >   return-path: off
> >   pause-before-switchover: off
> >   x-multifd: off
> > 
> > Fixes: 2833c59b947
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/migration.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b913b98803..da0e4a1f56 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1391,11 +1391,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >              return;
> >          }
> >          migrate_set_block_enabled(true, &local_err);
> > +        s->must_remove_block_options = true;
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> > +            block_cleanup_parameters(s);
> >              return;
> >          }
> > -        s->must_remove_block_options = true;
> >      }
> >  
> >      if (has_inc && inc) {
> > @@ -1417,11 +1418,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >      } else if (strstart(uri, "fd:", &p)) {
> >          fd_start_outgoing_migration(s, p, &local_err);
> >      } else {
> > -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
> > +        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
> >                     "a valid migration protocol");
> >          migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> >                            MIGRATION_STATUS_FAILED);
> > -        return;
> >      }
> >  
> >      if (local_err) {
> > -- 
> > 2.14.3
> > 
> 
> Ouch...
> 
> Instead it seems to be my fault in 4a84214ebe ("migration: provide
> migrate_caps_check()", 2017-07-18).  For now I cannot understand why I
> did that before since it's obviously strange if without this
> squashed...
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0aa596f867..88ed9375aa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>  {
>      MigrationState *s = migrate_get_current();
>      MigrationCapabilityStatusList *cap;
> +    bool cap_list[MIGRATION_CAPABILITY__MAX];
>  
>      if (migration_is_setup_or_active(s->state)) {
>          error_setg(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
>  
> -    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
> +    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
> +    if (!migrate_caps_check(cap_list, params, errp)) {
>          return;
>      }

> 
> Otherwise I'll get:
> 
> (qemu) migrate_set_capability postcopy-ram on                                  
> (qemu) migrate_set_capability compress on                                      
> Postcopy is not currently compatible with compression                          
> (qemu) info migrate_capabilities       
> xbzrle: off                            
> rdma-pin-all: off                      
> auto-converge: off                     
> zero-blocks: off                       
> compress: on         <------------------------- :(
> events: off                            
> postcopy-ram: on                       
> x-colo: off                            
> release-ram: off                       
> block: off                             
> return-path: off                       
> pause-before-switchover: off           
> x-multifd: off
>  
> And it looks very likely that this should solve the block bug too.

Yes, it looks like it should - but it doesn't solve the 2nd of my cases;
so we also need the 2nd half of my change.

> (So I think either I got a brain fart last July, or now...)

Of course it's the combination of two bugs :-)

Dave

> Thanks,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: Fix block failure cases

Posted by Peter Xu 7 years, 7 months ago

On Thu, Mar 01, 2018 at 03:32:19PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Wed, Feb 28, 2018 at 04:49:37PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > This fixes a couple of cases where the block migration capability
> > > doesn't get cleared when a migration failed.
> > > 
> > > 1) When block migration is compiled out:
> > >   (qemu) migrate -d -b  "exec:cat > /dev/null"
> > >   QEMU compiled without old-style (blk/-b, inc/-i) block migration
> > >   Use drive_mirror+NBD instead.
> > >   (qemu) migrate_set_capability xbzrle off
> > >   QEMU compiled without old-style (blk/-b, inc/-i) block migration
> > >   Use drive_mirror+NBD instead.
> > > 
> > >   This corresponds to https://bugzilla.redhat.com/show_bug.cgi?id=1550022
> > > 
> > > 2) When a migration with a bad protocol is tried:
> > >   (qemu) migrate -d -b "foo:bah"
> > >   Parameter 'uri' expects a valid migration protocol
> > >   (qemu) info migrate_capabilities
> > >   xbzrle: off
> > >   rdma-pin-all: off
> > >   auto-converge: off
> > >   zero-blocks: off
> > >   compress: off
> > >   events: off
> > >   postcopy-ram: off
> > >   x-colo: off
> > >   release-ram: off
> > >   block: on   <<<<<<-----
> > >   return-path: off
> > >   pause-before-switchover: off
> > >   x-multifd: off
> > > 
> > > Fixes: 2833c59b947
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  migration/migration.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index b913b98803..da0e4a1f56 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -1391,11 +1391,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > >              return;
> > >          }
> > >          migrate_set_block_enabled(true, &local_err);
> > > +        s->must_remove_block_options = true;
> > >          if (local_err) {
> > >              error_propagate(errp, local_err);
> > > +            block_cleanup_parameters(s);
> > >              return;
> > >          }
> > > -        s->must_remove_block_options = true;
> > >      }
> > >  
> > >      if (has_inc && inc) {
> > > @@ -1417,11 +1418,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> > >      } else if (strstart(uri, "fd:", &p)) {
> > >          fd_start_outgoing_migration(s, p, &local_err);
> > >      } else {
> > > -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
> > > +        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
> > >                     "a valid migration protocol");
> > >          migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > >                            MIGRATION_STATUS_FAILED);
> > > -        return;
> > >      }
> > >  
> > >      if (local_err) {
> > > -- 
> > > 2.14.3
> > > 
> > 
> > Ouch...
> > 
> > Instead it seems to be my fault in 4a84214ebe ("migration: provide
> > migrate_caps_check()", 2017-07-18).  For now I cannot understand why I
> > did that before since it's obviously strange if without this
> > squashed...
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 0aa596f867..88ed9375aa 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -747,13 +747,15 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> >  {
> >      MigrationState *s = migrate_get_current();
> >      MigrationCapabilityStatusList *cap;
> > +    bool cap_list[MIGRATION_CAPABILITY__MAX];
> >  
> >      if (migration_is_setup_or_active(s->state)) {
> >          error_setg(errp, QERR_MIGRATION_ACTIVE);
> >          return;
> >      }
> >  
> > -    if (!migrate_caps_check(s->enabled_capabilities, params, errp)) {
> > +    memcpy(cap_list, s->enabled_capabilities, sizeof(cap_list));
> > +    if (!migrate_caps_check(cap_list, params, errp)) {
> >          return;
> >      }
> 
> > 
> > Otherwise I'll get:
> > 
> > (qemu) migrate_set_capability postcopy-ram on                                  
> > (qemu) migrate_set_capability compress on                                      
> > Postcopy is not currently compatible with compression                          
> > (qemu) info migrate_capabilities       
> > xbzrle: off                            
> > rdma-pin-all: off                      
> > auto-converge: off                     
> > zero-blocks: off                       
> > compress: on         <------------------------- :(
> > events: off                            
> > postcopy-ram: on                       
> > x-colo: off                            
> > release-ram: off                       
> > block: off                             
> > return-path: off                       
> > pause-before-switchover: off           
> > x-multifd: off
> >  
> > And it looks very likely that this should solve the block bug too.
> 
> Yes, it looks like it should - but it doesn't solve the 2nd of my cases;
> so we also need the 2nd half of my change.
> 
> > (So I think either I got a brain fart last July, or now...)
> 
> Of course it's the combination of two bugs :-)

Ah sure. :)

Please just let me know if you want me to post a patch, or I'll assume
you'll handle this together in the next post (and thanks anyways :)

-- 
Peter Xu