Zhang Chen <chen.zh...@intel.com> wrote:
> There is no need to start COLO through MIGRATION_STATUS_ACTIVE.
Hi
I don't understand what you are trying to do. In my reading, at least
the commit message is wrong:
void migrate_start_colo_process(MigrationState *s)
{
...
migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
MIGRATION_STATUS_COLO);
...
}
and
void *colo_process_incoming_thread(void *opaque)
{
...
migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
MIGRATION_STATUS_COLO);
So colo starts with MIGRATION_STATUS_ACTIVE.
> Signed-off-by: Zhang Chen <chen.zh...@intel.com>
> ---
> migration/colo.c | 2 --
> migration/migration.c | 18 +++++++++++-------
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 2415325262..ad1a4426b3 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -667,8 +667,6 @@ void migrate_start_colo_process(MigrationState *s)
> colo_checkpoint_notify, s);
>
> qemu_sem_init(&s->colo_exit_sem, 0);
> - migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> - MIGRATION_STATUS_COLO);
> colo_process_checkpoint(s);
> qemu_mutex_lock_iothread();
> }
> diff --git a/migration/migration.c b/migration/migration.c
> index abaf6f9e3d..4c8662a839 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3222,7 +3222,10 @@ static void migration_completion(MigrationState *s)
> goto fail_invalidate;
> }
>
> - if (!migrate_colo_enabled()) {
> + if (migrate_colo_enabled()) {
> + migrate_set_state(&s->state, current_active_state,
> + MIGRATION_STATUS_COLO);
> + } else {
> migrate_set_state(&s->state, current_active_state,
> MIGRATION_STATUS_COMPLETED);
> }
This moves the setup to MIGRATION_STATUS_COLO to completion time instead
of the beggining of the process. I have no clue why. I guess you can
put a comment/commit message to say what you ar.e trynig to do.
> @@ -3607,12 +3610,7 @@ static void migration_iteration_finish(MigrationState
> *s)
> migration_calculate_complete(s);
> runstate_set(RUN_STATE_POSTMIGRATE);
> break;
> -
> - case MIGRATION_STATUS_ACTIVE:
> - /*
> - * We should really assert here, but since it's during
> - * migration, let's try to reduce the usage of assertions.
> - */
> + case MIGRATION_STATUS_COLO:
> if (!migrate_colo_enabled()) {
> error_report("%s: critical error: calling COLO code without "
> "COLO enabled", __func__);
> @@ -3622,6 +3620,12 @@ static void migration_iteration_finish(MigrationState
> *s)
> * Fixme: we will run VM in COLO no matter its old running state.
> * After exited COLO, we will keep running.
> */
> + /* Fallthrough */
> + case MIGRATION_STATUS_ACTIVE:
> + /*
> + * We should really assert here, but since it's during
> + * migration, let's try to reduce the usage of assertions.
> + */
> s->vm_was_running = true;
> /* Fallthrough */
> case MIGRATION_STATUS_FAILED:
I guess this change is related to the previous one, but I don't
understand colo enough to review it.
Later, Juan.
