Re: [PATCH v4 1/8] migration: stop vm earlier for cpr

[Fabiano Rosas <farosas@suse.de>]

Ben Chaney <bchaney@akamai.com> writes:

> From: Steve Sistare <steven.sistare@oracle.com>
>
> Stop the vm earlier for cpr, before cpr_save_state which causes new QEMU
> to proceed and initialize devices.  We must guarantee devices are stopped
> in old QEMU, and all source notifiers called, before they are initialized
> in new QEMU.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Signed-off-by: Ben Chaney <bchaney@akamai.com>
> ---
>  migration/migration.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1bcde301f7..f36e59d9e8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1654,6 +1654,7 @@ void migration_cancel(void)
>                            MIGRATION_STATUS_CANCELLED);
>          cpr_state_close();
>          migrate_hup_delete(s);
> +        vm_resume(s->vm_old_state);
>      }
>  }
>  
> @@ -2212,6 +2213,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>      MigrationAddress *addr = NULL;
>      MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
>      MigrationChannel *cpr_channel = NULL;
> +    bool stopped = false;
>  

This is not needed anymore, all cleanup now happens after
migration_connect_error_propagate -> migration_cleanup.

>      /*
>       * Having preliminary checks for uri and channel
> @@ -2264,6 +2266,46 @@ void qmp_migrate(const char *uri, bool has_channels,
>          return;
>      }
>  
> +    /*
> +     * CPR-transfer  ordering:
> +     *
> +     *   SOURCE                              TARGET
> +     *   ------                              ------
> +     *                                       cpr_state_load() blocks
> +     *   |                                        |
> +     *   |  1. migration_stop_vm()                |
> +     *   |     VM stopped, devices quiesced       |
> +     *   |                                        | Waiting for
> +     *   |  2. notifiers (PRECOPY_SETUP)          | FDs from source
> +     *   |     vhost_reset_owner() releases       |
> +     *   |     device ownership                   |

Patch 2 should come before, then. Otherwise this is not accurate.

> +     *   |                                        |
> +     *   |  3. cpr_state_save() ---- FDs -------> |
> +     *   |                                        |
> +     *   v                                        v
> +     *   postmigrate                         Device init begins
> +     *                                       - cpr_find_fd()
> +     *                                       - vhost_dev_init()
> +     *                                       - VHOST_SET_OWNER
> +     *
> +     * Step 3 is the synchronization/cut-over point. Target proceeds immediately
> +     * upon receiving FDs, so steps 1-2 must complete otherwise:
> +     * - Target's VHOST_SET_OWNER fails with -EBUSY (source still owns)
> +     * - Race between source I/O and target device init
> +     *
> +     *  We stop the VM early (before FD transfer) to prevent this race.
> +     *  Unlike regular migration, CPR-transfer passes memory via FD (memfd)
> +     *  rather than copying RAM, so early VM stop should have minimal downtime.
> +     */
> +    if (migrate_mode_is_cpr(s)) {
> +        int ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
> +        if (ret < 0) {
> +            error_setg(&local_err, "migration_stop_vm failed, error %d", -ret);
> +            goto out;
> +        }
> +        stopped = true;
> +    }
> +
>      if (!cpr_state_save(cpr_channel, &local_err)) {
>          goto out;
>      }
> @@ -2290,6 +2332,9 @@ out:
>      if (local_err) {
>          migration_connect_error_propagate(s, error_copy(local_err));
>          error_propagate(errp, local_err);
> +        if (stopped) {
> +            vm_resume(s->vm_old_state);
> +        }

This resume should now happen at migration_cleanup.

>      }
>  }
>  
> @@ -2334,6 +2379,9 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          }
>          migration_connect_error_propagate(s, error_copy(local_err));
>          error_propagate(errp, local_err);
> +        if (migrate_mode_is_cpr(s)) {
> +            vm_resume(s->vm_old_state);
> +        }

Same here.

>          return;
>      }
>  }
> @@ -4017,7 +4065,6 @@ void migration_connect(MigrationState *s, Error *error_in)
>      Error *local_err = NULL;
>      uint64_t rate_limit;
>      bool resume = (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
> -    int ret;
>  
>      /*
>       * If there's a previous error, free it and prepare for another one.
> @@ -4088,14 +4135,6 @@ void migration_connect(MigrationState *s, Error *error_in)
>          return;
>      }
>  
> -    if (migrate_mode_is_cpr(s)) {
> -        ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
> -        if (ret < 0) {
> -            error_setg(&local_err, "migration_stop_vm failed, error %d", -ret);
> -            goto fail;
> -        }
> -    }
> -
>      /*
>       * Take a refcount to make sure the migration object won't get freed by
>       * the main thread already in migration_shutdown().