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

[Fabiano Rosas <farosas@suse.de>]

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) {