[PATCH 0/4] migration: New postcopy state, and some cleanups

[PATCH 0/4] migration: New postcopy state, and some cleanups

[Peter Xu]

The major goal of this patchset is patch 4, which introduced a new postcopy
state so that we will send an event in postcopy reconnect failures that
Libvirt would prefer to have.  There's more information for that issue in
the commit message alone.

Patch 1-2 are cleanups that are not directly relevant but I found/stored
that could be good to have.  I made it simple by putting them together in
one thread to make patch management easier, but I can send them separately
when necessary.

Patch 3 is also a cleanup, but will be needed for patch 4 as dependency.

Comments welcomed, thanks.

CI: https://gitlab.com/peterx/qemu/-/pipelines/1328309702
    (msys2-64bit is failing, but doesn't look relevant)

Peter Xu (4):
  migration/multifd: Avoid the final FLUSH in complete()
  migration: Rename thread debug names
  migration: Use MigrationStatus instead of int
  migration/postcopy: Add postcopy-recover-setup phase

 qapi/migration.json      |  4 ++
 migration/migration.h    |  9 +++--
 migration/postcopy-ram.h |  3 ++
 migration/colo.c         |  2 +-
 migration/migration.c    | 85 ++++++++++++++++++++++++++++++++--------
 migration/multifd.c      |  6 +--
 migration/postcopy-ram.c | 10 ++++-
 migration/ram.c          |  4 --
 migration/savevm.c       |  6 +--
 9 files changed, 95 insertions(+), 34 deletions(-)

-- 
2.45.0

[PATCH 1/4] migration/multifd: Avoid the final FLUSH in complete()

[Peter Xu]

We always do the flush when finishing one round of scan, and during
complete() phase we should scan one more round making sure no dirty page
existed.  In that case we shouldn't need one explicit FLUSH at the end of
complete(), as when reaching there all pages should have been flushed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index ceea586b06..edec1a2d07 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3300,10 +3300,6 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         }
     }
 
-    if (migrate_multifd() && !migrate_multifd_flush_after_each_section() &&
-        !migrate_mapped_ram()) {
-        qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
-    }
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
     return qemu_fflush(f);
 }
-- 
2.45.0

Re: [PATCH 1/4] migration/multifd: Avoid the final FLUSH in complete()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> We always do the flush when finishing one round of scan, and during
> complete() phase we should scan one more round making sure no dirty page
> existed.  In that case we shouldn't need one explicit FLUSH at the end of
> complete(), as when reaching there all pages should have been flushed.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 1/4] migration/multifd: Avoid the final FLUSH in complete()

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> We always do the flush when finishing one round of scan, and during
>> complete() phase we should scan one more round making sure no dirty page
>> existed.  In that case we shouldn't need one explicit FLUSH at the end of
>> complete(), as when reaching there all pages should have been flushed.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>

Actually, let's be more clear and make this a:

Tested-by: Fabiano Rosas <farosas@suse.de>

That way I'll remember this went through the same tests as the other
multifd sync changes we made.

Re: [PATCH 1/4] migration/multifd: Avoid the final FLUSH in complete()

[Peter Xu]

On Thu, Jun 13, 2024 at 11:54:58AM -0300, Fabiano Rosas wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> We always do the flush when finishing one round of scan, and during
> >> complete() phase we should scan one more round making sure no dirty page
> >> existed.  In that case we shouldn't need one explicit FLUSH at the end of
> >> complete(), as when reaching there all pages should have been flushed.
> >>
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> 
> Actually, let's be more clear and make this a:
> 
> Tested-by: Fabiano Rosas <farosas@suse.de>
> 
> That way I'll remember this went through the same tests as the other
> multifd sync changes we made.

Or... could I take both? :)  And thanks for checking that.

-- 
Peter Xu

[PATCH 2/4] migration: Rename thread debug names

[Peter Xu]

The postcopy thread names on dest QEMU are slightly confusing, partly I'll
need to blame myself on 36f62f11e4 ("migration: Postcopy preemption
preparation on channel creation").  E.g., "fault-fast" reads like a fast
version of "fault-default", but it's actually the fast version of
"postcopy/listen".

Taking this chance, rename all the migration threads with proper rules.
Considering we only have 15 chars usable, prefix all threads with "mig/",
meanwhile identify src/dst threads properly this time.  So now most thread
names will look like "mig/DIR/xxx", where DIR will be "src"/"dst", except
the bg-snapshot thread which doesn't have a direction.

For multifd threads, making them "mig/{src|dst}/{send|recv}_%d".

We used to have "live_migration" thread for a very long time, now it's
called "mig/src/main".  We may hope to have "mig/dst/main" soon but not
yet.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/colo.c         | 2 +-
 migration/migration.c    | 6 +++---
 migration/multifd.c      | 6 +++---
 migration/postcopy-ram.c | 4 ++--
 migration/savevm.c       | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index f96c2ee069..6449490221 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -935,7 +935,7 @@ void coroutine_fn colo_incoming_co(void)
     assert(bql_locked());
     assert(migration_incoming_colo_enabled());
 
-    qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
+    qemu_thread_create(&th, "mig/dst/colo", colo_process_incoming_thread,
                        mis, QEMU_THREAD_JOINABLE);
 
     mis->colo_incoming_co = qemu_coroutine_self();
diff --git a/migration/migration.c b/migration/migration.c
index e1b269624c..d41e00ed4c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2408,7 +2408,7 @@ static int open_return_path_on_source(MigrationState *ms)
 
     trace_open_return_path_on_source();
 
-    qemu_thread_create(&ms->rp_state.rp_thread, "return path",
+    qemu_thread_create(&ms->rp_state.rp_thread, "mig/src/rp-thr",
                        source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
     ms->rp_state.rp_thread_created = true;
 
@@ -3747,10 +3747,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     }
 
     if (migrate_background_snapshot()) {
-        qemu_thread_create(&s->thread, "bg_snapshot",
+        qemu_thread_create(&s->thread, "mig/snapshot",
                 bg_migration_thread, s, QEMU_THREAD_JOINABLE);
     } else {
-        qemu_thread_create(&s->thread, "live_migration",
+        qemu_thread_create(&s->thread, "mig/src/main",
                 migration_thread, s, QEMU_THREAD_JOINABLE);
     }
     s->migration_thread_running = true;
diff --git a/migration/multifd.c b/migration/multifd.c
index f317bff077..7afc0965f6 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1059,7 +1059,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     args->p = p;
 
     p->tls_thread_created = true;
-    qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
+    qemu_thread_create(&p->tls_thread, "mig/src/tls",
                        multifd_tls_handshake_thread, args,
                        QEMU_THREAD_JOINABLE);
     return true;
@@ -1185,7 +1185,7 @@ bool multifd_send_setup(void)
         } else {
             p->iov = g_new0(struct iovec, page_count);
         }
-        p->name = g_strdup_printf("multifdsend_%d", i);
+        p->name = g_strdup_printf("mig/src/send_%d", i);
         p->page_size = qemu_target_page_size();
         p->page_count = page_count;
         p->write_flags = 0;
@@ -1601,7 +1601,7 @@ int multifd_recv_setup(Error **errp)
                 + sizeof(uint64_t) * page_count;
             p->packet = g_malloc0(p->packet_len);
         }
-        p->name = g_strdup_printf("multifdrecv_%d", i);
+        p->name = g_strdup_printf("mig/dst/recv_%d", i);
         p->iov = g_new0(struct iovec, page_count);
         p->normal = g_new0(ram_addr_t, page_count);
         p->zero = g_new0(ram_addr_t, page_count);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 3419779548..97701e6bb2 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1238,7 +1238,7 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
         return -1;
     }
 
-    postcopy_thread_create(mis, &mis->fault_thread, "fault-default",
+    postcopy_thread_create(mis, &mis->fault_thread, "mig/dst/fault",
                            postcopy_ram_fault_thread, QEMU_THREAD_JOINABLE);
     mis->have_fault_thread = true;
 
@@ -1258,7 +1258,7 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
          * This thread needs to be created after the temp pages because
          * it'll fetch RAM_CHANNEL_POSTCOPY PostcopyTmpPage immediately.
          */
-        postcopy_thread_create(mis, &mis->postcopy_prio_thread, "fault-fast",
+        postcopy_thread_create(mis, &mis->postcopy_prio_thread, "mig/dst/preempt",
                                postcopy_preempt_thread, QEMU_THREAD_JOINABLE);
         mis->preempt_thread_status = PREEMPT_THREAD_CREATED;
     }
diff --git a/migration/savevm.c b/migration/savevm.c
index c621f2359b..e71410d8c1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2129,7 +2129,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
     }
 
     mis->have_listen_thread = true;
-    postcopy_thread_create(mis, &mis->listen_thread, "postcopy/listen",
+    postcopy_thread_create(mis, &mis->listen_thread, "mig/dst/listen",
                            postcopy_ram_listen_thread, QEMU_THREAD_DETACHED);
     trace_loadvm_postcopy_handle_listen("return");
 
-- 
2.45.0

Re: [PATCH 2/4] migration: Rename thread debug names

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> The postcopy thread names on dest QEMU are slightly confusing, partly I'll
> need to blame myself on 36f62f11e4 ("migration: Postcopy preemption
> preparation on channel creation").  E.g., "fault-fast" reads like a fast
> version of "fault-default", but it's actually the fast version of
> "postcopy/listen".
>
> Taking this chance, rename all the migration threads with proper rules.
> Considering we only have 15 chars usable, prefix all threads with "mig/",
> meanwhile identify src/dst threads properly this time.  So now most thread
> names will look like "mig/DIR/xxx", where DIR will be "src"/"dst", except
> the bg-snapshot thread which doesn't have a direction.
>
> For multifd threads, making them "mig/{src|dst}/{send|recv}_%d".
>
> We used to have "live_migration" thread for a very long time, now it's
> called "mig/src/main".  We may hope to have "mig/dst/main" soon but not
> yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 3/4] migration: Use MigrationStatus instead of int

[Peter Xu]

QEMU uses "int" in most cases even if it stores MigrationStatus.  I don't
know why, so let's try to do that right and see what blows up..

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h |  9 +++++----
 migration/migration.c | 13 +++++++------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 6af01362d4..38aa1402d5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -160,7 +160,7 @@ struct MigrationIncomingState {
     /* PostCopyFD's for external userfaultfds & handlers of shared memory */
     GArray   *postcopy_remote_fds;
 
-    int state;
+    MigrationStatus state;
 
     /*
      * The incoming migration coroutine, non-NULL during qemu_loadvm_state().
@@ -301,7 +301,7 @@ struct MigrationState {
     /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
-    int state;
+    MigrationStatus state;
 
     /* State related to return path */
     struct {
@@ -459,7 +459,8 @@ struct MigrationState {
     bool rdma_migration;
 };
 
-void migrate_set_state(int *state, int old_state, int new_state);
+void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
+                       MigrationStatus new_state);
 
 void migration_fd_process_incoming(QEMUFile *f);
 void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
@@ -479,7 +480,7 @@ int migrate_init(MigrationState *s, Error **errp);
 bool migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
-bool migration_postcopy_is_alive(int state);
+bool migration_postcopy_is_alive(MigrationStatus state);
 MigrationState *migrate_get_current(void);
 bool migration_has_failed(MigrationState *);
 bool migrate_mode_is_cpr(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index d41e00ed4c..bfbd657035 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -390,7 +390,7 @@ void migration_incoming_state_destroy(void)
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
-static void migrate_generate_event(int new_state)
+static void migrate_generate_event(MigrationStatus new_state)
 {
     if (migrate_events()) {
         qapi_event_send_migration(new_state);
@@ -1273,8 +1273,6 @@ static void fill_destination_migration_info(MigrationInfo *info)
     }
 
     switch (mis->state) {
-    case MIGRATION_STATUS_NONE:
-        return;
     case MIGRATION_STATUS_SETUP:
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_CANCELLED:
@@ -1290,6 +1288,8 @@ static void fill_destination_migration_info(MigrationInfo *info)
         info->has_status = true;
         fill_destination_postcopy_migration_info(info);
         break;
+    default:
+        return;
     }
     info->status = mis->state;
 
@@ -1337,7 +1337,8 @@ void qmp_migrate_start_postcopy(Error **errp)
 
 /* shared migration helpers */
 
-void migrate_set_state(int *state, int old_state, int new_state)
+void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
+                       MigrationStatus new_state)
 {
     assert(new_state < MIGRATION_STATUS__MAX);
     if (qatomic_cmpxchg(state, old_state, new_state) == old_state) {
@@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
     }
 }
 
-bool migration_postcopy_is_alive(int state)
+bool migration_postcopy_is_alive(MigrationStatus state)
 {
     switch (state) {
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
@@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
     case MIGRATION_STATUS_DEVICE:
     case MIGRATION_STATUS_WAIT_UNPLUG:
         return false;
-    case MIGRATION_STATUS__MAX:
+    default:
         g_assert_not_reached();
     }
 
-- 
2.45.0

Re: [PATCH 3/4] migration: Use MigrationStatus instead of int

[Peter Xu]

On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
>      }
>  }
>  
> -bool migration_postcopy_is_alive(int state)
> +bool migration_postcopy_is_alive(MigrationStatus state)
>  {
>      switch (state) {
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
>      case MIGRATION_STATUS_DEVICE:
>      case MIGRATION_STATUS_WAIT_UNPLUG:
>          return false;
> -    case MIGRATION_STATUS__MAX:
> +    default:
>          g_assert_not_reached();
>      }

This survives the tests, but I just found that it's risky, as it's not
covering all the states..

I'll squash below when I send v2 instead.

===8<===
From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Wed, 12 Jun 2024 10:57:26 -0400
Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 9475dce7dc..706cee1b69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
     case MIGRATION_STATUS_COMPLETED:
     case MIGRATION_STATUS_FAILED:
         return true;
-    case MIGRATION_STATUS_SETUP:
-    case MIGRATION_STATUS_CANCELLING:
-    case MIGRATION_STATUS_ACTIVE:
-    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-    case MIGRATION_STATUS_COLO:
-    case MIGRATION_STATUS_PRE_SWITCHOVER:
-    case MIGRATION_STATUS_DEVICE:
-    case MIGRATION_STATUS_WAIT_UNPLUG:
-        return false;
     default:
-        g_assert_not_reached();
+        return false;
     }
-
-    return false;
 }
 
 bool migration_is_active(void)
-- 
2.45.0

===8<===

-- 
Peter Xu

Re: [PATCH 3/4] migration: Use MigrationStatus instead of int

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
>> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
>>      }
>>  }
>>  
>> -bool migration_postcopy_is_alive(int state)
>> +bool migration_postcopy_is_alive(MigrationStatus state)
>>  {
>>      switch (state) {
>>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
>>      case MIGRATION_STATUS_DEVICE:
>>      case MIGRATION_STATUS_WAIT_UNPLUG:
>>          return false;
>> -    case MIGRATION_STATUS__MAX:
>> +    default:
>>          g_assert_not_reached();
>>      }
>
> This survives the tests, but I just found that it's risky, as it's not
> covering all the states..
>
> I'll squash below when I send v2 instead.
>
> ===8<===
> From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Wed, 12 Jun 2024 10:57:26 -0400
> Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 9475dce7dc..706cee1b69 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
>      case MIGRATION_STATUS_COMPLETED:
>      case MIGRATION_STATUS_FAILED:
>          return true;
> -    case MIGRATION_STATUS_SETUP:
> -    case MIGRATION_STATUS_CANCELLING:
> -    case MIGRATION_STATUS_ACTIVE:
> -    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> -    case MIGRATION_STATUS_COLO:
> -    case MIGRATION_STATUS_PRE_SWITCHOVER:
> -    case MIGRATION_STATUS_DEVICE:
> -    case MIGRATION_STATUS_WAIT_UNPLUG:
> -        return false;
>      default:
> -        g_assert_not_reached();
> +        return false;
>      }

Unrelated, but should we consider POSTCOPY_PAUSED as idle?

> -
> -    return false;
>  }
>  
>  bool migration_is_active(void)
> -- 
> 2.45.0
>
> ===8<===

Re: [PATCH 3/4] migration: Use MigrationStatus instead of int

[Peter Xu]

On Thu, Jun 13, 2024 at 11:59:20AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
> >> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
> >>      }
> >>  }
> >>  
> >> -bool migration_postcopy_is_alive(int state)
> >> +bool migration_postcopy_is_alive(MigrationStatus state)
> >>  {
> >>      switch (state) {
> >>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
> >>      case MIGRATION_STATUS_DEVICE:
> >>      case MIGRATION_STATUS_WAIT_UNPLUG:
> >>          return false;
> >> -    case MIGRATION_STATUS__MAX:
> >> +    default:
> >>          g_assert_not_reached();
> >>      }
> >
> > This survives the tests, but I just found that it's risky, as it's not
> > covering all the states..
> >
> > I'll squash below when I send v2 instead.
> >
> > ===8<===
> > From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Wed, 12 Jun 2024 10:57:26 -0400
> > Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 9475dce7dc..706cee1b69 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
> >      case MIGRATION_STATUS_COMPLETED:
> >      case MIGRATION_STATUS_FAILED:
> >          return true;
> > -    case MIGRATION_STATUS_SETUP:
> > -    case MIGRATION_STATUS_CANCELLING:
> > -    case MIGRATION_STATUS_ACTIVE:
> > -    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > -    case MIGRATION_STATUS_COLO:
> > -    case MIGRATION_STATUS_PRE_SWITCHOVER:
> > -    case MIGRATION_STATUS_DEVICE:
> > -    case MIGRATION_STATUS_WAIT_UNPLUG:
> > -        return false;
> >      default:
> > -        g_assert_not_reached();
> > +        return false;
> >      }
> 
> Unrelated, but should we consider POSTCOPY_PAUSED as idle?

I think not; idle should be about "whether a migration is in progress" in
verbose, IMHO.

E.g. see migrate_add_blocker_modes() -> is_busy(), otherwise we'll allow
changing migration blockers when postcopy is paused.

PS: offtopic, but.. "is_busy" may deserve some better name in the future..

-- 
Peter Xu

[PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Peter Xu]

This patch adds a migration state on src called "postcopy-recover-setup".
The new state will describe the intermediate step starting from when the
src QEMU started an postcopy recovery request, until the migration channels
are properly established, but before the recovery process take place.

The request came from Libvirt where Libvirt currently rely on the migration
state events to detect migration state changes.  That works for most of the
migration process but except postcopy recovery failures at the beginning.

Currently postcopy recovery only has two major states:

  - postcopy-paused: this is the state that both sides of QEMU will be in
    for a long time as long as the migration channel was interrupted.

  - postcopy-recover: this is the state where both sides of QEMU handshake
    with each other, preparing for a continuation of postcopy which used to
    be interrupted.

The issue here is when the recovery port is invalid, the src QEMU will take
the URI/channels, noticing the ports are not valid, and it'll silently keep
in the postcopy-paused state, with no event sent to Libvirt.  In this case,
the only thing Libvirt can do is to poll the migration status with a proper
interval, however that's less optimal.

Considering that this is the only case where Libvirt won't get a
notification from QEMU on such events, let's add postcopy-recover-setup
state to mimic what we used to have with the "setup" state of a newly
initialized migration, describing the phase of connection establishment.

With that, postcopy recovery will have two paths to go now, and either path
will guarantee an event generated.  Now the events will look like this
during a recovery process on src QEMU:

  - Initially when the recovery is initiated on src, QEMU will go from
    "postcopy-paused" -> "postcopy-recover-setup".  Old QEMUs don't have
    this event.

  - Depending on whether the channel re-establishment is succeeded:

    - In succeeded case, src QEMU will move from "postcopy-recover-setup"
      to "postcopy-recover".  Old QEMUs also have this event.

    - In failure case, src QEMU will move from "postcopy-recover-setup" to
      "postcopy-paused" again.  Old QEMUs don't have this event.

This guarantees that Libvirt will always receive a notification for
recovery process properly.

One thing to mention is, such new status is only needed on src QEMU not
both.  On dest QEMU, the state machine doesn't change.  Hence the events
don't change either.  It's done like so because dest QEMU may not have an
explicit point of setup start.  E.g., it can happen that when dest QEMUs
doesn't use migrate-recover command to use a new URI/channel, but the old
URI/channels can be reused in recovery, in which case the old ports simply
can work again after the network routes are fixed up.

The patch has some touch-ups in the dest path too, but it's because there's
some unclearness on using migrate_set_state(), so the change should make it
crystal clear now by checking current status always.  The next step from
that POV would be making migrate_set_state() not using cmpxchg() but always
update the status, but that's for later.

Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Cc: Prasad Pandit <ppandit@redhat.com>
Buglink: https://issues.redhat.com/browse/RHEL-38485
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi/migration.json      |  4 +++
 migration/postcopy-ram.h |  3 ++
 migration/migration.c    | 66 +++++++++++++++++++++++++++++++++++-----
 migration/postcopy-ram.c |  6 ++++
 migration/savevm.c       |  4 +--
 5 files changed, 73 insertions(+), 10 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index a351fd3714..a135bbcd96 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -142,6 +142,9 @@
 #
 # @postcopy-paused: during postcopy but paused.  (since 3.0)
 #
+# @postcopy-recover-setup: setup phase for a postcopy recover process,
+#     preparing for a recover phase to start.  (since 9.1)
+#
 # @postcopy-recover: trying to recover from a paused postcopy.  (since
 #     3.0)
 #
@@ -166,6 +169,7 @@
 { 'enum': 'MigrationStatus',
   'data': [ 'none', 'setup', 'cancelling', 'cancelled',
             'active', 'postcopy-active', 'postcopy-paused',
+            'postcopy-recover-setup',
             'postcopy-recover', 'completed', 'failed', 'colo',
             'pre-switchover', 'device', 'wait-unplug' ] }
 ##
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index ecae941211..a6df1b2811 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -13,6 +13,8 @@
 #ifndef QEMU_POSTCOPY_RAM_H
 #define QEMU_POSTCOPY_RAM_H
 
+#include "qapi/qapi-types-migration.h"
+
 /* Return true if the host supports everything we need to do postcopy-ram */
 bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
                                     Error **errp);
@@ -193,5 +195,6 @@ enum PostcopyChannels {
 void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
 void postcopy_preempt_setup(MigrationState *s);
 int postcopy_preempt_establish_channel(MigrationState *s);
+bool postcopy_is_paused(MigrationStatus status);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index bfbd657035..9475dce7dc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -595,6 +595,26 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
     return true;
 }
 
+static bool
+migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
+{
+    MigrationStatus current = mis->state;
+
+    if (current == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+        /* Postcopy paused state doesn't change when setup new ports */
+        return true;
+    }
+
+    if (current != MIGRATION_STATUS_NONE) {
+        error_setg(errp, "Illegal migration incoming state: %s",
+                   MigrationStatus_str(current));
+        return false;
+    }
+
+    migrate_set_state(&mis->state, current, MIGRATION_STATUS_SETUP);
+    return true;
+}
+
 static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           MigrationChannelList *channels,
                                           Error **errp)
@@ -633,8 +653,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
         return;
     }
 
-    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
-                      MIGRATION_STATUS_SETUP);
+    if (!migration_incoming_state_setup(mis, errp)) {
+        return;
+    }
 
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         SocketAddress *saddr = &addr->u.socket;
@@ -1070,6 +1091,7 @@ bool migration_is_setup_or_active(void)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
     case MIGRATION_STATUS_POSTCOPY_RECOVER:
     case MIGRATION_STATUS_SETUP:
     case MIGRATION_STATUS_PRE_SWITCHOVER:
@@ -1092,6 +1114,7 @@ bool migration_is_running(void)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
     case MIGRATION_STATUS_POSTCOPY_RECOVER:
     case MIGRATION_STATUS_SETUP:
     case MIGRATION_STATUS_PRE_SWITCHOVER:
@@ -1229,6 +1252,7 @@ static void fill_source_migration_info(MigrationInfo *info)
     case MIGRATION_STATUS_PRE_SWITCHOVER:
     case MIGRATION_STATUS_DEVICE:
     case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
     case MIGRATION_STATUS_POSTCOPY_RECOVER:
         /* TODO add some postcopy stats */
         populate_time_info(info, s);
@@ -1279,6 +1303,7 @@ static void fill_destination_migration_info(MigrationInfo *info)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
     case MIGRATION_STATUS_POSTCOPY_RECOVER:
     case MIGRATION_STATUS_FAILED:
     case MIGRATION_STATUS_COLO:
@@ -1435,9 +1460,30 @@ static void migrate_error_free(MigrationState *s)
 
 static void migrate_fd_error(MigrationState *s, const Error *error)
 {
+    MigrationStatus current = s->state;
+    MigrationStatus next;
+
     assert(s->to_dst_file == NULL);
-    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                      MIGRATION_STATUS_FAILED);
+
+    switch (current) {
+    case MIGRATION_STATUS_SETUP:
+        next = MIGRATION_STATUS_FAILED;
+        break;
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
+        /* Never fail a postcopy migration; switch back to PAUSED instead */
+        next = MIGRATION_STATUS_POSTCOPY_PAUSED;
+        break;
+    default:
+        /*
+         * This really shouldn't happen. Just be careful to not crash a VM
+         * just for this.  Instead, dump something.
+         */
+        error_report("%s: Illegal migration status (%s) detected",
+                     __func__, MigrationStatus_str(current));
+        return;
+    }
+
+    migrate_set_state(&s->state, current, next);
     migrate_set_error(s, error);
 }
 
@@ -1538,6 +1584,7 @@ bool migration_in_postcopy(void)
     switch (s->state) {
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_PAUSED:
+    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
     case MIGRATION_STATUS_POSTCOPY_RECOVER:
         return true;
     default:
@@ -1936,6 +1983,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
             return false;
         }
 
+        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
+                          MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
+
         /* This is a resume, skip init status */
         return true;
     }
@@ -2968,9 +3018,9 @@ static MigThrError postcopy_pause(MigrationState *s)
          * We wait until things fixed up. Then someone will setup the
          * status back for us.
          */
-        while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+        do {
             qemu_sem_wait(&s->postcopy_pause_sem);
-        }
+        } while (postcopy_is_paused(s->state));
 
         if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
             /* Woken up by a recover procedure. Give it a shot */
@@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
 {
     Error *local_err = NULL;
     uint64_t rate_limit;
-    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
+    bool resume = migration_in_postcopy();
     int ret;
 
     /*
@@ -3733,7 +3783,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
 
     if (resume) {
         /* Wakeup the main migration thread to do the recovery */
-        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
+        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
                           MIGRATION_STATUS_POSTCOPY_RECOVER);
         qemu_sem_post(&s->postcopy_pause_sem);
         return;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 97701e6bb2..1c374b7ea1 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1770,3 +1770,9 @@ void *postcopy_preempt_thread(void *opaque)
 
     return NULL;
 }
+
+bool postcopy_is_paused(MigrationStatus status)
+{
+    return status == MIGRATION_STATUS_POSTCOPY_PAUSED ||
+        status == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
+}
diff --git a/migration/savevm.c b/migration/savevm.c
index e71410d8c1..deb57833f8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2864,9 +2864,9 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
     error_report("Detected IO failure for postcopy. "
                  "Migration paused.");
 
-    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+    do {
         qemu_sem_wait(&mis->postcopy_pause_sem_dst);
-    }
+    } while (postcopy_is_paused(mis->state));
 
     trace_postcopy_pause_incoming_continued();
 
-- 
2.45.0

Re: [PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> This patch adds a migration state on src called "postcopy-recover-setup".
> The new state will describe the intermediate step starting from when the
> src QEMU started an postcopy recovery request, until the migration channels
> are properly established, but before the recovery process take place.
>
> The request came from Libvirt where Libvirt currently rely on the migration
> state events to detect migration state changes.  That works for most of the
> migration process but except postcopy recovery failures at the beginning.
>
> Currently postcopy recovery only has two major states:
>
>   - postcopy-paused: this is the state that both sides of QEMU will be in
>     for a long time as long as the migration channel was interrupted.
>
>   - postcopy-recover: this is the state where both sides of QEMU handshake
>     with each other, preparing for a continuation of postcopy which used to
>     be interrupted.
>
> The issue here is when the recovery port is invalid, the src QEMU will take
> the URI/channels, noticing the ports are not valid, and it'll silently keep
> in the postcopy-paused state, with no event sent to Libvirt.  In this case,
> the only thing Libvirt can do is to poll the migration status with a proper
> interval, however that's less optimal.
>
> Considering that this is the only case where Libvirt won't get a
> notification from QEMU on such events, let's add postcopy-recover-setup
> state to mimic what we used to have with the "setup" state of a newly

s/used to //

> initialized migration, describing the phase of connection establishment.
>
> With that, postcopy recovery will have two paths to go now, and either path
> will guarantee an event generated.  Now the events will look like this
> during a recovery process on src QEMU:
>
>   - Initially when the recovery is initiated on src, QEMU will go from
>     "postcopy-paused" -> "postcopy-recover-setup".  Old QEMUs don't have
>     this event.
>
>   - Depending on whether the channel re-establishment is succeeded:
>
>     - In succeeded case, src QEMU will move from "postcopy-recover-setup"
>       to "postcopy-recover".  Old QEMUs also have this event.
>
>     - In failure case, src QEMU will move from "postcopy-recover-setup" to
>       "postcopy-paused" again.  Old QEMUs don't have this event.
>
> This guarantees that Libvirt will always receive a notification for
> recovery process properly.
>
> One thing to mention is, such new status is only needed on src QEMU not
> both.  On dest QEMU, the state machine doesn't change.  Hence the events
> don't change either.  It's done like so because dest QEMU may not have an
> explicit point of setup start.  E.g., it can happen that when dest QEMUs
> doesn't use migrate-recover command to use a new URI/channel, but the old
> URI/channels can be reused in recovery, in which case the old ports simply
> can work again after the network routes are fixed up.
>
> The patch has some touch-ups in the dest path too, but it's because there's
> some unclearness on using migrate_set_state(), so the change should make it
> crystal clear now by checking current status always.  The next step from

Can we get a separate patch for these cleanups?

> that POV would be making migrate_set_state() not using cmpxchg() but always
> update the status, but that's for later.
>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Prasad Pandit <ppandit@redhat.com>
> Buglink: https://issues.redhat.com/browse/RHEL-38485
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi/migration.json      |  4 +++
>  migration/postcopy-ram.h |  3 ++
>  migration/migration.c    | 66 +++++++++++++++++++++++++++++++++++-----
>  migration/postcopy-ram.c |  6 ++++
>  migration/savevm.c       |  4 +--
>  5 files changed, 73 insertions(+), 10 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index a351fd3714..a135bbcd96 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -142,6 +142,9 @@
>  #
>  # @postcopy-paused: during postcopy but paused.  (since 3.0)
>  #
> +# @postcopy-recover-setup: setup phase for a postcopy recover process,
> +#     preparing for a recover phase to start.  (since 9.1)

recover*y* process
recover*y* phase

> +#
>  # @postcopy-recover: trying to recover from a paused postcopy.  (since
>  #     3.0)
>  #
> @@ -166,6 +169,7 @@
>  { 'enum': 'MigrationStatus',
>    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
>              'active', 'postcopy-active', 'postcopy-paused',
> +            'postcopy-recover-setup',
>              'postcopy-recover', 'completed', 'failed', 'colo',
>              'pre-switchover', 'device', 'wait-unplug' ] }
>  ##
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index ecae941211..a6df1b2811 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_POSTCOPY_RAM_H
>  #define QEMU_POSTCOPY_RAM_H
>  
> +#include "qapi/qapi-types-migration.h"
> +
>  /* Return true if the host supports everything we need to do postcopy-ram */
>  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
>                                      Error **errp);
> @@ -193,5 +195,6 @@ enum PostcopyChannels {
>  void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
>  void postcopy_preempt_setup(MigrationState *s);
>  int postcopy_preempt_establish_channel(MigrationState *s);
> +bool postcopy_is_paused(MigrationStatus status);
>  
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index bfbd657035..9475dce7dc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -595,6 +595,26 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
>      return true;
>  }
>  
> +static bool
> +migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
> +{
> +    MigrationStatus current = mis->state;
> +
> +    if (current == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +        /* Postcopy paused state doesn't change when setup new ports */

The "setup new ports" part is a bit vague. Maybe:

/*
 * The SETUP state only happens at the start of migration. A postcopy
 * migration recovery migration stays in POSTCOPY_PAUSED.
 */

> +        return true;
> +    }
> +
> +    if (current != MIGRATION_STATUS_NONE) {
> +        error_setg(errp, "Illegal migration incoming state: %s",
> +                   MigrationStatus_str(current));
> +        return false;
> +    }

This is a good candidate for a separate patch due to the extra change in
behavior not necessarily related to postcopy.

> +
> +    migrate_set_state(&mis->state, current, MIGRATION_STATUS_SETUP);
> +    return true;
> +}
> +
>  static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>                                            MigrationChannelList *channels,
>                                            Error **errp)
> @@ -633,8 +653,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>          return;
>      }
>  
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> -                      MIGRATION_STATUS_SETUP);
> +    if (!migration_incoming_state_setup(mis, errp)) {
> +        return;
> +    }
>  
>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>          SocketAddress *saddr = &addr->u.socket;
> @@ -1070,6 +1091,7 @@ bool migration_is_setup_or_active(void)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_SETUP:
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
> @@ -1092,6 +1114,7 @@ bool migration_is_running(void)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_SETUP:
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
> @@ -1229,6 +1252,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_PRE_SWITCHOVER:
>      case MIGRATION_STATUS_DEVICE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>          /* TODO add some postcopy stats */
>          populate_time_info(info, s);
> @@ -1279,6 +1303,7 @@ static void fill_destination_migration_info(MigrationInfo *info)
>      case MIGRATION_STATUS_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:

Does this need to be here? We don't reach this state on destination, right?

>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>      case MIGRATION_STATUS_FAILED:
>      case MIGRATION_STATUS_COLO:
> @@ -1435,9 +1460,30 @@ static void migrate_error_free(MigrationState *s)
>  
>  static void migrate_fd_error(MigrationState *s, const Error *error)
>  {

The default case of the swtich below is a bit surprising to me. Why
wouldn't we allow this function to be called from other places to set
STATUS_FAILED?

...unless this is only mean for the connection phase, so:

just to check your understanding here because it seems we've drifted a
bit from the original definition on those, specially with
migrate_fd_cleanup(), but does this _fd_ in the function name implies
something like a "connection phase"? As in, "connect to the fd", "the fd
connection errored out" and "cleanup the fd connection". Maybe it's time
to switch this "fd" to something clearer...

> +    MigrationStatus current = s->state;
> +    MigrationStatus next;
> +
>      assert(s->to_dst_file == NULL);
> -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                      MIGRATION_STATUS_FAILED);
> +
> +    switch (current) {
> +    case MIGRATION_STATUS_SETUP:
> +        next = MIGRATION_STATUS_FAILED;
> +        break;
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> +        /* Never fail a postcopy migration; switch back to PAUSED instead */
> +        next = MIGRATION_STATUS_POSTCOPY_PAUSED;

So presumably we can keep recovering the migration indefinitely?

> +        break;
> +    default:
> +        /*
> +         * This really shouldn't happen. Just be careful to not crash a VM
> +         * just for this.  Instead, dump something.
> +         */
> +        error_report("%s: Illegal migration status (%s) detected",
> +                     __func__, MigrationStatus_str(current));
> +        return;
> +    }
> +
> +    migrate_set_state(&s->state, current, next);
>      migrate_set_error(s, error);
>  }
>  
> @@ -1538,6 +1584,7 @@ bool migration_in_postcopy(void)
>      switch (s->state) {
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>          return true;
>      default:
> @@ -1936,6 +1983,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>              return false;
>          }
>  
> +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> +                          MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
> +
>          /* This is a resume, skip init status */
>          return true;
>      }
> @@ -2968,9 +3018,9 @@ static MigThrError postcopy_pause(MigrationState *s)
>           * We wait until things fixed up. Then someone will setup the
>           * status back for us.
>           */
> -        while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +        do {
>              qemu_sem_wait(&s->postcopy_pause_sem);
> -        }
> +        } while (postcopy_is_paused(s->state));

Is there a particular reason to go from while() to do{}while()?

>  
>          if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
>              /* Woken up by a recover procedure. Give it a shot */
> @@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>  {
>      Error *local_err = NULL;
>      uint64_t rate_limit;
> -    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
> +    bool resume = migration_in_postcopy();

Here you're expecting just PAUSED or RECOVER_SETUP, right? We'll not
reach here in any of the other postcopy states.

>      int ret;
>  
>      /*
> @@ -3733,7 +3783,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>  
>      if (resume) {
>          /* Wakeup the main migration thread to do the recovery */
> -        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
>                            MIGRATION_STATUS_POSTCOPY_RECOVER);
>          qemu_sem_post(&s->postcopy_pause_sem);
>          return;
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 97701e6bb2..1c374b7ea1 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1770,3 +1770,9 @@ void *postcopy_preempt_thread(void *opaque)
>  
>      return NULL;
>  }
> +
> +bool postcopy_is_paused(MigrationStatus status)
> +{
> +    return status == MIGRATION_STATUS_POSTCOPY_PAUSED ||
> +        status == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
> +}
> diff --git a/migration/savevm.c b/migration/savevm.c
> index e71410d8c1..deb57833f8 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2864,9 +2864,9 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>      error_report("Detected IO failure for postcopy. "
>                   "Migration paused.");
>  
> -    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> +    do {
>          qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> -    }
> +    } while (postcopy_is_paused(mis->state));
>  
>      trace_postcopy_pause_incoming_continued();

Re: [PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Peter Xu]

On Thu, Jun 13, 2024 at 11:51:58AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > This patch adds a migration state on src called "postcopy-recover-setup".
> > The new state will describe the intermediate step starting from when the
> > src QEMU started an postcopy recovery request, until the migration channels
> > are properly established, but before the recovery process take place.
> >
> > The request came from Libvirt where Libvirt currently rely on the migration
> > state events to detect migration state changes.  That works for most of the
> > migration process but except postcopy recovery failures at the beginning.
> >
> > Currently postcopy recovery only has two major states:
> >
> >   - postcopy-paused: this is the state that both sides of QEMU will be in
> >     for a long time as long as the migration channel was interrupted.
> >
> >   - postcopy-recover: this is the state where both sides of QEMU handshake
> >     with each other, preparing for a continuation of postcopy which used to
> >     be interrupted.
> >
> > The issue here is when the recovery port is invalid, the src QEMU will take
> > the URI/channels, noticing the ports are not valid, and it'll silently keep
> > in the postcopy-paused state, with no event sent to Libvirt.  In this case,
> > the only thing Libvirt can do is to poll the migration status with a proper
> > interval, however that's less optimal.
> >
> > Considering that this is the only case where Libvirt won't get a
> > notification from QEMU on such events, let's add postcopy-recover-setup
> > state to mimic what we used to have with the "setup" state of a newly
> 
> s/used to //

Fixed.

> 
> > initialized migration, describing the phase of connection establishment.
> >
> > With that, postcopy recovery will have two paths to go now, and either path
> > will guarantee an event generated.  Now the events will look like this
> > during a recovery process on src QEMU:
> >
> >   - Initially when the recovery is initiated on src, QEMU will go from
> >     "postcopy-paused" -> "postcopy-recover-setup".  Old QEMUs don't have
> >     this event.
> >
> >   - Depending on whether the channel re-establishment is succeeded:
> >
> >     - In succeeded case, src QEMU will move from "postcopy-recover-setup"
> >       to "postcopy-recover".  Old QEMUs also have this event.
> >
> >     - In failure case, src QEMU will move from "postcopy-recover-setup" to
> >       "postcopy-paused" again.  Old QEMUs don't have this event.
> >
> > This guarantees that Libvirt will always receive a notification for
> > recovery process properly.
> >
> > One thing to mention is, such new status is only needed on src QEMU not
> > both.  On dest QEMU, the state machine doesn't change.  Hence the events
> > don't change either.  It's done like so because dest QEMU may not have an
> > explicit point of setup start.  E.g., it can happen that when dest QEMUs
> > doesn't use migrate-recover command to use a new URI/channel, but the old
> > URI/channels can be reused in recovery, in which case the old ports simply
> > can work again after the network routes are fixed up.
> >
> > The patch has some touch-ups in the dest path too, but it's because there's
> > some unclearness on using migrate_set_state(), so the change should make it
> > crystal clear now by checking current status always.  The next step from
> 
> Can we get a separate patch for these cleanups?

We can, and I'll do.

> 
> > that POV would be making migrate_set_state() not using cmpxchg() but always
> > update the status, but that's for later.
> >
> > Cc: Jiri Denemark <jdenemar@redhat.com>
> > Cc: Fabiano Rosas <farosas@suse.de>
> > Cc: Prasad Pandit <ppandit@redhat.com>
> > Buglink: https://issues.redhat.com/browse/RHEL-38485
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qapi/migration.json      |  4 +++
> >  migration/postcopy-ram.h |  3 ++
> >  migration/migration.c    | 66 +++++++++++++++++++++++++++++++++++-----
> >  migration/postcopy-ram.c |  6 ++++
> >  migration/savevm.c       |  4 +--
> >  5 files changed, 73 insertions(+), 10 deletions(-)
> >
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index a351fd3714..a135bbcd96 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -142,6 +142,9 @@
> >  #
> >  # @postcopy-paused: during postcopy but paused.  (since 3.0)
> >  #
> > +# @postcopy-recover-setup: setup phase for a postcopy recover process,
> > +#     preparing for a recover phase to start.  (since 9.1)
> 
> recover*y* process
> recover*y* phase

OK.

> 
> > +#
> >  # @postcopy-recover: trying to recover from a paused postcopy.  (since
> >  #     3.0)
> >  #
> > @@ -166,6 +169,7 @@
> >  { 'enum': 'MigrationStatus',
> >    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
> >              'active', 'postcopy-active', 'postcopy-paused',
> > +            'postcopy-recover-setup',
> >              'postcopy-recover', 'completed', 'failed', 'colo',
> >              'pre-switchover', 'device', 'wait-unplug' ] }
> >  ##
> > diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> > index ecae941211..a6df1b2811 100644
> > --- a/migration/postcopy-ram.h
> > +++ b/migration/postcopy-ram.h
> > @@ -13,6 +13,8 @@
> >  #ifndef QEMU_POSTCOPY_RAM_H
> >  #define QEMU_POSTCOPY_RAM_H
> >  
> > +#include "qapi/qapi-types-migration.h"
> > +
> >  /* Return true if the host supports everything we need to do postcopy-ram */
> >  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
> >                                      Error **errp);
> > @@ -193,5 +195,6 @@ enum PostcopyChannels {
> >  void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
> >  void postcopy_preempt_setup(MigrationState *s);
> >  int postcopy_preempt_establish_channel(MigrationState *s);
> > +bool postcopy_is_paused(MigrationStatus status);
> >  
> >  #endif
> > diff --git a/migration/migration.c b/migration/migration.c
> > index bfbd657035..9475dce7dc 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -595,6 +595,26 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
> >      return true;
> >  }
> >  
> > +static bool
> > +migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
> > +{
> > +    MigrationStatus current = mis->state;
> > +
> > +    if (current == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +        /* Postcopy paused state doesn't change when setup new ports */
> 
> The "setup new ports" part is a bit vague. Maybe:
> 
> /*
>  * The SETUP state only happens at the start of migration. A postcopy
>  * migration recovery migration stays in POSTCOPY_PAUSED.
>  */

Mentioning SETUP in this if clause might be slightly confusing?  How about
I only keep the latter sentence (and change it a bit; there's one extra
"migration")?

  /*
   * Postcopy migration stays in POSTCOPY_PAUSED even if reconnection
   * happens.
   */

> 
> > +        return true;
> > +    }
> > +
> > +    if (current != MIGRATION_STATUS_NONE) {
> > +        error_setg(errp, "Illegal migration incoming state: %s",
> > +                   MigrationStatus_str(current));
> > +        return false;
> > +    }
> 
> This is a good candidate for a separate patch due to the extra change in
> behavior not necessarily related to postcopy.

I'll split that, but just to mention, I think there should have no
functional change, or we're doomed.

The old code worked the same by ignoring migrate_set_state() when PAUSED in
the cmpxchg(), which is implicit and unclear.

> 
> > +
> > +    migrate_set_state(&mis->state, current, MIGRATION_STATUS_SETUP);
> > +    return true;
> > +}
> > +
> >  static void qemu_start_incoming_migration(const char *uri, bool has_channels,
> >                                            MigrationChannelList *channels,
> >                                            Error **errp)
> > @@ -633,8 +653,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
> >          return;
> >      }
> >  
> > -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> > -                      MIGRATION_STATUS_SETUP);
> > +    if (!migration_incoming_state_setup(mis, errp)) {
> > +        return;
> > +    }
> >  
> >      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> >          SocketAddress *saddr = &addr->u.socket;
> > @@ -1070,6 +1091,7 @@ bool migration_is_setup_or_active(void)
> >      case MIGRATION_STATUS_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
> >      case MIGRATION_STATUS_SETUP:
> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
> > @@ -1092,6 +1114,7 @@ bool migration_is_running(void)
> >      case MIGRATION_STATUS_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
> >      case MIGRATION_STATUS_SETUP:
> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
> > @@ -1229,6 +1252,7 @@ static void fill_source_migration_info(MigrationInfo *info)
> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
> >      case MIGRATION_STATUS_DEVICE:
> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
> >          /* TODO add some postcopy stats */
> >          populate_time_info(info, s);
> > @@ -1279,6 +1303,7 @@ static void fill_destination_migration_info(MigrationInfo *info)
> >      case MIGRATION_STATUS_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> 
> Does this need to be here? We don't reach this state on destination, right?

Right, it won't hurt having that here, but let me drop it.

> 
> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
> >      case MIGRATION_STATUS_FAILED:
> >      case MIGRATION_STATUS_COLO:
> > @@ -1435,9 +1460,30 @@ static void migrate_error_free(MigrationState *s)
> >  
> >  static void migrate_fd_error(MigrationState *s, const Error *error)
> >  {
> 
> The default case of the swtich below is a bit surprising to me. Why
> wouldn't we allow this function to be called from other places to set
> STATUS_FAILED?
> 
> ...unless this is only mean for the connection phase, so:
> 
> just to check your understanding here because it seems we've drifted a
> bit from the original definition on those, specially with
> migrate_fd_cleanup(), but does this _fd_ in the function name implies
> something like a "connection phase"? As in, "connect to the fd", "the fd
> connection errored out" and "cleanup the fd connection". Maybe it's time
> to switch this "fd" to something clearer...

IIUC it's just that many functions around this area used to be called
migrate_fd_*().  Let me know if you have some suggestions, though.

> 
> > +    MigrationStatus current = s->state;
> > +    MigrationStatus next;
> > +
> >      assert(s->to_dst_file == NULL);
> > -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > -                      MIGRATION_STATUS_FAILED);
> > +
> > +    switch (current) {
> > +    case MIGRATION_STATUS_SETUP:
> > +        next = MIGRATION_STATUS_FAILED;
> > +        break;
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> > +        /* Never fail a postcopy migration; switch back to PAUSED instead */
> > +        next = MIGRATION_STATUS_POSTCOPY_PAUSED;
> 
> So presumably we can keep recovering the migration indefinitely?

Yes, that's by design.

> 
> > +        break;
> > +    default:
> > +        /*
> > +         * This really shouldn't happen. Just be careful to not crash a VM
> > +         * just for this.  Instead, dump something.
> > +         */
> > +        error_report("%s: Illegal migration status (%s) detected",
> > +                     __func__, MigrationStatus_str(current));
> > +        return;
> > +    }
> > +
> > +    migrate_set_state(&s->state, current, next);
> >      migrate_set_error(s, error);
> >  }
> >  
> > @@ -1538,6 +1584,7 @@ bool migration_in_postcopy(void)
> >      switch (s->state) {
> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
> >          return true;
> >      default:
> > @@ -1936,6 +1983,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
> >              return false;
> >          }
> >  
> > +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> > +                          MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
> > +
> >          /* This is a resume, skip init status */
> >          return true;
> >      }
> > @@ -2968,9 +3018,9 @@ static MigThrError postcopy_pause(MigrationState *s)
> >           * We wait until things fixed up. Then someone will setup the
> >           * status back for us.
> >           */
> > -        while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +        do {
> >              qemu_sem_wait(&s->postcopy_pause_sem);
> > -        }
> > +        } while (postcopy_is_paused(s->state));
> 
> Is there a particular reason to go from while() to do{}while()?

It'll be the same AFAICT, the 1st check should mostly be meaningless as the
sem must be posted at least once.  Yes I should have had a comment
somewhere but I didn't.. if you want me to keep the old way it's also fine,
isn't a big deal here to check first either, just save some cycles when I
worked on these lines.

> 
> >  
> >          if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
> >              /* Woken up by a recover procedure. Give it a shot */
> > @@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> >  {
> >      Error *local_err = NULL;
> >      uint64_t rate_limit;
> > -    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
> > +    bool resume = migration_in_postcopy();
> 
> Here you're expecting just PAUSED or RECOVER_SETUP, right? We'll not
> reach here in any of the other postcopy states.

I think here it must be RECOVER_SETUP after this patch.  I changed it to
use the helper as I think that's cleaner (precopy doesn't allow resume),
and we don't need such change if the state machine trivially changes again.

> 
> >      int ret;
> >  
> >      /*
> > @@ -3733,7 +3783,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> >  
> >      if (resume) {
> >          /* Wakeup the main migration thread to do the recovery */
> > -        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
> > +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
> >                            MIGRATION_STATUS_POSTCOPY_RECOVER);
> >          qemu_sem_post(&s->postcopy_pause_sem);
> >          return;
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 97701e6bb2..1c374b7ea1 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -1770,3 +1770,9 @@ void *postcopy_preempt_thread(void *opaque)
> >  
> >      return NULL;
> >  }
> > +
> > +bool postcopy_is_paused(MigrationStatus status)
> > +{
> > +    return status == MIGRATION_STATUS_POSTCOPY_PAUSED ||
> > +        status == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
> > +}
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index e71410d8c1..deb57833f8 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2864,9 +2864,9 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> >      error_report("Detected IO failure for postcopy. "
> >                   "Migration paused.");
> >  
> > -    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +    do {
> >          qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > -    }
> > +    } while (postcopy_is_paused(mis->state));
> >  
> >      trace_postcopy_pause_incoming_continued();
> 

-- 
Peter Xu

Re: [PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jun 13, 2024 at 11:51:58AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > This patch adds a migration state on src called "postcopy-recover-setup".
>> > The new state will describe the intermediate step starting from when the
>> > src QEMU started an postcopy recovery request, until the migration channels
>> > are properly established, but before the recovery process take place.
>> >
>> > The request came from Libvirt where Libvirt currently rely on the migration
>> > state events to detect migration state changes.  That works for most of the
>> > migration process but except postcopy recovery failures at the beginning.
>> >
>> > Currently postcopy recovery only has two major states:
>> >
>> >   - postcopy-paused: this is the state that both sides of QEMU will be in
>> >     for a long time as long as the migration channel was interrupted.
>> >
>> >   - postcopy-recover: this is the state where both sides of QEMU handshake
>> >     with each other, preparing for a continuation of postcopy which used to
>> >     be interrupted.
>> >
>> > The issue here is when the recovery port is invalid, the src QEMU will take
>> > the URI/channels, noticing the ports are not valid, and it'll silently keep
>> > in the postcopy-paused state, with no event sent to Libvirt.  In this case,
>> > the only thing Libvirt can do is to poll the migration status with a proper
>> > interval, however that's less optimal.
>> >
>> > Considering that this is the only case where Libvirt won't get a
>> > notification from QEMU on such events, let's add postcopy-recover-setup
>> > state to mimic what we used to have with the "setup" state of a newly
>> 
>> s/used to //
>
> Fixed.
>
>> 
>> > initialized migration, describing the phase of connection establishment.
>> >
>> > With that, postcopy recovery will have two paths to go now, and either path
>> > will guarantee an event generated.  Now the events will look like this
>> > during a recovery process on src QEMU:
>> >
>> >   - Initially when the recovery is initiated on src, QEMU will go from
>> >     "postcopy-paused" -> "postcopy-recover-setup".  Old QEMUs don't have
>> >     this event.
>> >
>> >   - Depending on whether the channel re-establishment is succeeded:
>> >
>> >     - In succeeded case, src QEMU will move from "postcopy-recover-setup"
>> >       to "postcopy-recover".  Old QEMUs also have this event.
>> >
>> >     - In failure case, src QEMU will move from "postcopy-recover-setup" to
>> >       "postcopy-paused" again.  Old QEMUs don't have this event.
>> >
>> > This guarantees that Libvirt will always receive a notification for
>> > recovery process properly.
>> >
>> > One thing to mention is, such new status is only needed on src QEMU not
>> > both.  On dest QEMU, the state machine doesn't change.  Hence the events
>> > don't change either.  It's done like so because dest QEMU may not have an
>> > explicit point of setup start.  E.g., it can happen that when dest QEMUs
>> > doesn't use migrate-recover command to use a new URI/channel, but the old
>> > URI/channels can be reused in recovery, in which case the old ports simply
>> > can work again after the network routes are fixed up.
>> >
>> > The patch has some touch-ups in the dest path too, but it's because there's
>> > some unclearness on using migrate_set_state(), so the change should make it
>> > crystal clear now by checking current status always.  The next step from
>> 
>> Can we get a separate patch for these cleanups?
>
> We can, and I'll do.
>
>> 
>> > that POV would be making migrate_set_state() not using cmpxchg() but always
>> > update the status, but that's for later.
>> >
>> > Cc: Jiri Denemark <jdenemar@redhat.com>
>> > Cc: Fabiano Rosas <farosas@suse.de>
>> > Cc: Prasad Pandit <ppandit@redhat.com>
>> > Buglink: https://issues.redhat.com/browse/RHEL-38485
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  qapi/migration.json      |  4 +++
>> >  migration/postcopy-ram.h |  3 ++
>> >  migration/migration.c    | 66 +++++++++++++++++++++++++++++++++++-----
>> >  migration/postcopy-ram.c |  6 ++++
>> >  migration/savevm.c       |  4 +--
>> >  5 files changed, 73 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index a351fd3714..a135bbcd96 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -142,6 +142,9 @@
>> >  #
>> >  # @postcopy-paused: during postcopy but paused.  (since 3.0)
>> >  #
>> > +# @postcopy-recover-setup: setup phase for a postcopy recover process,
>> > +#     preparing for a recover phase to start.  (since 9.1)
>> 
>> recover*y* process
>> recover*y* phase
>
> OK.
>
>> 
>> > +#
>> >  # @postcopy-recover: trying to recover from a paused postcopy.  (since
>> >  #     3.0)
>> >  #
>> > @@ -166,6 +169,7 @@
>> >  { 'enum': 'MigrationStatus',
>> >    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
>> >              'active', 'postcopy-active', 'postcopy-paused',
>> > +            'postcopy-recover-setup',
>> >              'postcopy-recover', 'completed', 'failed', 'colo',
>> >              'pre-switchover', 'device', 'wait-unplug' ] }
>> >  ##
>> > diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
>> > index ecae941211..a6df1b2811 100644
>> > --- a/migration/postcopy-ram.h
>> > +++ b/migration/postcopy-ram.h
>> > @@ -13,6 +13,8 @@
>> >  #ifndef QEMU_POSTCOPY_RAM_H
>> >  #define QEMU_POSTCOPY_RAM_H
>> >  
>> > +#include "qapi/qapi-types-migration.h"
>> > +
>> >  /* Return true if the host supports everything we need to do postcopy-ram */
>> >  bool postcopy_ram_supported_by_host(MigrationIncomingState *mis,
>> >                                      Error **errp);
>> > @@ -193,5 +195,6 @@ enum PostcopyChannels {
>> >  void postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file);
>> >  void postcopy_preempt_setup(MigrationState *s);
>> >  int postcopy_preempt_establish_channel(MigrationState *s);
>> > +bool postcopy_is_paused(MigrationStatus status);
>> >  
>> >  #endif
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index bfbd657035..9475dce7dc 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -595,6 +595,26 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel,
>> >      return true;
>> >  }
>> >  
>> > +static bool
>> > +migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp)
>> > +{
>> > +    MigrationStatus current = mis->state;
>> > +
>> > +    if (current == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>> > +        /* Postcopy paused state doesn't change when setup new ports */
>> 
>> The "setup new ports" part is a bit vague. Maybe:
>> 
>> /*
>>  * The SETUP state only happens at the start of migration. A postcopy
>>  * migration recovery migration stays in POSTCOPY_PAUSED.
>>  */
>
> Mentioning SETUP in this if clause might be slightly confusing?  How about
> I only keep the latter sentence (and change it a bit; there's one extra
> "migration")?
>
>   /*
>    * Postcopy migration stays in POSTCOPY_PAUSED even if reconnection
>    * happens.
>    */

Yep.

>
>> 
>> > +        return true;
>> > +    }
>> > +
>> > +    if (current != MIGRATION_STATUS_NONE) {
>> > +        error_setg(errp, "Illegal migration incoming state: %s",
>> > +                   MigrationStatus_str(current));
>> > +        return false;
>> > +    }
>> 
>> This is a good candidate for a separate patch due to the extra change in
>> behavior not necessarily related to postcopy.
>
> I'll split that, but just to mention, I think there should have no
> functional change, or we're doomed.

Right, but it's technically not impossible, so better to have it
separate anyway.

>
> The old code worked the same by ignoring migrate_set_state() when PAUSED in
> the cmpxchg(), which is implicit and unclear.
>

Agreed.

>> 
>> > +
>> > +    migrate_set_state(&mis->state, current, MIGRATION_STATUS_SETUP);
>> > +    return true;
>> > +}
>> > +
>> >  static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>> >                                            MigrationChannelList *channels,
>> >                                            Error **errp)
>> > @@ -633,8 +653,9 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>> >          return;
>> >      }
>> >  
>> > -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
>> > -                      MIGRATION_STATUS_SETUP);
>> > +    if (!migration_incoming_state_setup(mis, errp)) {
>> > +        return;
>> > +    }
>> >  
>> >      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> >          SocketAddress *saddr = &addr->u.socket;
>> > @@ -1070,6 +1091,7 @@ bool migration_is_setup_or_active(void)
>> >      case MIGRATION_STATUS_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>> >      case MIGRATION_STATUS_SETUP:
>> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
>> > @@ -1092,6 +1114,7 @@ bool migration_is_running(void)
>> >      case MIGRATION_STATUS_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>> >      case MIGRATION_STATUS_SETUP:
>> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
>> > @@ -1229,6 +1252,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>> >      case MIGRATION_STATUS_PRE_SWITCHOVER:
>> >      case MIGRATION_STATUS_DEVICE:
>> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>> >          /* TODO add some postcopy stats */
>> >          populate_time_info(info, s);
>> > @@ -1279,6 +1303,7 @@ static void fill_destination_migration_info(MigrationInfo *info)
>> >      case MIGRATION_STATUS_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> 
>> Does this need to be here? We don't reach this state on destination, right?
>
> Right, it won't hurt having that here, but let me drop it.
>
>> 
>> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>> >      case MIGRATION_STATUS_FAILED:
>> >      case MIGRATION_STATUS_COLO:
>> > @@ -1435,9 +1460,30 @@ static void migrate_error_free(MigrationState *s)
>> >  
>> >  static void migrate_fd_error(MigrationState *s, const Error *error)
>> >  {
>> 
>> The default case of the swtich below is a bit surprising to me. Why
>> wouldn't we allow this function to be called from other places to set
>> STATUS_FAILED?
>> 
>> ...unless this is only mean for the connection phase, so:
>> 
>> just to check your understanding here because it seems we've drifted a
>> bit from the original definition on those, specially with
>> migrate_fd_cleanup(), but does this _fd_ in the function name implies
>> something like a "connection phase"? As in, "connect to the fd", "the fd
>> connection errored out" and "cleanup the fd connection". Maybe it's time
>> to switch this "fd" to something clearer...
>
> IIUC it's just that many functions around this area used to be called
> migrate_fd_*().  Let me know if you have some suggestions, though.
>

At some other point I think we need to rename those. Very little going
on in these functions that relates directly to an fd. It needs a closer
inspection, however.

>> 
>> > +    MigrationStatus current = s->state;
>> > +    MigrationStatus next;
>> > +
>> >      assert(s->to_dst_file == NULL);
>> > -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> > -                      MIGRATION_STATUS_FAILED);
>> > +
>> > +    switch (current) {
>> > +    case MIGRATION_STATUS_SETUP:
>> > +        next = MIGRATION_STATUS_FAILED;
>> > +        break;
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> > +        /* Never fail a postcopy migration; switch back to PAUSED instead */
>> > +        next = MIGRATION_STATUS_POSTCOPY_PAUSED;
>> 
>> So presumably we can keep recovering the migration indefinitely?
>
> Yes, that's by design.
>

ok

>> 
>> > +        break;
>> > +    default:
>> > +        /*
>> > +         * This really shouldn't happen. Just be careful to not crash a VM
>> > +         * just for this.  Instead, dump something.
>> > +         */
>> > +        error_report("%s: Illegal migration status (%s) detected",
>> > +                     __func__, MigrationStatus_str(current));
>> > +        return;
>> > +    }
>> > +
>> > +    migrate_set_state(&s->state, current, next);
>> >      migrate_set_error(s, error);
>> >  }
>> >  
>> > @@ -1538,6 +1584,7 @@ bool migration_in_postcopy(void)
>> >      switch (s->state) {
>> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >      case MIGRATION_STATUS_POSTCOPY_PAUSED:
>> > +    case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
>> >      case MIGRATION_STATUS_POSTCOPY_RECOVER:
>> >          return true;
>> >      default:
>> > @@ -1936,6 +1983,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>> >              return false;
>> >          }
>> >  
>> > +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
>> > +                          MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP);
>> > +
>> >          /* This is a resume, skip init status */
>> >          return true;
>> >      }
>> > @@ -2968,9 +3018,9 @@ static MigThrError postcopy_pause(MigrationState *s)
>> >           * We wait until things fixed up. Then someone will setup the
>> >           * status back for us.
>> >           */
>> > -        while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>> > +        do {
>> >              qemu_sem_wait(&s->postcopy_pause_sem);
>> > -        }
>> > +        } while (postcopy_is_paused(s->state));
>> 
>> Is there a particular reason to go from while() to do{}while()?
>
> It'll be the same AFAICT, the 1st check should mostly be meaningless as the
> sem must be posted at least once.  Yes I should have had a comment
> somewhere but I didn't.. if you want me to keep the old way it's also fine,
> isn't a big deal here to check first either, just save some cycles when I
> worked on these lines.
>

That's fine. Just making sure there wasn't some hidden purpose here.

>> 
>> >  
>> >          if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
>> >              /* Woken up by a recover procedure. Give it a shot */
>> > @@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> >  {
>> >      Error *local_err = NULL;
>> >      uint64_t rate_limit;
>> > -    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>> > +    bool resume = migration_in_postcopy();
>> 
>> Here you're expecting just PAUSED or RECOVER_SETUP, right? We'll not
>> reach here in any of the other postcopy states.
>
> I think here it must be RECOVER_SETUP after this patch.  I changed it to
> use the helper as I think that's cleaner (precopy doesn't allow resume),
> and we don't need such change if the state machine trivially changes again.
>

Intent matters for anyone reading the code in the future. I would use
the state explicitly, but I don't have a strong opinion, feel free to
ignore.

>> 
>> >      int ret;
>> >  
>> >      /*
>> > @@ -3733,7 +3783,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> >  
>> >      if (resume) {
>> >          /* Wakeup the main migration thread to do the recovery */
>> > -        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_PAUSED,
>> > +        migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
>> >                            MIGRATION_STATUS_POSTCOPY_RECOVER);
>> >          qemu_sem_post(&s->postcopy_pause_sem);
>> >          return;
>> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> > index 97701e6bb2..1c374b7ea1 100644
>> > --- a/migration/postcopy-ram.c
>> > +++ b/migration/postcopy-ram.c
>> > @@ -1770,3 +1770,9 @@ void *postcopy_preempt_thread(void *opaque)
>> >  
>> >      return NULL;
>> >  }
>> > +
>> > +bool postcopy_is_paused(MigrationStatus status)
>> > +{
>> > +    return status == MIGRATION_STATUS_POSTCOPY_PAUSED ||
>> > +        status == MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP;
>> > +}
>> > diff --git a/migration/savevm.c b/migration/savevm.c
>> > index e71410d8c1..deb57833f8 100644
>> > --- a/migration/savevm.c
>> > +++ b/migration/savevm.c
>> > @@ -2864,9 +2864,9 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>> >      error_report("Detected IO failure for postcopy. "
>> >                   "Migration paused.");
>> >  
>> > -    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
>> > +    do {
>> >          qemu_sem_wait(&mis->postcopy_pause_sem_dst);
>> > -    }
>> > +    } while (postcopy_is_paused(mis->state));
>> >  
>> >      trace_postcopy_pause_incoming_continued();
>> 

Re: [PATCH 4/4] migration/postcopy: Add postcopy-recover-setup phase

[Peter Xu]

On Thu, Jun 13, 2024 at 02:21:04PM -0300, Fabiano Rosas wrote:
> >> > @@ -3666,7 +3716,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> >> >  {
> >> >      Error *local_err = NULL;
> >> >      uint64_t rate_limit;
> >> > -    bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
> >> > +    bool resume = migration_in_postcopy();
> >> 
> >> Here you're expecting just PAUSED or RECOVER_SETUP, right? We'll not
> >> reach here in any of the other postcopy states.
> >
> > I think here it must be RECOVER_SETUP after this patch.  I changed it to
> > use the helper as I think that's cleaner (precopy doesn't allow resume),
> > and we don't need such change if the state machine trivially changes again.
> >
> 
> Intent matters for anyone reading the code in the future. I would use
> the state explicitly, but I don't have a strong opinion, feel free to
> ignore.

Neither do I; let me switch back then if it helps in some form.

-- 
Peter Xu

Re: [PATCH 0/4] migration: New postcopy state, and some cleanups

[Peter Xu]

On Wed, Jun 12, 2024 at 10:42:24AM -0400, Peter Xu wrote:
> The major goal of this patchset is patch 4, which introduced a new postcopy
> state so that we will send an event in postcopy reconnect failures that
> Libvirt would prefer to have.  There's more information for that issue in
> the commit message alone.
> 
> Patch 1-2 are cleanups that are not directly relevant but I found/stored
> that could be good to have.  I made it simple by putting them together in
> one thread to make patch management easier, but I can send them separately
> when necessary.
> 
> Patch 3 is also a cleanup, but will be needed for patch 4 as dependency.
> 
> Comments welcomed, thanks.
> 
> CI: https://gitlab.com/peterx/qemu/-/pipelines/1328309702
>     (msys2-64bit is failing, but doesn't look relevant)

I forgot to update the doc file, I'll attach one more patch when repost,
attached here for early comments.

I also forgot to copy QAPI maintainers; It turns out I start to forget how
to send patches.. sorry. I'll do that in v2.

===8<===
From b4693c1576fb3741ca25962bd91f31c2afb60863 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Wed, 12 Jun 2024 11:18:27 -0400
Subject: [PATCH] migration/docs: Update postcopy recover session for SETUP
 phase

Firstly, the "Paused" state was added in the wrong place before. The state
machine section was describing PostcopyState, rather than MigrationStatus.
Drop the Paused state descriptions.

Then in the postcopy recover session, add more information on the state
machine for MigrationStatus in the lines.  Add the new RECOVER_SETUP phase.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/migration/postcopy.rst | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/docs/devel/migration/postcopy.rst b/docs/devel/migration/postcopy.rst
index 6c51e96d79..a15594e11f 100644
--- a/docs/devel/migration/postcopy.rst
+++ b/docs/devel/migration/postcopy.rst
@@ -99,17 +99,6 @@ ADVISE->DISCARD->LISTEN->RUNNING->END
     (although it can't do the cleanup it would do as it
     finishes a normal migration).
 
- - Paused
-
-    Postcopy can run into a paused state (normally on both sides when
-    happens), where all threads will be temporarily halted mostly due to
-    network errors.  When reaching paused state, migration will make sure
-    the qemu binary on both sides maintain the data without corrupting
-    the VM.  To continue the migration, the admin needs to fix the
-    migration channel using the QMP command 'migrate-recover' on the
-    destination node, then resume the migration using QMP command 'migrate'
-    again on source node, with resume=true flag set.
-
  - End
 
     The listen thread can now quit, and perform the cleanup of migration
@@ -221,7 +210,8 @@ paused postcopy migration.
 
 The recovery phase normally contains a few steps:
 
-  - When network issue occurs, both QEMU will go into PAUSED state
+  - When network issue occurs, both QEMU will go into **POSTCOPY_PAUSED**
+    migration state.
 
   - When the network is recovered (or a new network is provided), the admin
     can setup the new channel for migration using QMP command
@@ -229,9 +219,20 @@ The recovery phase normally contains a few steps:
 
   - On source host, the admin can continue the interrupted postcopy
     migration using QMP command 'migrate' with resume=true flag set.
-
-  - After the connection is re-established, QEMU will continue the postcopy
-    migration on both sides.
+    Source QEMU will go into **POSTCOPY_RECOVER_SETUP** state trying to
+    re-establish the channels.
+
+  - When both sides of QEMU successfully reconnects using a new or fixed up
+    channel, they will go into **POSTCOPY_RECOVER** state, some handshake
+    procedure will be needed to properly synchronize the VM states between
+    the two QEMUs to continue the postcopy migration.  For example, there
+    can be pages sent right during the window when the network is
+    interrupted, then the handshake will guarantee pages lost in-flight
+    will be resent again.
+
+  - After a proper handshake synchronization, QEMU will continue the
+    postcopy migration on both sides and go back to **POSTCOPY_ACTIVE**
+    state.  Postcopy migration will continue.
 
 During a paused postcopy migration, the VM can logically still continue
 running, and it will not be impacted from any page access to pages that
-- 
2.45.0


-- 
Peter Xu