Re: [PATCH v2 1/3] migration: Rework multi-channel checks on URI

[Peter Xu]

On Wed, Feb 08, 2023 at 08:19:11PM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > The whole idea of multi-channel checks was not properly done, IMHO.
> >
> > Currently we check multi-channel in a lot of places, but actually that's
> > not needed because we only need to check it right after we get the URI and
> > that should be it.
> >
> > If the URI check succeeded, we should never need to check it again because
> > we must have it.  If it check fails, we should fail immediately on either
> > the qmp_migrate or qmp_migrate_incoming, instead of failingg it later after
> > the connection established.
> >
> > Neither should we fail any set capabiliities like what we used to do here:
> >
> > 5ad15e8614 ("migration: allow enabling mutilfd for specific protocol only", 
> > 2021-10-19)
> >
> > Because logically the URI will only be set later after the capability is
> > set, so it doesn't make a lot of sense to check the URI type when setting
> > the capability, because we're checking the cap with an old URI passed in,
> > and that may not even be the URI we're going to use later.
> >
> > This patch mostly reverted all such checks for before, dropping the
> > variable migrate_allow_multi_channels and helpers.  Instead, add a common
> > helper to check URI for multi-channels for either qmp_migrate and
> > qmp_migrate_incoming and that should do all the proper checks.  The failure
> > will only trigger with the "migrate" or "migrate_incoming" command, or when
> > user specified "-incoming xxx" where "xxx" is not "defer".
> >
> > With that, make postcopy_preempt_setup() as simple as creating the channel.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> The idea is right.  But I think that changing everything in a single
> patch is confusing.
> 
> > ---
> >  migration/migration.c    | 56 +++++++++++++++++++---------------------
> >  migration/migration.h    |  3 ---
> >  migration/multifd.c      | 12 ++-------
> >  migration/postcopy-ram.c | 14 +---------
> >  migration/postcopy-ram.h |  2 +-
> >  5 files changed, 31 insertions(+), 56 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f4f7d207f0..ef7fceb5d7 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -182,16 +182,26 @@ static int migration_maybe_pause(MigrationState *s,
> >                                   int new_state);
> >  static void migrate_fd_cancel(MigrationState *s);
> >  
> > -static bool migrate_allow_multi_channels = true;
> > +static bool migration_needs_multiple_sockets(void)
> > +{
> > +    return migrate_use_multifd() || migrate_postcopy_preempt();
> > +}
> 
> This function (and use it) makes sense
> 
> > -void migrate_protocol_allow_multi_channels(bool allow)
> > +static bool uri_supports_multi_channels(const char *uri)
> >  {
> > -    migrate_allow_multi_channels = allow;
> > +    return strstart(uri, "tcp:", NULL) || strstart(uri, "unix:", NULL) ||
> > +        strstart(uri, "vsock:", NULL);
> 
> Indentation is wrong.  Fixing it by hand.

Will do.

> 
> This other is also ok with me.
> 
> >  }
> >  
> > -bool migrate_multi_channels_is_allowed(void)
> > +static bool migration_uri_validate(const char *uri, Error **errp)
> >  {
> > -    return migrate_allow_multi_channels;
> > +    if (migration_needs_multiple_sockets() &&
> > +        !uri_supports_multi_channels(uri)) {
> > +        error_setg(errp, "Migration requires multi-channel URIs (e.g. 
> > tcp)");
> > +        return false;
> > +    }
> > +
> > +    return true;
> >  }
> 
> This name is not O:-)
> 
> What about:
> 
> migration_channels_and_uri_compatible()
> 
> No, it is not perfect, but I can think anything else.
> 
> But validate don't mean anything.  I can't know without looking at the
> function  what is the meaning of the result.

I don't have an obvious preference; I think it means I'll just go ahead and
rename it. :)

> 
> >  static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
> > @@ -491,12 +501,15 @@ static void qemu_start_incoming_migration(const char 
> > *uri, Error **errp)
> >  {
> >      const char *p = NULL;
> >  
> > -    migrate_protocol_allow_multi_channels(false); /* reset it anyway */
> > +    /* URI is not suitable for migration? */
> > +    if (!migration_uri_validate(uri, errp)) {
> > +        return;
> > +    }
> > +
> >      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> >      if (strstart(uri, "tcp:", &p) ||
> >          strstart(uri, "unix:", NULL) ||
> >          strstart(uri, "vsock:", NULL)) {
> > -        migrate_protocol_allow_multi_channels(true);
> >          socket_start_incoming_migration(p ? p : uri, errp);
> >  #ifdef CONFIG_RDMA
> >      } else if (strstart(uri, "rdma:", &p)) {
> > @@ -721,11 +734,6 @@ void migration_fd_process_incoming(QEMUFile *f, Error 
> > **errp)
> >      migration_incoming_process();
> >  }
> >  
> > -static bool migration_needs_multiple_sockets(void)
> > -{
> > -    return migrate_use_multifd() || migrate_postcopy_preempt();
> > -}
> > -
> >  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> >  {
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > @@ -1347,15 +1355,6 @@ static bool migrate_caps_check(bool *cap_list,
> >      }
> >  #endif
> >  
> > -
> > -    /* incoming side only */
> > -    if (runstate_check(RUN_STATE_INMIGRATE) &&
> > -        !migrate_multi_channels_is_allowed() &&
> > -        cap_list[MIGRATION_CAPABILITY_MULTIFD]) {
> > -        error_setg(errp, "multifd is not supported by current protocol");
> > -        return false;
> > -    }
> > -
> >      if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_PREEMPT]) {
> >          if (!cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> >              error_setg(errp, "Postcopy preempt requires postcopy-ram");
> > @@ -2440,6 +2439,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> > blk,
> >      MigrationState *s = migrate_get_current();
> >      const char *p = NULL;
> >  
> > +    /* URI is not suitable for migration? */
> > +    if (!migration_uri_validate(uri, errp)) {
> > +        return;
> > +    }
> > +
> >      if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> >                           has_resume && resume, errp)) {
> >          /* Error detected, put into errp */
> > @@ -2452,11 +2456,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> > blk,
> >          }
> >      }
> >  
> > -    migrate_protocol_allow_multi_channels(false);
> >      if (strstart(uri, "tcp:", &p) ||
> >          strstart(uri, "unix:", NULL) ||
> >          strstart(uri, "vsock:", NULL)) {
> > -        migrate_protocol_allow_multi_channels(true);
> >          socket_start_outgoing_migration(s, p ? p : uri, &local_err);
> >  #ifdef CONFIG_RDMA
> >      } else if (strstart(uri, "rdma:", &p)) {
> > @@ -4309,12 +4311,8 @@ void migrate_fd_connect(MigrationState *s, Error 
> > *error_in)
> >      }
> >  
> >      /* This needs to be done before resuming a postcopy */
> > -    if (postcopy_preempt_setup(s, &local_err)) {
> > -        error_report_err(local_err);
> > -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > -                          MIGRATION_STATUS_FAILED);
> > -        migrate_fd_cleanup(s);
> > -        return;
> > +    if (migrate_postcopy_preempt()) {
> > +        postcopy_preempt_setup(s);
> >      }
> 
> I think that this should go in a different patch.

It's so small and natural so I "hid" it in.  But I agree, I'll split.

Thanks!

-- 
Peter Xu