[PATCH 3/5] migration: Notify migration FAILED before starting VM

Peter Xu posted 5 patches 2 days, 11 hours ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Mark Kanda <mark.kanda@oracle.com>, Ben Chaney <bchaney@akamai.com>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
[PATCH 3/5] migration: Notify migration FAILED before starting VM
Posted by Peter Xu 2 days, 11 hours ago
Devices may opt-in migration FAILED notifiers to be invoked when migration
fails.  Currently, the notifications happen in migration_cleanup().  It is
normally fine, but maybe not ideal if there's dependency of the fallback
v.s. VM starts.

This patch moves the FAILED notification earlier, so that if the failure
happened during switchover, it'll notify before VM restart.

After walking over all existing FAILED notifier users, I got the conclusion
that this should also be a cleaner approach at least from design POV.

We have these notifier users, where the first two do not need to trap
FAILED:

|----------------------------+-------------------------------------+---------------------|
| device                     | handler                             | events needed       |
|----------------------------+-------------------------------------+---------------------|
| gicv3                      | kvm_arm_gicv3_notifier              | DONE                |
| vfio_iommufd / vfio_legacy | vfio_cpr_reboot_notifier            | SETUP               |
| cpr-exec                   | cpr_exec_notifier                   | FAILED, DONE        |
| virtio-net                 | virtio_net_migration_state_notifier | SETUP, FAILED       |
| vfio                       | vfio_migration_state_notifier       | FAILED              |
| vdpa                       | vdpa_net_migration_state_notifier   | SETUP, FAILED       |
| spice [*]                  | migration_state_notifier            | SETUP, FAILED, DONE |
|----------------------------+-------------------------------------+---------------------|

For cpr-exec, it tries to cleanup some cpr-exec specific fd or env
variables.  This should be fine either way, as long as before
migration_cleanup().

For virtio-net, we need to re-plug the primary device back to guest in the
failover mode.  Likely benign.

VFIO needs to re-start the device if FAILED.  IIUC it should do it before
vm_start(), if the VFIO device can be put into a STOPed state due to
migration, we should logically make it running again before vCPUs run.

VDPA will disable SVQ when migration is FAILED.  Likely benign too, but
looks better if we can do it before resuming vCPUs.

For spice, we should rely on "spice_server_migrate_end(false)" to retake
the ownership.  Benign, but looks more reasonable if the spice client does
it before VM runs again.

Note that this change may introduce slightly more downtime, if the
migration failed exactly at the switchover phase.  But that's very rare,
and even if it happens, none of above expects a long delay, but a short
one, likely will be buried in the total downtime even if failed.

Cc: Cédric Le Goater <clg@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 91775f8472..1d9a2fc068 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1481,7 +1481,6 @@ static void migration_cleanup_json_writer(MigrationState *s)
 
 static void migration_cleanup(MigrationState *s)
 {
-    MigrationEventType type;
     QEMUFile *tmp = NULL;
 
     trace_migration_cleanup();
@@ -1535,9 +1534,15 @@ static void migration_cleanup(MigrationState *s)
         /* It is used on info migrate.  We can't free it */
         error_report_err(error_copy(s->error));
     }
-    type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
-                                     MIG_EVENT_PRECOPY_DONE;
-    migration_call_notifiers(s, type, NULL);
+
+    /*
+     * FAILED notification should have already happened.  Notify DONE if
+     * migration completed successfully.
+     */
+    if (!migration_has_failed(s)) {
+        migration_call_notifiers(s, MIG_EVENT_PRECOPY_DONE, NULL);
+    }
+
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
@@ -3589,6 +3594,13 @@ static void migration_iteration_finish(MigrationState *s)
             error_free(local_err);
             break;
         }
+
+        /*
+         * Notify FAILED before starting VM, so that devices can invoke
+         * necessary fallbacks before vCPUs run again.
+         */
+        migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
+
         if (runstate_is_live(s->vm_old_state)) {
             if (!runstate_check(RUN_STATE_SHUTDOWN)) {
                 vm_start();
-- 
2.50.1


Re: [PATCH 3/5] migration: Notify migration FAILED before starting VM
Posted by Fabiano Rosas 1 day, 21 hours ago
Peter Xu <peterx@redhat.com> writes:

> Devices may opt-in migration FAILED notifiers to be invoked when migration
> fails.  Currently, the notifications happen in migration_cleanup().  It is
> normally fine, but maybe not ideal if there's dependency of the fallback
> v.s. VM starts.
>
> This patch moves the FAILED notification earlier, so that if the failure
> happened during switchover, it'll notify before VM restart.
>

The change to FAILED in patch 2 should come to this patch to avoid
having a window where the notification only happens at the end.

> After walking over all existing FAILED notifier users, I got the conclusion
> that this should also be a cleaner approach at least from design POV.
>
> We have these notifier users, where the first two do not need to trap
> FAILED:
>
> |----------------------------+-------------------------------------+---------------------|
> | device                     | handler                             | events needed       |
> |----------------------------+-------------------------------------+---------------------|
> | gicv3                      | kvm_arm_gicv3_notifier              | DONE                |
> | vfio_iommufd / vfio_legacy | vfio_cpr_reboot_notifier            | SETUP               |
> | cpr-exec                   | cpr_exec_notifier                   | FAILED, DONE        |
> | virtio-net                 | virtio_net_migration_state_notifier | SETUP, FAILED       |
> | vfio                       | vfio_migration_state_notifier       | FAILED              |
> | vdpa                       | vdpa_net_migration_state_notifier   | SETUP, FAILED       |
> | spice [*]                  | migration_state_notifier            | SETUP, FAILED, DONE |
> |----------------------------+-------------------------------------+---------------------|
>
> For cpr-exec, it tries to cleanup some cpr-exec specific fd or env
> variables.  This should be fine either way, as long as before
> migration_cleanup().
>
> For virtio-net, we need to re-plug the primary device back to guest in the
> failover mode.  Likely benign.
>
> VFIO needs to re-start the device if FAILED.  IIUC it should do it before
> vm_start(), if the VFIO device can be put into a STOPed state due to
> migration, we should logically make it running again before vCPUs run.
>
> VDPA will disable SVQ when migration is FAILED.  Likely benign too, but
> looks better if we can do it before resuming vCPUs.
>
> For spice, we should rely on "spice_server_migrate_end(false)" to retake
> the ownership.  Benign, but looks more reasonable if the spice client does
> it before VM runs again.
>
> Note that this change may introduce slightly more downtime, if the
> migration failed exactly at the switchover phase.  But that's very rare,
> and even if it happens, none of above expects a long delay, but a short
> one, likely will be buried in the total downtime even if failed.
>
> Cc: Cédric Le Goater <clg@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 91775f8472..1d9a2fc068 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1481,7 +1481,6 @@ static void migration_cleanup_json_writer(MigrationState *s)
>  
>  static void migration_cleanup(MigrationState *s)
>  {
> -    MigrationEventType type;
>      QEMUFile *tmp = NULL;
>  
>      trace_migration_cleanup();
> @@ -1535,9 +1534,15 @@ static void migration_cleanup(MigrationState *s)
>          /* It is used on info migrate.  We can't free it */
>          error_report_err(error_copy(s->error));
>      }
> -    type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
> -                                     MIG_EVENT_PRECOPY_DONE;
> -    migration_call_notifiers(s, type, NULL);
> +
> +    /*
> +     * FAILED notification should have already happened.  Notify DONE if
> +     * migration completed successfully.
> +     */
> +    if (!migration_has_failed(s)) {
> +        migration_call_notifiers(s, MIG_EVENT_PRECOPY_DONE, NULL);
> +    }
> +
>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>  }
>  
> @@ -3589,6 +3594,13 @@ static void migration_iteration_finish(MigrationState *s)
>              error_free(local_err);
>              break;
>          }
> +
> +        /*
> +         * Notify FAILED before starting VM, so that devices can invoke
> +         * necessary fallbacks before vCPUs run again.
> +         */
> +        migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
> +
>          if (runstate_is_live(s->vm_old_state)) {
>              if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>                  vm_start();
Re: [PATCH 3/5] migration: Notify migration FAILED before starting VM
Posted by Peter Xu 1 day, 18 hours ago
On Fri, Jan 23, 2026 at 09:59:35AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Devices may opt-in migration FAILED notifiers to be invoked when migration
> > fails.  Currently, the notifications happen in migration_cleanup().  It is
> > normally fine, but maybe not ideal if there's dependency of the fallback
> > v.s. VM starts.
> >
> > This patch moves the FAILED notification earlier, so that if the failure
> > happened during switchover, it'll notify before VM restart.
> >
> 
> The change to FAILED in patch 2 should come to this patch to avoid
> having a window where the notification only happens at the end.

Hmm.. Isn't that expected?  Even after patch 2, we still notify FAILED at
the end for precopy.  It's the same for postcopy.

For a failed postcopy we have following behavior:

Before patch 2
==============

  - notify FAILED (during switchover)
  - vm_start()
  - notify FAILED (during migration_cleanup)

After patch 2 
=============

  - vm_start()
  - notify FAILED (during migration_cleanup)

So patch 2 fixes the duplicate issue, and only fixes that.

After patch 3
=============

  - notify FAILED (during migration_iteration_finish)
  - vm_start()

Patch 3 changes the place of FAILED notification so that it happens always
before vm_start(), for both precopy and postcopy.

-- 
Peter Xu
Re: [PATCH 3/5] migration: Notify migration FAILED before starting VM
Posted by Fabiano Rosas 1 day, 16 hours ago
Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 23, 2026 at 09:59:35AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Devices may opt-in migration FAILED notifiers to be invoked when migration
>> > fails.  Currently, the notifications happen in migration_cleanup().  It is
>> > normally fine, but maybe not ideal if there's dependency of the fallback
>> > v.s. VM starts.
>> >
>> > This patch moves the FAILED notification earlier, so that if the failure
>> > happened during switchover, it'll notify before VM restart.
>> >
>> 
>> The change to FAILED in patch 2 should come to this patch to avoid
>> having a window where the notification only happens at the end.
>
> Hmm.. Isn't that expected?  Even after patch 2, we still notify FAILED at
> the end for precopy.  It's the same for postcopy.
>

Sorry, I meant: s/at the end/after vm_start/.

> For a failed postcopy we have following behavior:
>
> Before patch 2
> ==============
>
>   - notify FAILED (during switchover)
>   - vm_start()
>   - notify FAILED (during migration_cleanup)
>
> After patch 2 
> =============
>
>   - vm_start()
>   - notify FAILED (during migration_cleanup)
>
> So patch 2 fixes the duplicate issue, and only fixes that.
>
> After patch 3
> =============
>
>   - notify FAILED (during migration_iteration_finish)
>   - vm_start()
>
> Patch 3 changes the place of FAILED notification so that it happens always
> before vm_start(), for both precopy and postcopy.

Right, my point is that with patch 3 we're establishing that the correct
place to notify is before vm_start(). But after patch 2, *if* any driver
actually depends on being informed of failure *before* starting the VM,
that will not happen. I think both changes could be made at once so that
this intermediate state never exists.