[PATCH 0/3] Some minor fixes for migration states

[PATCH 0/3] Some minor fixes for migration states

[Zhang Chen]

This series solved some fixme and comments in code.
Please see the details in each patch commit message.

Zhang Chen (3):
  migration/migration.c: Add missed default error handler for migration
    state
  migration/migration.c: Avoid COLO boot in postcopy migration
  migration/migration.c: Remove the MIGRATION_STATUS_ACTIVE when
    migration finished

 migration/migration.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/3] migration/migration.c: Add missed default error handler for migration state

[Zhang Chen]

In the migration_completion() no other status is expected, for
example MIGRATION_STATUS_CANCELLING, MIGRATION_STATUS_CANCELLED, etc.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0652165610..2afa77da03 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3205,7 +3205,7 @@ static void migration_completion(MigrationState *s)
         qemu_mutex_unlock_iothread();
 
         trace_migration_completion_postcopy_end_after_complete();
-    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
+    } else {
         goto fail;
     }
 
-- 
2.25.1

Re: [PATCH 1/3] migration/migration.c: Add missed default error handler for migration state

[Dr. David Alan Gilbert]

* Zhang Chen (chen.zhang@intel.com) wrote:
> In the migration_completion() no other status is expected, for
> example MIGRATION_STATUS_CANCELLING, MIGRATION_STATUS_CANCELLED, etc.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>

I think you're right;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

 however, did you actually see this trigger in a different state?

Dave
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0652165610..2afa77da03 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3205,7 +3205,7 @@ static void migration_completion(MigrationState *s)
>          qemu_mutex_unlock_iothread();
>  
>          trace_migration_completion_postcopy_end_after_complete();
> -    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
> +    } else {
>          goto fail;
>      }
>  
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH 1/3] migration/migration.c: Add missed default error handler for migration state

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Thursday, January 20, 2022 1:52 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Juan Quintela <quintela@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>
> Subject: Re: [PATCH 1/3] migration/migration.c: Add missed default error
> handler for migration state
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > In the migration_completion() no other status is expected, for example
> > MIGRATION_STATUS_CANCELLING, MIGRATION_STATUS_CANCELLED, etc.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> 
> I think you're right;
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
>  however, did you actually see this trigger in a different state?

No, I just read the code and found it.

Thanks
Chen

> 
> Dave
> > ---
> >  migration/migration.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c index
> > 0652165610..2afa77da03 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3205,7 +3205,7 @@ static void migration_completion(MigrationState
> *s)
> >          qemu_mutex_unlock_iothread();
> >
> >          trace_migration_completion_postcopy_end_after_complete();
> > -    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
> > +    } else {
> >          goto fail;
> >      }
> >
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/3] migration/migration.c: Add missed default error handler for migration state

[Juan Quintela]

Zhang Chen <chen.zhang@intel.com> wrote:
> In the migration_completion() no other status is expected, for
> example MIGRATION_STATUS_CANCELLING, MIGRATION_STATUS_CANCELLED, etc.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued

[PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy migration

[Zhang Chen]

COLO dose not support postcopy migration and remove the Fixme.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/migration.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2afa77da03..3fac9c67ca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3230,7 +3230,11 @@ static void migration_completion(MigrationState *s)
         goto fail_invalidate;
     }
 
-    if (!migrate_colo_enabled()) {
+    if (migrate_colo_enabled() && s->state == MIGRATION_STATUS_ACTIVE) {
+        /* COLO dose not support postcopy */
+        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_COLO);
+    } else {
         migrate_set_state(&s->state, current_active_state,
                           MIGRATION_STATUS_COMPLETED);
     }
@@ -3621,10 +3625,6 @@ static void migration_iteration_finish(MigrationState *s)
                          "COLO enabled", __func__);
         }
         migrate_start_colo_process(s);
-        /*
-         * Fixme: we will run VM in COLO no matter its old running state.
-         * After exited COLO, we will keep running.
-         */
          /* Fallthrough */
     case MIGRATION_STATUS_ACTIVE:
         /*
-- 
2.25.1

Re: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy migration

[Dr. David Alan Gilbert]

* Zhang Chen (chen.zhang@intel.com) wrote:
> COLO dose not support postcopy migration and remove the Fixme.


'does' not 'dose'

> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  migration/migration.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 2afa77da03..3fac9c67ca 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3230,7 +3230,11 @@ static void migration_completion(MigrationState *s)
>          goto fail_invalidate;
>      }
>  
> -    if (!migrate_colo_enabled()) {
> +    if (migrate_colo_enabled() && s->state == MIGRATION_STATUS_ACTIVE) {
> +        /* COLO dose not support postcopy */
> +        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> +                          MIGRATION_STATUS_COLO);

I'm a bit confused; where were we setting the source state to COLO
before - I can't find it!

Dave

> +    } else {
>          migrate_set_state(&s->state, current_active_state,
>                            MIGRATION_STATUS_COMPLETED);
>      }
> @@ -3621,10 +3625,6 @@ static void migration_iteration_finish(MigrationState *s)
>                           "COLO enabled", __func__);
>          }
>          migrate_start_colo_process(s);
> -        /*
> -         * Fixme: we will run VM in COLO no matter its old running state.
> -         * After exited COLO, we will keep running.
> -         */
>           /* Fallthrough */
>      case MIGRATION_STATUS_ACTIVE:
>          /*
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy migration

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Thursday, January 20, 2022 3:41 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Juan Quintela <quintela@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>
> Subject: Re: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy
> migration
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > COLO dose not support postcopy migration and remove the Fixme.
> 
> 
> 'does' not 'dose'
> 

Yes, typo.

> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  migration/migration.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c index
> > 2afa77da03..3fac9c67ca 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -3230,7 +3230,11 @@ static void migration_completion(MigrationState
> *s)
> >          goto fail_invalidate;
> >      }
> >
> > -    if (!migrate_colo_enabled()) {
> > +    if (migrate_colo_enabled() && s->state ==
> MIGRATION_STATUS_ACTIVE) {
> > +        /* COLO dose not support postcopy */
> > +        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> > +                          MIGRATION_STATUS_COLO);
> 
> I'm a bit confused; where were we setting the source state to COLO before -
> I can't find it!

Yes, please read this mail:
https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03525.html

After that I found I missed something, so this patch disabled postcopy migration and try to fix it.

Thanks
Chen

> 
> Dave
> 
> > +    } else {
> >          migrate_set_state(&s->state, current_active_state,
> >                            MIGRATION_STATUS_COMPLETED);
> >      }
> > @@ -3621,10 +3625,6 @@ static void
> migration_iteration_finish(MigrationState *s)
> >                           "COLO enabled", __func__);
> >          }
> >          migrate_start_colo_process(s);
> > -        /*
> > -         * Fixme: we will run VM in COLO no matter its old running state.
> > -         * After exited COLO, we will keep running.
> > -         */
> >           /* Fallthrough */
> >      case MIGRATION_STATUS_ACTIVE:
> >          /*
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy migration

[Juan Quintela]

Zhang Chen <chen.zhang@intel.com> wrote:
> COLO dose not support postcopy migration and remove the Fixme.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

fixed the typo.

RE: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy migration

[Zhang, Chen]

> -----Original Message-----
> From: Juan Quintela <quintela@redhat.com>
> Sent: Thursday, January 27, 2022 3:30 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>
> Subject: Re: [PATCH 2/3] migration/migration.c: Avoid COLO boot in postcopy
> migration
> 
> Zhang Chen <chen.zhang@intel.com> wrote:
> > COLO dose not support postcopy migration and remove the Fixme.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> fixed the typo.

OK, already updated V2 for this series.

Thanks
Chen

[PATCH 3/3] migration/migration.c: Remove the MIGRATION_STATUS_ACTIVE when migration finished

[Zhang Chen]

The MIGRATION_STATUS_ACTIVE indicates that migration is running.
Remove it to be handled by the default operation,
It should be part of the unknown ending states.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/migration.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3fac9c67ca..21e1498f46 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3625,12 +3625,6 @@ static void migration_iteration_finish(MigrationState *s)
                          "COLO enabled", __func__);
         }
         migrate_start_colo_process(s);
-         /* Fallthrough */
-    case MIGRATION_STATUS_ACTIVE:
-        /*
-         * We should really assert here, but since it's during
-         * migration, let's try to reduce the usage of assertions.
-         */
         s->vm_was_running = true;
         /* Fallthrough */
     case MIGRATION_STATUS_FAILED:
-- 
2.25.1

Re: [PATCH 3/3] migration/migration.c: Remove the MIGRATION_STATUS_ACTIVE when migration finished

[Juan Quintela]

Zhang Chen <chen.zhang@intel.com> wrote:
> The MIGRATION_STATUS_ACTIVE indicates that migration is running.
> Remove it to be handled by the default operation,
> It should be part of the unknown ending states.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued

RE: [PATCH 0/3] Some minor fixes for migration states

[Zhang, Chen]

Any comments? Ping...

Thanks
Chen

> -----Original Message-----
> From: Zhang, Chen <chen.zhang@intel.com>
> Sent: Friday, December 31, 2021 2:00 PM
> To: Dr. David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela
> <quintela@redhat.com>; qemu-dev <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>
> Subject: [PATCH 0/3] Some minor fixes for migration states
> 
> This series solved some fixme and comments in code.
> Please see the details in each patch commit message.
> 
> Zhang Chen (3):
>   migration/migration.c: Add missed default error handler for migration
>     state
>   migration/migration.c: Avoid COLO boot in postcopy migration
>   migration/migration.c: Remove the MIGRATION_STATUS_ACTIVE when
>     migration finished
> 
>  migration/migration.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> --
> 2.25.1