Peter Xu <[email protected]> writes:
> Migration notifiers will notify at any of three places: (1) SETUP
> phase, (2) migration completes, (3) migration fails.
>
> There's actually a special case for spice: one can refer to
> b82fc321bf ("Postcopy+spice: Pass spice migration data earlier"). It
> doesn't need another 4th event because in commit 9d9babf78d ("migration:
> MigrationEvent for notifiers") we merged it together with the DONE event.
>
> The merge makes some sense if we treat "switchover" of postcopy as "DONE",
> however that also means for postcopy we'll notify DONE twice.. The other
> one at the end of postcopy when migration_cleanup().
>
> In reality, the current code base will also notify FAILED for postcopy
> twice. It's because an (maybe accidental) change in commit
> 4af667f87c ("migration: notifier error checking").
>
> First of all, we still need that notification when switchover as stated in
> Dave's commit, however that's only needed for spice. To fix it, introduce
> POSTCOPY_START event to differenciate it from DONE. Use that instead in
> postcopy_start(). Then spice will need to capture this event too.
>
> Then we remove the extra FAILED notification in postcopy_start().
>
> If one wonder if other DONE users should also monitor POSTCOPY_START
> event.. We have two more DONE users:
>
> - kvm_arm_gicv3_notifier
> - cpr_exec_notifier
>
> Both of them do not need a notification for POSTCOPY_START, but only when
> migration completed. Actually, both of them are used in CPR, which doesn't
> support postcopy.
>
> I didn't attach Fixes: because I am not aware of any real bug on such
> double reporting. I'm wildly guessing the 2nd notify might be silently
> ignored in many cases. However this is still worth fixing.
>
> Cc: Marc-André Lureau <[email protected]>
> Cc: Dr. David Alan Gilbert <[email protected]>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> include/migration/misc.h | 1 +
> migration/migration.c | 3 +--
> ui/spice-core.c | 3 ++-
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index e26d418a6e..b002466e10 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -63,6 +63,7 @@ typedef enum MigrationEventType {
> MIG_EVENT_PRECOPY_SETUP,
> MIG_EVENT_PRECOPY_DONE,
> MIG_EVENT_PRECOPY_FAILED,
> + MIG_EVENT_POSTCOPY_START,
> MIG_EVENT_MAX
> } MigrationEventType;
With the new state there's a doc text to update at
migration_add_notifier below.
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 47d9189aaf..91775f8472 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2857,7 +2857,7 @@ static int postcopy_start(MigrationState *ms, Error
> **errp)
> * at the transition to postcopy and after the device state; in
> particular
> * spice needs to trigger a transition now
> */
> - migration_call_notifiers(ms, MIG_EVENT_PRECOPY_DONE, NULL);
> + migration_call_notifiers(ms, MIG_EVENT_POSTCOPY_START, NULL);
>
> migration_downtime_end(ms);
>
> @@ -2906,7 +2906,6 @@ fail:
> migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
> }
> migration_block_activate(NULL);
> - migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
Does anything consuming the FAILED event expects it to be issued before
the vm_start() at migration_iteration_finish?
migration_iteration_finish:
bql_lock();
switch (s->state) {
case MIGRATION_STATUS_FAILED:
if (!migration_block_activate(&local_err)) {
error_free(local_err);
break;
}
if (runstate_is_live(s->vm_old_state)) {
if (!runstate_check(RUN_STATE_SHUTDOWN)) {
vm_start();
}
} else {
if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
runstate_set(s->vm_old_state);
}
}
The extra migration_block_activate() in postcopy_start() also seems
wrong, no?
> bql_unlock();
> return -1;
> }
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 8a6050f4ae..ce3c2954e3 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -585,7 +585,8 @@ static int migration_state_notifier(NotifierWithReturn
> *notifier,
>
> if (e->type == MIG_EVENT_PRECOPY_SETUP) {
> spice_server_migrate_start(spice_server);
> - } else if (e->type == MIG_EVENT_PRECOPY_DONE) {
> + } else if (e->type == MIG_EVENT_PRECOPY_DONE ||
> + e->type == MIG_EVENT_POSTCOPY_START) {
> spice_server_migrate_end(spice_server, true);
> spice_have_target_host = false;
> } else if (e->type == MIG_EVENT_PRECOPY_FAILED) {
