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. > > When at this, update the notifier transition graph in the comment, and move > it from migration_add_notifier() to be closer to where the enum is defined. > > 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 | 16 ++++++++++++---- > migration/migration.c | 3 +-- > ui/spice-core.c | 3 ++- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/include/migration/misc.h b/include/migration/misc.h > index e26d418a6e..1cd6cfd7f7 100644 > --- a/include/migration/misc.h > +++ b/include/migration/misc.h > @@ -59,10 +59,22 @@ void migration_shutdown(void); > bool migration_is_running(void); > bool migration_thread_is_self(void); > > +/* > + * Notifiers may receive events in any of the following orders: > + * > + * - MIG_EVENT_PRECOPY_SETUP [-> MIG_EVENT_POSTCOPY_START] > + * -> MIG_EVENT_PRECOPY_DONE > + * > + * - MIG_EVENT_PRECOPY_SETUP [-> MIG_EVENT_POSTCOPY_START] > + * -> MIG_EVENT_PRECOPY_FAILED > + * > + * - MIG_EVENT_PRECOPY_FAILED > + */
Ugh, and for cpr-exec is also possible: - MIG_EVENT_PRECOPY_DONE -> MIG_EVENT_PRECOPY_FAILED Maybe add it for completion. Reviewed-by: Fabiano Rosas <[email protected]> > typedef enum MigrationEventType { > MIG_EVENT_PRECOPY_SETUP, > MIG_EVENT_PRECOPY_DONE, > MIG_EVENT_PRECOPY_FAILED, > + MIG_EVENT_POSTCOPY_START, > MIG_EVENT_MAX > } MigrationEventType; > > @@ -81,10 +93,6 @@ typedef int (*MigrationNotifyFunc)(NotifierWithReturn > *notify, > /* > * Register the notifier @notify to be called when a migration event occurs > * for MIG_MODE_NORMAL, as specified by the MigrationEvent passed to @func. > - * Notifiers may receive events in any of the following orders: > - * - MIG_EVENT_PRECOPY_SETUP -> MIG_EVENT_PRECOPY_DONE > - * - MIG_EVENT_PRECOPY_SETUP -> MIG_EVENT_PRECOPY_FAILED > - * - MIG_EVENT_PRECOPY_FAILED > */ > void migration_add_notifier(NotifierWithReturn *notify, > MigrationNotifyFunc func); > diff --git a/migration/migration.c b/migration/migration.c > index 341b9be80e..bd24006c1a 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2591,7 +2591,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(MIG_EVENT_PRECOPY_DONE, NULL); > + migration_call_notifiers(MIG_EVENT_POSTCOPY_START, NULL); > > migration_downtime_end(ms); > > @@ -2640,7 +2640,6 @@ fail: > migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED); > } > migration_block_activate(NULL); > - migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL); > 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) {
