migration: avoid starting a new migration task while the previous one still exist

Avoid starting a new migration task while the previous one still exist.

Signed-off-by: Zeng Junliang <zengjunliang@huawei.com>
---
 migration.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

Il 04/11/2013 12:26, Zhanghaoyu (A) ha scritto:
> Avoid starting a new migration task while the previous one still exist.

Can you explain how to reproduce the problem?

Also please use pbonzini@redhat.com instead.  My Gmail address is an
implementation detail. :)

> Signed-off-by: Zeng Junliang <zengjunliang@huawei.com>

It looks like the author of the patch is not the same as you.  If so,
you need to make Zeng Junliang the author (using --author on the "git
commit" command line) and add your own signoff line.

Paolo

> ---
>  migration.c |   34 ++++++++++++++++++++++------------
>  1 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 2b1ab20..ab4c439 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -40,8 +40,10 @@ enum {
>      MIG_STATE_ERROR = -1,
>      MIG_STATE_NONE,
>      MIG_STATE_SETUP,
> +    MIG_STATE_CANCELLING,
>      MIG_STATE_CANCELLED,
>      MIG_STATE_ACTIVE,
> +    MIG_STATE_COMPLETING,
>      MIG_STATE_COMPLETED,
>  };
>  
> @@ -196,6 +198,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_total_time = false;
>          break;
>      case MIG_STATE_ACTIVE:
> +    case MIG_STATE_CANCELLING:
> +    case MIG_STATE_COMPLETING:
>          info->has_status = true;
>          info->status = g_strdup("active");
>          info->has_total_time = true;
> @@ -282,6 +286,13 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>  
>  /* shared migration helpers */
>  
> +static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> +{
> +    if (atomic_cmpxchg(&s->state, old_state, new_state) == new_state) {
> +        trace_migrate_set_state(new_state);
> +    }
> +}
> +
>  static void migrate_fd_cleanup(void *opaque)
>  {
>      MigrationState *s = opaque;
> @@ -301,20 +312,18 @@ static void migrate_fd_cleanup(void *opaque)
>  
>      assert(s->state != MIG_STATE_ACTIVE);
>  
> -    if (s->state != MIG_STATE_COMPLETED) {
> +    if (s->state != MIG_STATE_COMPLETING) {
>          qemu_savevm_state_cancel();
> +        if (s->state == MIG_STATE_CANCELLING) {
> +            migrate_set_state(s, MIG_STATE_CANCELLING, MIG_STATE_CANCELLED); 
> +        }
> +    }else {
> +        migrate_set_state(s, MIG_STATE_COMPLETING, MIG_STATE_COMPLETED); 
>      }
>  
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
>  
> -static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> -{
> -    if (atomic_cmpxchg(&s->state, old_state, new_state) == new_state) {
> -        trace_migrate_set_state(new_state);
> -    }
> -}
> -
>  void migrate_fd_error(MigrationState *s)
>  {
>      DPRINTF("setting error state\n");
> @@ -328,7 +337,7 @@ static void migrate_fd_cancel(MigrationState *s)
>  {
>      DPRINTF("cancelling migration\n");
>  
> -    migrate_set_state(s, s->state, MIG_STATE_CANCELLED);
> +    migrate_set_state(s, s->state, MIG_STATE_CANCELLING);
>  }
>  
>  void add_migration_state_change_notifier(Notifier *notify)
> @@ -405,7 +414,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      params.blk = has_blk && blk;
>      params.shared = has_inc && inc;
>  
> -    if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) {
> +    if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP ||
> +        s->state == MIG_STATE_COMPLETING || s->state == MIG_STATE_CANCELLING) {
>          error_set(errp, QERR_MIGRATION_ACTIVE);
>          return;
>      }
> @@ -594,7 +604,7 @@ static void *migration_thread(void *opaque)
>                  }
>  
>                  if (!qemu_file_get_error(s->file)) {
> -                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
> +                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETING);
>                      break;
>                  }
>              }
> @@ -634,7 +644,7 @@ static void *migration_thread(void *opaque)
>      }
>  
>      qemu_mutex_lock_iothread();
> -    if (s->state == MIG_STATE_COMPLETED) {
> +    if (s->state == MIG_STATE_COMPLETING) {
>          int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>          s->total_time = end_time - s->total_time;
>          s->downtime = end_time - start_time;
>

On 11/04/2013 04:26 AM, Zhanghaoyu (A) wrote:
> Avoid starting a new migration task while the previous one still exist.
> 
> Signed-off-by: Zeng Junliang <zengjunliang@huawei.com>
> ---

It's best to put comments here...

>  migration.c |   34 ++++++++++++++++++++++------------
>  1 files changed, 22 insertions(+), 12 deletions(-)
...

>          s->downtime = end_time - start_time;
> -- 1.7.3.1.msysgit.0 BTW, while error happened during migration, need
> the "erroring" state to avoid starting a new migration task while
> current migration task still exist? And, do the new added migration
> states need to be reported to libvirt?

...rather than here, since most mail clients will strip anything after
the '-- ' separator output by git as if it were the signature, making it
very difficult to reply to (as you can see, my mailer corrupted the
formatting of your question due to how I forced it to reply to your
signature).

As to whether libvirt needs to know about the state, I'm not sure.
Libvirt already has its own code to prevent concurrent attempts at
migration, so theoretically libvirt should never trip the situation that
you appear to be coding for.  Perhaps providing more details in the
commit message about how to actually get into the situation will make it
easier to evaluate.