Re: [Qemu-devel] [PATCH RFC v6 5/7] migration: fix the multifd code when receiving less channels

[Fei Li]

On 10/30/2018 02:05 PM, Peter Xu wrote:
> On Mon, Oct 29, 2018 at 08:58:16PM +0800, Fei Li wrote:
> > In our current code, when multifd is used during migration, if there
> > is an error before the destination receives all new channels, the
> > source keeps running, however the destination does not exit but keeps
> > waiting until the source is killed deliberately.
> >
> > Fix this by simply killing the destination when it fails to receive
> > packet via some channel.
> >
> > Cc: Dr. David Alan Gilbert <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > Signed-off-by: Fei Li <address@hidden>
> > ---
> >   migration/channel.c   |  7 ++++++-
> >   migration/migration.c |  9 +++++++--
> >   migration/migration.h |  2 +-
> >   migration/ram.c       | 17 ++++++++++++++---
> >   migration/ram.h       |  2 +-
> >   5 files changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 33e0e9b82f..572be4245a 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -44,7 +44,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
> >               error_report_err(local_err);
> [1]
>
> >           }
> >       } else {
> > -        migration_ioc_process_incoming(ioc);
> > +        Error *local_err = NULL;
> > +        migration_ioc_process_incoming(ioc, &local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            exit(EXIT_FAILURE);
> I would still suggest that you don't quit.  See TLS error at [1], it
> only dumps the error.  IMHO users can quit easily for dst vm, I'll
> just let them decide if they want.
>
> Then you can merge the error path for both.
Ok, got it, thanks :)
> > +        }
> >       }
> >   }
> >   
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 8b36e7f184..87dfc7374f 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -541,7 +541,7 @@ void migration_fd_process_incoming(QEMUFile *f)
> >       migration_incoming_process();
> >   }
> >   
> > -void migration_ioc_process_incoming(QIOChannel *ioc)
> > +void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> >   {
> >       MigrationIncomingState *mis = migration_incoming_get_current();
> >       bool start_migration;
> > @@ -563,9 +563,14 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
> >            */
> >           start_migration = !migrate_use_multifd();
> >       } else {
> > +        Error *local_err = NULL;
> >           /* Multiple connections */
> >           assert(migrate_use_multifd());
> > -        start_migration = multifd_recv_new_channel(ioc);
> > +        start_migration = multifd_recv_new_channel(ioc, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> >       }
> >   
> >       if (start_migration) {
> > diff --git a/migration/migration.h b/migration/migration.h
> > index f7813f8261..7df4d426d0 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -229,7 +229,7 @@ struct MigrationState
> >   void migrate_set_state(int *state, int old_state, int new_state);
> >   
> >   void migration_fd_process_incoming(QEMUFile *f);
> > -void migration_ioc_process_incoming(QIOChannel *ioc);
> > +void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
> >   void migration_incoming_process(void);
> >   
> >   bool  migration_has_all_channels(void);
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 4db3b3e8f4..8f03afe228 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1072,6 +1072,7 @@ out:
> >   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
> >   {
> >       MultiFDSendParams *p = opaque;
> > +    MigrationState *s = migrate_get_current();
> This seems to be the source part, then I'll suggest you split the
> patch and keep this patch only touches the dest vm path.
ok
> >       QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
> >       Error *local_err = NULL;
> >   
> > @@ -1080,6 +1081,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
> >       }
> >   
> >       if (qio_task_propagate_error(task, &local_err)) {
> > +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> >           if (multifd_save_cleanup(&local_err) != 0) {
> >               migrate_set_error(migrate_get_current(), local_err);
> >           }
> > @@ -1337,16 +1339,20 @@ bool multifd_recv_all_channels_created(void)
> >   }
> >   
> >   /* Return true if multifd is ready for the migration, otherwise false */
> > -bool multifd_recv_new_channel(QIOChannel *ioc)
> > +bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
> >   {
> > +    MigrationIncomingState *mis = migration_incoming_get_current();
> >       MultiFDRecvParams *p;
> >       Error *local_err = NULL;
> >       int id;
> >   
> >       id = multifd_recv_initial_packet(ioc, &local_err);
> >       if (id < 0) {
> > +        error_propagate_prepend(errp, local_err,
> > +                        "failed to receive packet via multifd channel %x: ",
> > +                        multifd_recv_state->count);
> >           multifd_recv_terminate_threads(local_err, false);
> > -        return false;
> > +        goto fail;
> >       }
> >   
> >       p = &multifd_recv_state->params[id];
> > @@ -1354,7 +1360,8 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
> >           error_setg(&local_err, "multifd: received id '%d' already setup'",
> >                      id);
> >           multifd_recv_terminate_threads(local_err, true);
> > -        return false;
> > +        error_propagate(errp, local_err);
> > +        goto fail;
> >       }
> >       p->c = ioc;
> >       object_ref(OBJECT(ioc));
> > @@ -1366,6 +1373,10 @@ bool multifd_recv_new_channel(QIOChannel *ioc)
> >                          QEMU_THREAD_JOINABLE);
> >       atomic_inc(&multifd_recv_state->count);
> >       return multifd_recv_state->count == migrate_multifd_channels();
> > +fail:
> > +    qemu_fclose(mis->from_src_file);
> > +    mis->from_src_file = NULL;
> > +    return false;
> Do we need this?
>
> I'd suggest to put all cleanups into a single function.  For dest vm
> I say it's process_incoming_migration_bh.
>
> Regards,
Not sure whether I understand correctly, if multifd_recv_new_channel() 
fails,
that means migration_incoming_process() will not be called, then
process_incoming_migration_co() and process_incoming_migration_bh()
will not be called either. In that way, there is no cleanup.

Have a nice day, thanks
Fei