migration: Notifier fixes for 11.0

[PATCH 2/5] migration: Fix double notification of DONE/FAIL for postcopy

Posted by Peter Xu 2 days, 11 hours ago

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 <marcandre.lureau@redhat.com>
Cc: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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;
 
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);
     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) {
-- 
2.50.1

Re: [PATCH 2/5] migration: Fix double notification of DONE/FAIL for postcopy

Posted by Fabiano Rosas 1 day, 21 hours ago

Peter Xu <peterx@redhat.com> 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 <marcandre.lureau@redhat.com>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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) {

Re: [PATCH 2/5] migration: Fix double notification of DONE/FAIL for postcopy

Posted by Peter Xu 1 day, 19 hours ago

On Fri, Jan 23, 2026 at 09:52:31AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> 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 <marcandre.lureau@redhat.com>
> > Cc: Dr. David Alan Gilbert <dave@treblig.org>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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.

Yeh I noticed it, I didn't do it as I found POSTCOPY_START is special in
this case marking an optional phase for postcopy-only.

I can still update it, though..  Then it'll look like:

   - MIG_EVENT_SETUP [-> MIG_EVENT_POSTCOPY_START] -> MIG_EVENT_DONE
   - MIG_EVENT_SETUP [-> MIG_EVENT_POSTCOPY_START] -> MIG_EVENT_FAILED
   - MIG_EVENT_FAILED

I'll also move it above the enum instead if no objections.

-- 
Peter Xu

Re: [PATCH 2/5] migration: Fix double notification of DONE/FAIL for postcopy

Posted by Fabiano Rosas 1 day, 21 hours ago

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> 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 <marcandre.lureau@redhat.com>
>> Cc: Dr. David Alan Gilbert <dave@treblig.org>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  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) {

[PATCH 1/5] migration: Add a tracepoint for invoking migration notifiers
[PATCH 2/5] migration: Fix double notification of DONE/FAIL for postcopy
[PATCH 3/5] migration: Notify migration FAILED before starting VM
[PATCH 4/5] migration: Drop explicit block activation in postcopy fail path
[PATCH 5/5] migration: Rename MIG_EVENT_PRECOPY_* to MIG_EVENT_*