Re: [PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> This patch adds a migration state on src called "postcopy-recover-setup".
> The new state will describe the intermediate step starting from when the
> src QEMU started an postcopy recovery request, until the migration channels
> are properly established, but before the recovery process take place.
>
> The request came from Libvirt where Libvirt currently rely on the migration
> state events to detect migration state changes.  That works for most of the
> migration process but except postcopy recovery failures at the beginning.
>
> Currently postcopy recovery only has two major states:
>
>   - postcopy-paused: this is the state that both sides of QEMU will be in
>     for a long time as long as the migration channel was interrupted.
>
>   - postcopy-recover: this is the state where both sides of QEMU handshake
>     with each other, preparing for a continuation of postcopy which used to
>     be interrupted.
>
> The issue here is when the recovery port is invalid, the src QEMU will take
> the URI/channels, noticing the ports are not valid, and it'll silently keep
> in the postcopy-paused state, with no event sent to Libvirt.  In this case,
> the only thing Libvirt can do is to poll the migration status with a proper
> interval, however that's less optimal.
>
> Considering that this is the only case where Libvirt won't get a
> notification from QEMU on such events, let's add postcopy-recover-setup
> state to mimic what we used to have with the "setup" state of a newly

s/used to //

> initialized migration, describing the phase of connection establishment.
>
> With that, postcopy recovery will have two paths to go now, and either path
> will guarantee an event generated.  Now the events will look like this
> during a recovery process on src QEMU:
>
>   - Initially when the recovery is initiated on src, QEMU will go from
>     "postcopy-paused" -> "postcopy-recover-setup".  Old QEMUs don't have
>     this event.
>
>   - Depending on whether the channel re-establishment is succeeded:
>
>     - In succeeded case, src QEMU will move from "postcopy-recover-setup"
>       to "postcopy-recover".  Old QEMUs also have this event.
>
>     - In failure case, src QEMU will move from "postcopy-recover-setup" to
>       "postcopy-paused" again.  Old QEMUs don't have this event.
>
> This guarantees that Libvirt will always receive a notification for
> recovery process properly.
>
> One thing to mention is, such new status is only needed on src QEMU not
> both.  On dest QEMU, the state machine doesn't change.  Hence the events
> don't change either.  It's done like so because dest QEMU may not have an
> explicit point of setup start.  E.g., it can happen that when dest QEMUs
> doesn't use migrate-recover command to use a new URI/channel, but the old
> URI/channels can be reused in recovery, in which case the old ports simply
> can work again after the network routes are fixed up.
>
> The patch has some touch-ups in the dest path too, but it's because there's
> some unclearness on using migrate_set_state(), so the change should make it
> crystal clear now by checking current status always.  The next step from

Can we get a separate patch for these cleanups?

> that POV would be making migrate_set_state() not using cmpxchg() but always
> update the status, but that's for later.
>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Prasad Pandit <ppandit@redhat.com>
> Buglink: https://issues.redhat.com/browse/RHEL-38485
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi/migration.json      |  4 +++
>  migration/postcopy-ram.h |  3 ++
>  migration/migration.c    | 66 +++++++++++++++++++++++++++++++++++-----
>  migration/postcopy-ram.c |  6 ++++
>  migration/savevm.c       |  4 +--
>  5 files changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index a351fd3714..a135bbcd96 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -142,6 +142,9 @@
>  #
>  # @postcopy-paused: during postcopy but paused.  (since 3.0)
>  #
> +# @postcopy-recover-setup: setup phase for a postcopy recover process,
> +#     preparing for a recover phase to start.  (since 9.1)

recover*y* process
recover*y* phase

> +#
>  # @postcopy-recover: trying to recover from a paused postcopy.  (since
>  #     3.0)
>  #
> @@ -166,6 +169,7 @@
>  { 'enum': 'MigrationStatus',
>    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
>              'active', 'postcopy-active', 'postcopy-paused',
> +            'postcopy-recover-setup',
>              'postcopy-recover', 'completed', 'failed', 'colo',
>              'pre-switchover', 'device', 'wait-unplug' ] }
>  ##
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index ecae941211..a6df1b2811 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_POSTCOPY_RAM_H
>  #define QEMU_POSTCOPY_RAM_H
>  
> +#include "qapi/qapi-types-migration.h"
> +
>  /* Return true if the host supports everything we need to do postcopy-ram */
>  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
>                                      Error **errp);
> @@ -193,5 +195,6 @@ enum PostcopyChannels {
>  void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
>  void postcopy_preempt_setup(MigrationState *s);
>  int postcopy_preempt_establish_channel(MigrationState *s);
> +bool postcopy_is_paused(MigrationStatus status);
>  
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index bfbd657035..9475dce7dc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -595,6 +595,26 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
>      return true;
>  }
>  
> +static bool
> +migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
> +{
> +    MigrationStatus current = mis->state;
> +
> +    if (current == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +        /* Postcopy paused state doesn't change when setup new ports */

The "setup new ports" part is a bit vague. Maybe:

/*
 * The SETUP state only happens at the start of migration. A postcopy
 * migration recovery migration stays in POSTCOPY_PAUSED.
 */

> +        return true;
> +    }
> +
> +    if (current != MIGRATION_STATUS_NONE) {
> +        error_setg(errp, "Illegal migration incoming state: %s",
> +                   MigrationStatus_str(current));
> +        return false;
> +    }

This is a good candidate for a separate patch due to the extra change in
behavior not necessarily related to postcopy.

> +
> +    migrate_set_state(&mis->state, current, MIGRATION_STATUS_SETUP);
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>                                            MigrationChannelList *channels,
>                                            Error **errp)
> @@ -633,8 +653,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>          return;
>      }
>  
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> -                      MIGRATION_STATUS_SETUP);
> +    if (!migration_incoming_state_setup(mis, errp)) {
> +        return;
> +    }
>  
>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>          SocketAddress *saddr = &addr->u.socket;
> @@ -1070,6 +1091,7 @@ bool migration_is_setup_or_active(void)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_SETUP:
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
> @@ -1092,6 +1114,7 @@ bool migration_is_running(void)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_SETUP:
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
> @@ -1229,6 +1252,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
>      case MIGRATION_STATUS_DEVICE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>          /* TODO add some postcopy stats */
>          populate_time_info(info, s);
> @@ -1279,6 +1303,7 @@ static void fill_destination_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:

Does this need to be here? We don't reach this state on destination, right?

>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_FAILED:
>      case MIGRATION_STATUS_COLO:
> @@ -1435,9 +1460,30 @@ static void migrate_error_free(MigrationState *s)
>  
>  static void migrate_fd_error(MigrationState *s, const Error *error)
>  {

The default case of the swtich below is a bit surprising to me. Why
wouldn't we allow this function to be called from other places to set
STATUS_FAILED?

...unless this is only mean for the connection phase, so:

just to check your understanding here because it seems we've drifted a
bit from the original definition on those, specially with
migrate_fd_cleanup(), but does this _fd_ in the function name implies
something like a "connection phase"? As in, "connect to the fd", "the fd
connection errored out" and "cleanup the fd connection". Maybe it's time
to switch this "fd" to something clearer...

> +    MigrationStatus current = s->state;
> +    MigrationStatus next;
> +
>      assert(s->to_dst_file == NULL);
> -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                      MIGRATION_STATUS_FAILED);
> +
> +    switch (current) {
> +    case MIGRATION_STATUS_SETUP:
> +        next = MIGRATION_STATUS_FAILED;
> +        break;
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> +        /* Never fail a postcopy migration; switch back to PAUSED instead */
> +        next = MIGRATION_STATUS_POSTCOPY_PAUSED;

So presumably we can keep recovering the migration indefinitely?

> +        break;
> +    default:
> +        /*
> +         * This really shouldn't happen. Just be careful to not crash a VM
> +         * just for this.  Instead, dump something.
> +         */
> +        error_report("%s: Illegal migration status (%s) detected",
> +                     __func__, MigrationStatus_str(current));
> +        return;
> +    }
> +
> +    migrate_set_state(&s->state, current, next);
>      migrate_set_error(s, error);
>  }
>  
> @@ -1538,6 +1584,7 @@ bool migration_in_postcopy(void)
>      switch (s->state) {
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>          return true;
>      default:
> @@ -1936,6 +1983,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>              return false;
>          }
>  
> +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> +                          MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
> +
>          /* This is a resume, skip init status */
>          return true;
>      }
> @@ -2968,9 +3018,9 @@ static MigThrError postcopy_pause(MigrationState *s)
>           * We wait until things fixed up. Then someone will setup the
>           * status back for us.
>           */
> -        while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +        do {
>              qemu_sem_wait(&s->postcopy_pause_sem);
> -        }
> +        } while (postcopy_is_paused(s->state));

Is there a particular reason to go from while() to do{}while()?

>  
>          if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
>              /* Woken up by a recover procedure. Give it a shot */
> @@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>  {
>      Error *local_err = NULL;
>      uint64_t rate_limit;
> -    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
> +    bool resume = migration_in_postcopy();

Here you're expecting just PAUSED or RECOVER_SETUP, right? We'll not
reach here in any of the other postcopy states.

>      int ret;
>  
>      /*
> @@ -3733,7 +3783,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>  
>      if (resume) {
>          /* Wakeup the main migration thread to do the recovery */
> -        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
>                            MIGRATION_STATUS_POSTCOPY_RECOVER);
>          qemu_sem_post(&s->postcopy_pause_sem);
>          return;
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 97701e6bb2..1c374b7ea1 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1770,3 +1770,9 @@ void *postcopy_preempt_thread(void *opaque)
>  
>      return NULL;
>  }
> +
> +bool postcopy_is_paused(MigrationStatus status)
> +{
> +    return status == MIGRATION_STATUS_POSTCOPY_PAUSED ||
> +        status == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
> +}
> diff --git a/migration/savevm.c b/migration/savevm.c
> index e71410d8c1..deb57833f8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2864,9 +2864,9 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>      error_report("Detected IO failure for postcopy. "
>                   "Migration paused.");
>  
> -    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +    do {
>          qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> -    }
> +    } while (postcopy_is_paused(mis->state));
>  
>      trace_postcopy_pause_incoming_continued();