On Fri, Jul 14, 2023 at 08:48:23PM +0800,
Wei Wang <wei.w.w...@intel.com> wrote:
> Current migration_completion function is a bit long. Refactor the long
> implementation into different subfunctions:
> - migration_completion_precopy: completion code related to precopy
> - migration_completion_postcopy: completion code related to postcopy
> - close_return_path_on_source: rp thread related cleanup on migration
> completion. It is named to match with open_return_path_on_source.
>
> This improves readability and is easier for future updates (e.g. add new
> subfunctions when completion code related to new features are needed). No
> functional changes intended.
>
> Signed-off-by: Wei Wang <wei.w.w...@intel.com>
> ---
> migration/migration.c | 181 +++++++++++++++++++++++++-----------------
> 1 file changed, 106 insertions(+), 75 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 91bba630a8..83f1c74f99 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2058,6 +2058,21 @@ static int
> await_return_path_close_on_source(MigrationState *ms)
> return ms->rp_state.error;
> }
>
> +static int close_return_path_on_source(MigrationState *ms)
> +{
> + int ret;
> +
> + if (!ms->rp_state.rp_thread_created) {
> + return 0;
> + }
> +
> + trace_migration_return_path_end_before();
> + ret = await_return_path_close_on_source(ms);
> + trace_migration_return_path_end_after(ret);
> +
> + return ret;
> +}
> +
There is only one caller, migration_completion(). We can consolidate
two functions, await_return_path_close_on_source() and
close_return_path_on_source(), into single function.
> static inline void
> migration_wait_main_channel(MigrationState *ms)
> {
> @@ -2288,66 +2303,107 @@ static int migration_maybe_pause(MigrationState *s,
> return s->state == new_state ? 0 : -EINVAL;
> }
>
> -/**
> - * migration_completion: Used by migration_thread when there's not much left.
> - * The caller 'breaks' the loop when this returns.
> - *
> - * @s: Current migration state
> - */
> -static void migration_completion(MigrationState *s)
> +static int migration_completion_precopy(MigrationState *s,
> + int *current_active_state)
> {
> int ret;
> - int current_active_state = s->state;
>
> - if (s->state == MIGRATION_STATUS_ACTIVE) {
> - qemu_mutex_lock_iothread();
> - s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> - qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
> + qemu_mutex_lock_iothread();
> + s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
>
> - s->vm_old_state = runstate_get();
> - global_state_store();
> + s->vm_old_state = runstate_get();
> + global_state_store();
>
> - ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> - trace_migration_completion_vm_stop(ret);
> - if (ret >= 0) {
> - ret = migration_maybe_pause(s, ¤t_active_state,
> - MIGRATION_STATUS_DEVICE);
> - }
> - if (ret >= 0) {
> - /*
> - * Inactivate disks except in COLO, and track that we
> - * have done so in order to remember to reactivate
> - * them if migration fails or is cancelled.
> - */
> - s->block_inactive = !migrate_colo();
> - migration_rate_set(RATE_LIMIT_DISABLED);
> - ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
> - s->block_inactive);
> - }
> + ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> + trace_migration_completion_vm_stop(ret);
> + if (ret < 0) {
> + goto out_unlock;
> + }
>
> - qemu_mutex_unlock_iothread();
> + ret = migration_maybe_pause(s, current_active_state,
> + MIGRATION_STATUS_DEVICE);
> + if (ret < 0) {
> + goto out_unlock;
> + }
>
> - if (ret < 0) {
> - goto fail;
> - }
> - } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> - trace_migration_completion_postcopy_end();
> + /*
> + * Inactivate disks except in COLO, and track that we have done so in
> order
> + * to remember to reactivate them if migration fails or is cancelled.
> + */
> + s->block_inactive = !migrate_colo();
> + migration_rate_set(RATE_LIMIT_DISABLED);
> + ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
> + s->block_inactive);
> +out_unlock:
> + qemu_mutex_unlock_iothread();
> + return ret;
> +}
>
> - qemu_mutex_lock_iothread();
> - qemu_savevm_state_complete_postcopy(s->to_dst_file);
> - qemu_mutex_unlock_iothread();
> +static int migration_completion_postcopy(MigrationState *s)
> +{
> + trace_migration_completion_postcopy_end();
> +
> + qemu_mutex_lock_iothread();
> + qemu_savevm_state_complete_postcopy(s->to_dst_file);
> + qemu_mutex_unlock_iothread();
> +
> + /*
> + * Shutdown the postcopy fast path thread. This is only needed when dest
> + * QEMU binary is old (7.1/7.2). QEMU 8.0+ doesn't need this.
> + */
> + if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> + postcopy_preempt_shutdown_file(s);
> + }
> +
> + trace_migration_completion_postcopy_end_after_complete();
> +
> + return 0;
Always return 0? Make it void.
> +}
>
> +static void migration_completion_failed(MigrationState *s,
> + int current_active_state)
> +{
> + if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
> + s->state == MIGRATION_STATUS_DEVICE)) {
> /*
> - * Shutdown the postcopy fast path thread. This is only needed
> - * when dest QEMU binary is old (7.1/7.2). QEMU 8.0+ doesn't need
> - * this.
> + * If not doing postcopy, vm_start() will be called: let's
> + * regain control on images.
> */
> - if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> - postcopy_preempt_shutdown_file(s);
> + Error *local_err = NULL;
> +
> + qemu_mutex_lock_iothread();
> + bdrv_activate_all(&local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + } else {
> + s->block_inactive = false;
> }
> + qemu_mutex_unlock_iothread();
> + }
>
> - trace_migration_completion_postcopy_end_after_complete();
> - } else {
> + migrate_set_state(&s->state, current_active_state,
> + MIGRATION_STATUS_FAILED);
> +}
> +
> +/**
> + * migration_completion: Used by migration_thread when there's not much left.
> + * The caller 'breaks' the loop when this returns.
> + *
> + * @s: Current migration state
> + */
> +static void migration_completion(MigrationState *s)
> +{
> + int ret = -1;
> + int current_active_state = s->state;
> +
> + if (s->state == MIGRATION_STATUS_ACTIVE) {
> + ret = migration_completion_precopy(s, ¤t_active_state);
> + } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> + ret = migration_completion_postcopy(s);
Here we can set ret = 0.
> + }
> +
> + if (ret < 0) {
> goto fail;
> }
>
> @@ -2357,14 +2413,8 @@ static void migration_completion(MigrationState *s)
> * it will wait for the destination to send it's status in
> * a SHUT command).
> */
> - if (s->rp_state.rp_thread_created) {
> - int rp_error;
> - trace_migration_return_path_end_before();
> - rp_error = await_return_path_close_on_source(s);
> - trace_migration_return_path_end_after(rp_error);
> - if (rp_error) {
> - goto fail;
> - }
> + if (close_return_path_on_source(s) < 0) {
> + goto fail;
> }
>
> if (qemu_file_get_error(s->to_dst_file)) {
> @@ -2384,26 +2434,7 @@ static void migration_completion(MigrationState *s)
> return;
>
> fail:
> - if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
> - s->state == MIGRATION_STATUS_DEVICE)) {
> - /*
> - * If not doing postcopy, vm_start() will be called: let's
> - * regain control on images.
> - */
> - Error *local_err = NULL;
> -
> - qemu_mutex_lock_iothread();
> - bdrv_activate_all(&local_err);
> - if (local_err) {
> - error_report_err(local_err);
> - } else {
> - s->block_inactive = false;
> - }
> - qemu_mutex_unlock_iothread();
> - }
> -
> - migrate_set_state(&s->state, current_active_state,
> - MIGRATION_STATUS_FAILED);
> + migration_completion_failed(s, current_active_state);
> }
>
> /**
> --
> 2.27.0
>
>
--
Isaku Yamahata <isaku.yamah...@gmail.com>
