Re: [Qemu-devel] [Qemu-devel [RFC] [WIP] v1] Keeping the Source side alive incase of network failure (Migration recover from network failure)

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Md Haris Iqbal (haris.phnx@gmail.com) wrote:

Remember to add a more detailed comment about what the patch is doing.
(And possibly split it up a bit more)

> ---
>  include/migration/migration.h |  1 +
>  migration/migration.c         | 41 ++++++++++++++++++++++++++++++++++++-----
>  vl.c                          |  4 ++++
>  3 files changed, 41 insertions(+), 5 deletions(-)

> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ac2c12c..33da695 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -325,6 +325,7 @@ void global_state_store_running(void);
>  void flush_page_queue(MigrationState *ms);
>  int ram_save_queue_pages(MigrationState *ms, const char *rbname,
>                           ram_addr_t start, ram_addr_t len);
> +int qemu_migrate_postcopy_outgoing_recovery(MigrationState *ms);
>  
>  PostcopyState postcopy_state_get(void);
>  /* Set the state and return the old state */
> diff --git a/migration/migration.c b/migration/migration.c
> index 991313a..ee0c2a8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -539,6 +539,7 @@ static bool migration_is_setup_or_active(int state)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_SETUP:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVERY:
>          return true;
>  
>      default:
> @@ -1634,6 +1635,8 @@ static void *migration_thread(void *opaque)
>      /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
>      enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
>  
> +    int32_t ret;

The return type of qemu_file_get_error is int not int32_t.

>      rcu_register_thread();
>  
>      qemu_savevm_state_header(s->to_dst_file);
> @@ -1700,11 +1703,26 @@ static void *migration_thread(void *opaque)
>              }
>          }
>  
> -        if (qemu_file_get_error(s->to_dst_file)) {
> -            migrate_set_state(&s->state, current_active_state,
> -                              MIGRATION_STATUS_FAILED);
> -            trace_migration_thread_file_err();
> -            break;
> +        if ((ret = qemu_file_get_error(s->to_dst_file))) {
> +            fprintf(stderr, "1 : Error %s %d\n", strerror(-ret), -ret);

Remember to clean those fprintf's out at the end; although it can be useful
to leave some trace_ calls in; so for example modify
the trace_migrate_thread_file_err to take an error number.

> +            if(ret != -EIO && s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> +                /* Network Failure during postcopy */

That probably needs commenting as to why it's not -EIO - since that's probably
what people might expect from a network error;  we'll have to keep an eye out
to see if that's realyl a reliable check.
(Also qemu normally puts a space after the 'if')

> +                current_active_state = MIGRATION_STATUS_POSTCOPY_RECOVERY;

That probably needs a migrate_set_state .

> +                runstate_set(RUN_STATE_POSTMIGRATE_RECOVERY);
> +                fprintf(stderr, "1.1 : Error %s %d\n", strerror(-ret), -ret);
> +                ret = qemu_migrate_postcopy_outgoing_recovery(s);
> +                if(ret < 0) {
> +                    break;
> +                }
> +
> +            } else {
> +                migrate_set_state(&s->state, current_active_state,
> +                                             MIGRATION_STATUS_FAILED);
> +                fprintf(stderr, "1.2 : Error %s %d\n", strerror(-ret), -ret);
> +                trace_migration_thread_file_err();
> +                break;
> +            }
>          }
>          current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>          if (current_time >= initial_time + BUFFER_DELAY) {
> @@ -1797,6 +1815,19 @@ void migrate_fd_connect(MigrationState *s)
>      s->migration_thread_running = true;
>  }
>  
> +int qemu_migrate_postcopy_outgoing_recovery(MigrationState* ms)
> +{
> +    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> +                                  MIGRATION_STATUS_POSTCOPY_RECOVERY);
> +
> +    /* Code for network recovery to be added here */
> +    while(1) {
> +        fprintf(stderr, "Not letting it fail\n");
> +        sleep(2);
> +    }
> +
> +}
> +
>  PostcopyState  postcopy_state_get(void)
>  {
>      return atomic_mb_read(&incoming_postcopy_state);
> diff --git a/vl.c b/vl.c
> index 5fd22cb..c237140 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -618,6 +618,10 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
> +    { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE_RECOVERY },
> +
> +    { RUN_STATE_POSTMIGRATE_RECOVERY, RUN_STATE_FINISH_MIGRATE },
> +    { RUN_STATE_POSTMIGRATE_RECOVERY, RUN_STATE_SHUTDOWN },
>  
>      { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
>      { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
> -- 
> 2.7.4

Try to keep your patches together in a patch series; I find
  git format-patch -n --cover-letter and then the range of patches to generate
produces the nices result and then use git send-email.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK