Re: [PATCH 13/20] migration: Move channel setup out of postcopy_try_recover()

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

* Peter Xu (peterx@redhat.com) wrote:
> We used to use postcopy_try_recover() to replace migration_incoming_setup() to
> setup incoming channels.  That's fine for the old world, but in the new world
> there can be more than one channels that need setup.  Better move the channel
> setup out of it so that postcopy_try_recover() only handles the last phase of
> switching to the recovery phase.
> 
> To do that in migration_fd_process_incoming(), move the postcopy_try_recover()
> call to be after migration_incoming_setup(), which will setup the channels.
> While in migration_ioc_process_incoming(), postpone the recover() routine right
> before we'll jump into migration_incoming_process().
> 
> A side benefit is we don't need to pass in QEMUFile* to postcopy_try_recover()
> anymore.  Remove it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

OK, but note one question below:

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

> ---
>  migration/migration.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 67520d3105..b2e6446457 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -665,19 +665,20 @@ void migration_incoming_process(void)
>  }
>  
>  /* Returns true if recovered from a paused migration, otherwise false */
> -static bool postcopy_try_recover(QEMUFile *f)
> +static bool postcopy_try_recover(void)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      if (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>          /* Resumed from a paused postcopy migration */
>  
> -        mis->from_src_file = f;
> +        /* This should be set already in migration_incoming_setup() */
> +        assert(mis->from_src_file);
>          /* Postcopy has standalone thread to do vm load */
> -        qemu_file_set_blocking(f, true);
> +        qemu_file_set_blocking(mis->from_src_file, true);

Does that set_blocking happen on the 2nd channel somewhere?

Dave

>  
>          /* Re-configure the return path */
> -        mis->to_src_file = qemu_file_get_return_path(f);
> +        mis->to_src_file = qemu_file_get_return_path(mis->from_src_file);
>  
>          migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
>                            MIGRATION_STATUS_POSTCOPY_RECOVER);
> @@ -698,11 +699,10 @@ static bool postcopy_try_recover(QEMUFile *f)
>  
>  void migration_fd_process_incoming(QEMUFile *f, Error **errp)
>  {
> -    if (postcopy_try_recover(f)) {
> +    if (!migration_incoming_setup(f, errp)) {
>          return;
>      }
> -
> -    if (!migration_incoming_setup(f, errp)) {
> +    if (postcopy_try_recover()) {
>          return;
>      }
>      migration_incoming_process();
> @@ -718,11 +718,6 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>          /* The first connection (multifd may have multiple) */
>          QEMUFile *f = qemu_fopen_channel_input(ioc);
>  
> -        /* If it's a recovery, we're done */
> -        if (postcopy_try_recover(f)) {
> -            return;
> -        }
> -
>          if (!migration_incoming_setup(f, errp)) {
>              return;
>          }
> @@ -743,6 +738,10 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>      }
>  
>      if (start_migration) {
> +        /* If it's a recovery, we're done */
> +        if (postcopy_try_recover()) {
> +            return;
> +        }
>          migration_incoming_process();
>      }
>  }
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK