Fabiano Rosas <[email protected]> writes:
> 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?
>
Heh, nevermind...
>> 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) {
