migration: handle CANCELLING state in migration_completion()

[PATCH] migration: handle CANCELLING state in migration_completion()

Posted by Longpeng(Mike) 5 years ago

The following sequence may cause the VM abort during migration:

1. RUN_STATE_RUNNING,MIGRATION_STATUS_ACTIVE

2. before call migration_completion(), we send migrate_cancel
   QMP command, the state machine is changed to:
     RUN_STATE_RUNNING,MIGRATION_STATUS_CANCELLING

3. call migration_completion(), and the state machine is
   switch to: RUN_STATE_RUNNING,MIGRATION_STATUS_COMPLETED

4. call migration_iteration_finish(), because the migration
   status is COMPLETED, so it will try to set the runstate
   to POSTMIGRATE, but RUNNING-->POSTMIGRATE is an invalid
   transition, so abort().

The migration_completion() should not change the migration state
to COMPLETED if it is already changed to CANCELLING.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 migration/migration.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 3263aa5..b11a2bd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3061,6 +3061,8 @@ static void migration_completion(MigrationState *s)
 
         qemu_savevm_state_complete_postcopy(s->to_dst_file);
         trace_migration_completion_postcopy_end_after_complete();
+    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
+        goto fail;
     }
 
     /*
-- 
1.8.3.1

Re: [PATCH] migration: handle CANCELLING state in migration_completion()

Posted by Dr. David Alan Gilbert 5 years ago

* Longpeng(Mike) (longpeng2@huawei.com) wrote:
> The following sequence may cause the VM abort during migration:
> 
> 1. RUN_STATE_RUNNING,MIGRATION_STATUS_ACTIVE
> 
> 2. before call migration_completion(), we send migrate_cancel
>    QMP command, the state machine is changed to:
>      RUN_STATE_RUNNING,MIGRATION_STATUS_CANCELLING
> 
> 3. call migration_completion(), and the state machine is
>    switch to: RUN_STATE_RUNNING,MIGRATION_STATUS_COMPLETED
> 
> 4. call migration_iteration_finish(), because the migration
>    status is COMPLETED, so it will try to set the runstate
>    to POSTMIGRATE, but RUNNING-->POSTMIGRATE is an invalid
>    transition, so abort().
> 
> The migration_completion() should not change the migration state
> to COMPLETED if it is already changed to CANCELLING.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>

Queued

> ---
>  migration/migration.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa5..b11a2bd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3061,6 +3061,8 @@ static void migration_completion(MigrationState *s)
>  
>          qemu_savevm_state_complete_postcopy(s->to_dst_file);
>          trace_migration_completion_postcopy_end_after_complete();
> +    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
> +        goto fail;
>      }
>  
>      /*
> -- 
> 1.8.3.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] migration: handle CANCELLING state in migration_completion()

Posted by Dr. David Alan Gilbert 5 years ago

* Longpeng(Mike) (longpeng2@huawei.com) wrote:
> The following sequence may cause the VM abort during migration:
> 
> 1. RUN_STATE_RUNNING,MIGRATION_STATUS_ACTIVE
> 
> 2. before call migration_completion(), we send migrate_cancel
>    QMP command, the state machine is changed to:
>      RUN_STATE_RUNNING,MIGRATION_STATUS_CANCELLING
> 
> 3. call migration_completion(), and the state machine is
>    switch to: RUN_STATE_RUNNING,MIGRATION_STATUS_COMPLETED
> 
> 4. call migration_iteration_finish(), because the migration
>    status is COMPLETED, so it will try to set the runstate
>    to POSTMIGRATE, but RUNNING-->POSTMIGRATE is an invalid
>    transition, so abort().
> 
> The migration_completion() should not change the migration state
> to COMPLETED if it is already changed to CANCELLING.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>

Yes I think so; the only downside I see is I think this ends up going
from CACELLNG->FAILED.


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

> ---
>  migration/migration.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3263aa5..b11a2bd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3061,6 +3061,8 @@ static void migration_completion(MigrationState *s)
>  
>          qemu_savevm_state_complete_postcopy(s->to_dst_file);
>          trace_migration_completion_postcopy_end_after_complete();
> +    } else if (s->state == MIGRATION_STATUS_CANCELLING) {
> +        goto fail;
>      }
>  
>      /*
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK