[PATCH v2 0/9] migration: Fix issues during qmp_migrate_cancel

[PATCH v2 0/9] migration: Fix issues during qmp_migrate_cancel

[Fabiano Rosas]

Continuing with the migrate_cancel testing, this time I blocked cancel
during postcopy. This simplified the tests a lot.

I also added a new fix for pre-switchover, which was ignoring the
cancelling state.

Upfront are some trivial cleanups to function names.

Thanks

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1665902865

v1:
https://lore.kernel.org/r/20241202220137.32584-1-farosas@suse.de

While working on downstream issues with postcopy, I ended up writing a
set of tests for issuing qmp_migrate_cancel() at various points during
the migration. That exposed some bugs, which this series attempts to
fix.

There is also a fix for the issue Daniel found:

https://gitlab.com/qemu-project/qemu/-/issues/2633

I'm also sending the test code. It creates one test per
MIGRATION_STATUS_ state. Each test starts a migration, waits for that
specific state to be reached, issues qmp_migrate_cancel() and checks
that the migration state changes to cancelled (for now only cancelling
migration from the source side).

I was initially worried that this would be too racy, but so far each
test has survived 1000 iterations. I'm thinking it's worth merging,
specially because even after working on this I haven't been able to
clear the questions we have in our todo list [1], so we'll probably
need more work around this area in the future.

1- https://wiki.qemu.org/ToDo/LiveMigration#Migration_cancel_concurrency

Fabiano Rosas (9):
  migration: Set migration error outside of migrate_cancel
  migration: Unify migration_cancel and migrate_fd_cancel
  migration: Change migrate_fd_ to migration_
  migration: Fix hang after error in destination setup phase
  migration: Reject qmp_migrate_cancel after postcopy
  migration: Don't set FAILED state when cancelling
  tests/qtest/migration: Introduce migration_test_add_suffix
  tests/qtest/migration: Add a cancel test
  migration: Update migrate_cancel documentation

 migration/channel.c                    |   9 +-
 migration/migration.c                  |  70 +++++-----
 migration/migration.h                  |   4 +-
 migration/multifd.c                    |   2 +-
 migration/ram.c                        |   4 +-
 migration/rdma.c                       |   2 +-
 migration/trace-events                 |   4 +-
 qapi/migration.json                    |   4 +-
 tests/qtest/migration/migration-util.c |  24 ++++
 tests/qtest/migration/migration-util.h |   2 +
 tests/qtest/migration/precopy-tests.c  | 176 +++++++++++++++++++++++++
 11 files changed, 259 insertions(+), 42 deletions(-)

-- 
2.35.3

[PATCH v2 1/9] migration: Set migration error outside of migrate_cancel

[Fabiano Rosas]

There's no point passing the error into migration cancel only for it
to call migrate_set_error().

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 7 ++-----
 migration/migration.h | 2 +-
 migration/ram.c       | 4 +++-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 396928513a..7728f52aef 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -342,11 +342,8 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque)
     qemu_bh_schedule(bh);
 }
 
-void migration_cancel(const Error *error)
+void migration_cancel()
 {
-    if (error) {
-        migrate_set_error(current_migration, error);
-    }
     if (migrate_dirty_limit()) {
         qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
     }
@@ -365,7 +362,7 @@ void migration_shutdown(void)
      * Cancel the current migration - that will (eventually)
      * stop the migration using this structure
      */
-    migration_cancel(NULL);
+    migration_cancel();
     object_unref(OBJECT(current_migration));
 
     /*
diff --git a/migration/migration.h b/migration/migration.h
index eaebcc2042..3547420c52 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -530,7 +530,7 @@ void migration_make_urgent_request(void);
 void migration_consume_urgent_request(void);
 bool migration_rate_limit(void);
 void migration_bh_schedule(QEMUBHFunc *cb, void *opaque);
-void migration_cancel(const Error *error);
+void migration_cancel(void);
 
 void migration_populate_vfio_info(MigrationInfo *info);
 void migration_reset_vfio_bytes_transferred(void);
diff --git a/migration/ram.c b/migration/ram.c
index 6f460fd22d..589b6505eb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4465,8 +4465,10 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
          * Abort and indicate a proper reason.
          */
         error_setg(&err, "RAM block '%s' resized during precopy.", rb->idstr);
-        migration_cancel(err);
+        migrate_set_error(migrate_get_current(), err);
         error_free(err);
+
+        migration_cancel();
     }
 
     switch (ps) {
-- 
2.35.3

[PATCH v2 2/9] migration: Unify migration_cancel and migrate_fd_cancel

[Fabiano Rosas]

There's no need for two separate functions and this _fd_ is a historic
artifact that makes little sense nowadays.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c  | 18 +++++++-----------
 migration/trace-events |  2 +-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7728f52aef..e37842fdd2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -106,7 +106,6 @@ static GSList *migration_blockers[MIG_MODE__MAX];
 
 static bool migration_object_check(MigrationState *ms, Error **errp);
 static bool migration_switchover_start(MigrationState *s, Error **errp);
-static void migrate_fd_cancel(MigrationState *s);
 static bool close_return_path_on_source(MigrationState *s);
 static void migration_completion_end(MigrationState *s);
 static void migrate_hup_delete(MigrationState *s);
@@ -342,14 +341,6 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque)
     qemu_bh_schedule(bh);
 }
 
-void migration_cancel()
-{
-    if (migrate_dirty_limit()) {
-        qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
-    }
-    migrate_fd_cancel(current_migration);
-}
-
 void migration_shutdown(void)
 {
     /*
@@ -1555,12 +1546,17 @@ static void migrate_fd_error(MigrationState *s, const Error *error)
     migrate_set_error(s, error);
 }
 
-static void migrate_fd_cancel(MigrationState *s)
+void migration_cancel(void)
 {
+    MigrationState *s = migrate_get_current();
     int old_state ;
     bool setup = (s->state == MIGRATION_STATUS_SETUP);
 
-    trace_migrate_fd_cancel();
+    trace_migration_cancel();
+
+    if (migrate_dirty_limit()) {
+        qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
+    }
 
     WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
         if (s->rp_state.from_dst_file) {
diff --git a/migration/trace-events b/migration/trace-events
index 12b262f8ee..d22600abe6 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -156,7 +156,7 @@ multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostnam
 migrate_set_state(const char *new_state) "new state %s"
 migrate_fd_cleanup(void) ""
 migrate_error(const char *error_desc) "error=%s"
-migrate_fd_cancel(void) ""
+migration_cancel(void) ""
 migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
 migrate_pending_exact(uint64_t size, uint64_t pre, uint64_t post) "exact pending size %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")"
 migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate pending size %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")"
-- 
2.35.3

[PATCH v2 3/9] migration: Change migrate_fd_ to migration_

[Fabiano Rosas]

Remove all instances of _fd_ from the migration generic code. These
functions have grown over time and the _fd_ part is now just
confusing.

migration_fd_error() -> migration_error() makes it a little vague, so
change it to migration_set_error_state().

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/channel.c    |  4 ++--
 migration/migration.c  | 30 +++++++++++++++---------------
 migration/migration.h  |  2 +-
 migration/multifd.c    |  2 +-
 migration/rdma.c       |  2 +-
 migration/trace-events |  2 +-
 6 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index f9de064f3b..24a91ef911 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -74,7 +74,7 @@ void migration_channel_connect(MigrationState *s,
             if (!error) {
                 /* tls_channel_connect will call back to this
                  * function after the TLS handshake,
-                 * so we mustn't call migrate_fd_connect until then
+                 * so we mustn't call migration_connect until then
                  */
 
                 return;
@@ -89,7 +89,7 @@ void migration_channel_connect(MigrationState *s,
             qemu_mutex_unlock(&s->qemu_file_lock);
         }
     }
-    migrate_fd_connect(s, error);
+    migration_connect(s, error);
     error_free(error);
 }
 
diff --git a/migration/migration.c b/migration/migration.c
index e37842fdd2..db8d6e8ea7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1423,12 +1423,12 @@ static void migration_cleanup_json_writer(MigrationState *s)
     g_clear_pointer(&s->vmdesc, json_writer_free);
 }
 
-static void migrate_fd_cleanup(MigrationState *s)
+static void migration_cleanup(MigrationState *s)
 {
     MigrationEventType type;
     QEMUFile *tmp = NULL;
 
-    trace_migrate_fd_cleanup();
+    trace_migration_cleanup();
 
     migration_cleanup_json_writer(s);
 
@@ -1485,9 +1485,9 @@ static void migrate_fd_cleanup(MigrationState *s)
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
-static void migrate_fd_cleanup_bh(void *opaque)
+static void migration_cleanup_bh(void *opaque)
 {
-    migrate_fd_cleanup(opaque);
+    migration_cleanup(opaque);
 }
 
 void migrate_set_error(MigrationState *s, const Error *error)
@@ -1517,7 +1517,7 @@ static void migrate_error_free(MigrationState *s)
     }
 }
 
-static void migrate_fd_error(MigrationState *s, const Error *error)
+static void migration_set_error_state(MigrationState *s, const Error *error)
 {
     MigrationStatus current = s->state;
     MigrationStatus next;
@@ -2198,7 +2198,7 @@ void qmp_migrate(const char *uri, bool has_channels,
 
 out:
     if (local_err) {
-        migrate_fd_error(s, local_err);
+        migration_set_error_state(s, local_err);
         error_propagate(errp, local_err);
     }
 }
@@ -2243,7 +2243,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
         if (!resume_requested) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
-        migrate_fd_error(s, local_err);
+        migration_set_error_state(s, local_err);
         error_propagate(errp, local_err);
         return;
     }
@@ -3427,7 +3427,7 @@ static void migration_iteration_finish(MigrationState *s)
         break;
     }
 
-    migration_bh_schedule(migrate_fd_cleanup_bh, s);
+    migration_bh_schedule(migration_cleanup_bh, s);
     bql_unlock();
 }
 
@@ -3455,7 +3455,7 @@ static void bg_migration_iteration_finish(MigrationState *s)
         break;
     }
 
-    migration_bh_schedule(migrate_fd_cleanup_bh, s);
+    migration_bh_schedule(migration_cleanup_bh, s);
     bql_unlock();
 }
 
@@ -3837,7 +3837,7 @@ fail_setup:
     return NULL;
 }
 
-void migrate_fd_connect(MigrationState *s, Error *error_in)
+void migration_connect(MigrationState *s, Error *error_in)
 {
     Error *local_err = NULL;
     uint64_t rate_limit;
@@ -3847,24 +3847,24 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     /*
      * If there's a previous error, free it and prepare for another one.
      * Meanwhile if migration completes successfully, there won't have an error
-     * dumped when calling migrate_fd_cleanup().
+     * dumped when calling migration_cleanup().
      */
     migrate_error_free(s);
 
     s->expected_downtime = migrate_downtime_limit();
     if (error_in) {
-        migrate_fd_error(s, error_in);
+        migration_set_error_state(s, error_in);
         if (resume) {
             /*
              * Don't do cleanup for resume if channel is invalid, but only dump
              * the error.  We wait for another channel connect from the user.
              * The error_report still gives HMP user a hint on what failed.
-             * It's normally done in migrate_fd_cleanup(), but call it here
+             * It's normally done in migration_cleanup(), but call it here
              * explicitly.
              */
             error_report_err(error_copy(s->error));
         } else {
-            migrate_fd_cleanup(s);
+            migration_cleanup(s);
         }
         return;
     }
@@ -3944,7 +3944,7 @@ fail:
     migrate_set_error(s, local_err);
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     error_report_err(local_err);
-    migrate_fd_cleanup(s);
+    migration_cleanup(s);
 }
 
 static void migration_class_init(ObjectClass *klass, void *data)
diff --git a/migration/migration.h b/migration/migration.h
index 3547420c52..76afef34c4 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -484,7 +484,7 @@ bool  migration_has_all_channels(void);
 void migrate_set_error(MigrationState *s, const Error *error);
 bool migrate_has_error(MigrationState *s);
 
-void migrate_fd_connect(MigrationState *s, Error *error_in);
+void migration_connect(MigrationState *s, Error *error_in);
 
 int migration_call_notifiers(MigrationState *s, MigrationEventType type,
                              Error **errp);
diff --git a/migration/multifd.c b/migration/multifd.c
index ab73d6d984..d7ea8668ad 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -444,7 +444,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
          * channels have no I/O handler callback registered when reaching
          * here, because migration thread will wait for all multifd channel
          * establishments to complete during setup.  Since
-         * migrate_fd_cleanup() will be scheduled in main thread too, all
+         * migration_cleanup() will be scheduled in main thread too, all
          * previous callbacks should guarantee to be completed when
          * reaching here.  See multifd_send_state.channels_created and its
          * usage.  In the future, we could replace this with an assert
diff --git a/migration/rdma.c b/migration/rdma.c
index 855753c671..76fb034923 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4174,7 +4174,7 @@ void rdma_start_outgoing_migration(void *opaque,
 
     s->to_dst_file = rdma_new_output(rdma);
     s->rdma_migration = true;
-    migrate_fd_connect(s, NULL);
+    migration_connect(s, NULL);
     return;
 return_path_err:
     qemu_rdma_cleanup(rdma);
diff --git a/migration/trace-events b/migration/trace-events
index d22600abe6..58c0f07f5b 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -154,7 +154,7 @@ multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostnam
 
 # migration.c
 migrate_set_state(const char *new_state) "new state %s"
-migrate_fd_cleanup(void) ""
+migration_cleanup(void) ""
 migrate_error(const char *error_desc) "error=%s"
 migration_cancel(void) ""
 migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
-- 
2.35.3

[PATCH v2 4/9] migration: Fix hang after error in destination setup phase

[Fabiano Rosas]

If the destination side fails at migration_ioc_process_incoming()
before starting the coroutine, it will report the error but QEMU will
not exit.

Set the migration state to FAILED and exit the process if
exit-on-error allows.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2633
Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/channel.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration/channel.c b/migration/channel.c
index 24a91ef911..a547b1fbfe 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -33,6 +33,7 @@
 void migration_channel_process_incoming(QIOChannel *ioc)
 {
     MigrationState *s = migrate_get_current();
+    MigrationIncomingState *mis = migration_incoming_get_current();
     Error *local_err = NULL;
 
     trace_migration_set_incoming_channel(
@@ -47,6 +48,10 @@ void migration_channel_process_incoming(QIOChannel *ioc)
 
     if (local_err) {
         error_report_err(local_err);
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+        if (mis->exit_on_error) {
+            exit(EXIT_FAILURE);
+        }
     }
 }
 
-- 
2.35.3

[PATCH v2 5/9] migration: Reject qmp_migrate_cancel after postcopy

[Fabiano Rosas]

After postcopy has started, it's not possible to recover the source
machine in case a migration error occurs because the destination has
already been changing the state of the machine. For that same reason,
it doesn't make sense to try to cancel the migration after postcopy
has started. Reject the cancel command during postcopy.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index db8d6e8ea7..375de6d460 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2251,7 +2251,18 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
 
 void qmp_migrate_cancel(Error **errp)
 {
-    migration_cancel(NULL);
+    /*
+     * After postcopy migration has started, the source machine is not
+     * recoverable in case of a migration error. This also means the
+     * cancel command cannot be used as cancel should allow the
+     * machine to continue operation.
+     */
+    if (migration_in_postcopy()) {
+        error_setg(errp, "Postcopy migration in progress, cannot cancel.");
+        return;
+    }
+
+    migration_cancel();
 }
 
 void qmp_migrate_continue(MigrationStatus state, Error **errp)
-- 
2.35.3

[PATCH v2 6/9] migration: Don't set FAILED state when cancelling

[Fabiano Rosas]

It's possible that the migration is cancelled during
migration_switchover_start(). In that case, don't set the migration
state FAILED in migration_completion().

Fixes: 3dde8fdbad ("migration: Merge precopy/postcopy on switchover start")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 375de6d460..5dc43bcdc0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2986,7 +2986,9 @@ fail:
         error_free(local_err);
     }
 
-    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+    if (s->state != MIGRATION_STATUS_CANCELLING) {
+        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+    }
 }
 
 /**
-- 
2.35.3

[PATCH v2 7/9] tests/qtest/migration: Introduce migration_test_add_suffix

[Fabiano Rosas]

Introduce a new migration_test_add_suffix to allow programmatic
creation of tests based on a suffix. Pass the test name into the test
so it can know which variant to run.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/migration-util.c | 24 ++++++++++++++++++++++++
 tests/qtest/migration/migration-util.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c
index 6261d80e4a..642cf50c8d 100644
--- a/tests/qtest/migration/migration-util.c
+++ b/tests/qtest/migration/migration-util.c
@@ -236,6 +236,7 @@ char *resolve_machine_version(const char *alias, const char *var1,
 typedef struct {
     char *name;
     void (*func)(void);
+    void (*func_full)(void *);
 } MigrationTest;
 
 static void migration_test_destroy(gpointer data)
@@ -265,6 +266,29 @@ void migration_test_add(const char *path, void (*fn)(void))
                              migration_test_destroy);
 }
 
+static void migration_test_wrapper_full(const void *data)
+{
+    MigrationTest *test = (MigrationTest *)data;
+
+    g_test_message("Running /%s%s", qtest_get_arch(), test->name);
+    test->func_full(test->name);
+}
+
+void migration_test_add_suffix(const char *path, const char *suffix,
+                               void (*fn)(void *))
+{
+    MigrationTest *test = g_new0(MigrationTest, 1);
+
+    g_assert(g_str_has_suffix(path, "/"));
+    g_assert(!g_str_has_prefix(suffix, "/"));
+
+    test->func_full = fn;
+    test->name = g_strconcat(path, suffix, NULL);
+
+    qtest_add_data_func_full(test->name, test, migration_test_wrapper_full,
+                             migration_test_destroy);
+}
+
 #ifdef O_DIRECT
 /*
  * Probe for O_DIRECT support on the filesystem. Since this is used
diff --git a/tests/qtest/migration/migration-util.h b/tests/qtest/migration/migration-util.h
index f5f2e4650e..44815e9c42 100644
--- a/tests/qtest/migration/migration-util.h
+++ b/tests/qtest/migration/migration-util.h
@@ -51,6 +51,8 @@ static inline bool probe_o_direct_support(const char *tmpfs)
 bool ufd_version_check(bool *uffd_feature_thread_id);
 bool kvm_dirty_ring_supported(void);
 void migration_test_add(const char *path, void (*fn)(void));
+void migration_test_add_suffix(const char *path, const char *suffix,
+                               void (*fn)(void *));
 char *migrate_get_connect_uri(QTestState *who);
 void migrate_set_ports(QTestState *to, QList *channel_list);
 
-- 
2.35.3

[PATCH v2 8/9] tests/qtest/migration: Add a cancel test

[Fabiano Rosas]

The qmp_migrate_cancel() command is poorly tested and code inspection
reveals that there might be concurrency issues with its usage. Add a
test that runs a migration and calls qmp_migrate_cancel() at specific
moments.

In order to make the test more deterministic, instead of calling
qmp_migrate_cancel() at random moments during migration, do it after
the migration status change events are seen.

The expected result is that qmp_migrate_cancel() on the source ends
migration on the source with the "cancelled" state and ends migration
on the destination with the "failed" state. The only exception is that
a failed migration should continue in the failed state.

Cancelling is not allowed during postcopy (no test is added for this
because it's a trivial check in the code).

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration/precopy-tests.c | 176 ++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)

diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index 162fa69531..ba273d10b9 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -20,6 +20,7 @@
 #include "migration/migration-util.h"
 #include "ppc-util.h"
 #include "qobject/qlist.h"
+#include "qapi-types-migration.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/range.h"
@@ -536,6 +537,161 @@ static void test_multifd_tcp_cancel(void)
     migrate_end(from, to2, true);
 }
 
+static void test_cancel_src_after_failed(QTestState *from, QTestState *to,
+                                         const char *uri, const char *phase)
+{
+    /*
+     * No migrate_incoming_qmp() at the start to force source into
+     * failed state during migrate_qmp().
+     */
+
+    wait_for_serial("src_serial");
+    migrate_ensure_converge(from);
+
+    migrate_qmp(from, to, uri, NULL, "{}");
+
+    migration_event_wait(from, phase);
+    migrate_cancel(from);
+
+    /* cancelling will not move the migration out of 'failed' */
+
+    wait_for_migration_status(from, "failed",
+                              (const char * []) { "completed", NULL });
+
+    /*
+     * Not waiting for the destination because it never started
+     * migration.
+     */
+}
+
+static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to,
+                                            const char *uri, const char *phase)
+{
+    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
+
+    wait_for_serial("src_serial");
+    migrate_ensure_converge(from);
+
+    migrate_qmp(from, to, uri, NULL, "{}");
+
+    /* To move to cancelled/cancelling */
+    migrate_cancel(from);
+    migration_event_wait(from, phase);
+
+    /* The migrate_cancel under test */
+    migrate_cancel(from);
+
+    wait_for_migration_status(from, "cancelled",
+                              (const char * []) { "completed", NULL });
+
+    wait_for_migration_status(to, "failed",
+                              (const char * []) { "completed", NULL });
+}
+
+static void test_cancel_src_after_complete(QTestState *from, QTestState *to,
+                                           const char *uri, const char *phase)
+{
+    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
+
+    wait_for_serial("src_serial");
+    migrate_ensure_converge(from);
+
+    migrate_qmp(from, to, uri, NULL, "{}");
+
+    migration_event_wait(from, phase);
+    migrate_cancel(from);
+
+    /*
+     * qmp_migrate_cancel() exits early if migration is not running
+     * anymore, the status will not change to cancelled.
+     */
+    wait_for_migration_complete(from);
+    wait_for_migration_complete(to);
+}
+
+static void test_cancel_src_after_none(QTestState *from, QTestState *to,
+                                       const char *uri, const char *phase)
+{
+    /*
+     * Test that cancelling without a migration happening does not
+     * affect subsequent migrations
+     */
+    migrate_cancel(to);
+
+    wait_for_serial("src_serial");
+    migrate_cancel(from);
+
+    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
+
+    migrate_ensure_converge(from);
+    migrate_qmp(from, to, uri, NULL, "{}");
+
+    wait_for_migration_complete(from);
+    wait_for_migration_complete(to);
+}
+
+static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
+                                           const char *uri, const char *phase)
+{
+    migrate_set_capability(from, "pause-before-switchover", true);
+    migrate_set_capability(to, "pause-before-switchover", true);
+
+    migrate_set_capability(from, "multifd", true);
+    migrate_set_capability(to, "multifd", true);
+
+    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
+
+    wait_for_serial("src_serial");
+    migrate_ensure_converge(from);
+
+    migrate_qmp(from, to, uri, NULL, "{}");
+
+    migration_event_wait(from, phase);
+    migrate_cancel(from);
+    migration_event_wait(from, "cancelling");
+
+    wait_for_migration_status(from, "cancelled",
+                              (const char * []) { "completed", NULL });
+
+    wait_for_migration_status(to, "failed",
+                              (const char * []) { "completed", NULL });
+}
+
+static void test_cancel_src_after_status(void *opaque)
+{
+    const char *test_path = opaque;
+    g_autofree char *phase = g_path_get_basename(test_path);
+    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+    MigrateStart args = {
+        .hide_stderr = true,
+    };
+
+    if (migrate_start(&from, &to, "defer", &args)) {
+        return;
+    }
+
+    if (g_str_equal(phase, "cancelling") ||
+        g_str_equal(phase, "cancelled")) {
+        test_cancel_src_after_cancelled(from, to, uri, phase);
+
+    } else if (g_str_equal(phase, "completed")) {
+        test_cancel_src_after_complete(from, to, uri, phase);
+
+    } else if (g_str_equal(phase, "failed")) {
+        test_cancel_src_after_failed(from, to, uri, phase);
+
+    } else if (g_str_equal(phase, "none")) {
+        test_cancel_src_after_none(from, to, uri, phase);
+
+    } else {
+        /* any state that comes before pre-switchover */
+        test_cancel_src_pre_switchover(from, to, uri, phase);
+    }
+
+    migrate_end(from, to, false);
+}
+
 static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
 {
     qtest_qmp_assert_success(who,
@@ -1018,4 +1174,24 @@ void migration_test_add_precopy(MigrationTestEnv *env)
                                test_vcpu_dirty_limit);
         }
     }
+
+    /* ensure new status don't go unnoticed */
+    assert(MIGRATION_STATUS__MAX == 15);
+
+    for (int i = MIGRATION_STATUS_NONE; i < MIGRATION_STATUS__MAX; i++) {
+        switch (i) {
+        case MIGRATION_STATUS_DEVICE: /* happens too fast */
+        case MIGRATION_STATUS_WAIT_UNPLUG: /* no support in tests */
+        case MIGRATION_STATUS_COLO: /* no support in tests */
+        case MIGRATION_STATUS_POSTCOPY_ACTIVE: /* postcopy can't be cancelled */
+        case MIGRATION_STATUS_POSTCOPY_PAUSED:
+        case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
+        case MIGRATION_STATUS_POSTCOPY_RECOVER:
+            continue;
+        default:
+            migration_test_add_suffix("/migration/cancel/src/after/",
+                                      MigrationStatus_str(i),
+                                      test_cancel_src_after_status);
+        }
+    }
 }
-- 
2.35.3

[PATCH v2 9/9] migration: Update migrate_cancel documentation

[Fabiano Rosas]

Update the migrate_cancel command documentation with a few words about
postcopy and the expected state of the machine after migration.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 qapi/migration.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 43babd1df4..8b9c53595c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1524,7 +1524,9 @@
 ##
 # @migrate_cancel:
 #
-# Cancel the current executing migration process.
+# Cancel the currently executing migration process.  Allows a new
+# migration to be started right after.  When postcopy-ram is in use,
+# cancelling is not allowed after the postcopy phase has started.
 #
 # .. note:: This command succeeds even if there is no migration
 #    process running.
-- 
2.35.3

Re: [PATCH v2 9/9] migration: Update migrate_cancel documentation

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Update the migrate_cancel command documentation with a few words about
> postcopy and the expected state of the machine after migration.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  qapi/migration.json | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 43babd1df4..8b9c53595c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1524,7 +1524,9 @@
>  ##
>  # @migrate_cancel:
>  #
> -# Cancel the current executing migration process.
> +# Cancel the currently executing migration process.  Allows a new
> +# migration to be started right after.  When postcopy-ram is in use,
> +# cancelling is not allowed after the postcopy phase has started.
>  #
>  # .. note:: This command succeeds even if there is no migration
>  #    process running.

Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v2 1/9] migration: Set migration error outside of migrate_cancel

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:28PM -0300, Fabiano Rosas wrote:
> There's no point passing the error into migration cancel only for it
> to call migrate_set_error().
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 2/9] migration: Unify migration_cancel and migrate_fd_cancel

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:29PM -0300, Fabiano Rosas wrote:
> There's no need for two separate functions and this _fd_ is a historic
> artifact that makes little sense nowadays.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 3/9] migration: Change migrate_fd_ to migration_

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:30PM -0300, Fabiano Rosas wrote:
> Remove all instances of _fd_ from the migration generic code. These
> functions have grown over time and the _fd_ part is now just
> confusing.
> 
> migration_fd_error() -> migration_error() makes it a little vague, so
> change it to migration_set_error_state().

I am ok on even renaming all of them if you prefer, but let me comment on
something, as not all of them apply to "fd is old concept"..

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/channel.c    |  4 ++--
>  migration/migration.c  | 30 +++++++++++++++---------------
>  migration/migration.h  |  2 +-
>  migration/multifd.c    |  2 +-
>  migration/rdma.c       |  2 +-
>  migration/trace-events |  2 +-
>  6 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index f9de064f3b..24a91ef911 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -74,7 +74,7 @@ void migration_channel_connect(MigrationState *s,
>              if (!error) {
>                  /* tls_channel_connect will call back to this
>                   * function after the TLS handshake,
> -                 * so we mustn't call migrate_fd_connect until then
> +                 * so we mustn't call migration_connect until then

migrate_fd_connect() makes some point only because of rdma, which can set
to_dst_file on its own.. which is indeed an fd (if we say fd~=qemufile)..

>                   */
>  
>                  return;
> @@ -89,7 +89,7 @@ void migration_channel_connect(MigrationState *s,
>              qemu_mutex_unlock(&s->qemu_file_lock);
>          }
>      }
> -    migrate_fd_connect(s, error);
> +    migration_connect(s, error);
>      error_free(error);
>  }
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index e37842fdd2..db8d6e8ea7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1423,12 +1423,12 @@ static void migration_cleanup_json_writer(MigrationState *s)
>      g_clear_pointer(&s->vmdesc, json_writer_free);
>  }
>  
> -static void migrate_fd_cleanup(MigrationState *s)
> +static void migration_cleanup(MigrationState *s)
>  {
>      MigrationEventType type;
>      QEMUFile *tmp = NULL;
>  
> -    trace_migrate_fd_cleanup();
> +    trace_migration_cleanup();
>  
>      migration_cleanup_json_writer(s);
>  
> @@ -1485,9 +1485,9 @@ static void migrate_fd_cleanup(MigrationState *s)
>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>  }
>  
> -static void migrate_fd_cleanup_bh(void *opaque)
> +static void migration_cleanup_bh(void *opaque)
>  {
> -    migrate_fd_cleanup(opaque);
> +    migration_cleanup(opaque);
>  }
>  
>  void migrate_set_error(MigrationState *s, const Error *error)
> @@ -1517,7 +1517,7 @@ static void migrate_error_free(MigrationState *s)
>      }
>  }
>  
> -static void migrate_fd_error(MigrationState *s, const Error *error)
> +static void migration_set_error_state(MigrationState *s, const Error *error)

This one also is slightly tricky, on that it is only used when fd connect
fails, so it has:

    assert(s->to_dst_file == NULL);

To avoid it being wrongly used during migration active, maybe rename it to
migration_connect_set_error()?

All look good otherwise.

>  {
>      MigrationStatus current = s->state;
>      MigrationStatus next;
> @@ -2198,7 +2198,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>  
>  out:
>      if (local_err) {
> -        migrate_fd_error(s, local_err);
> +        migration_set_error_state(s, local_err);
>          error_propagate(errp, local_err);
>      }
>  }
> @@ -2243,7 +2243,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          if (!resume_requested) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> -        migrate_fd_error(s, local_err);
> +        migration_set_error_state(s, local_err);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -3427,7 +3427,7 @@ static void migration_iteration_finish(MigrationState *s)
>          break;
>      }
>  
> -    migration_bh_schedule(migrate_fd_cleanup_bh, s);
> +    migration_bh_schedule(migration_cleanup_bh, s);
>      bql_unlock();
>  }
>  
> @@ -3455,7 +3455,7 @@ static void bg_migration_iteration_finish(MigrationState *s)
>          break;
>      }
>  
> -    migration_bh_schedule(migrate_fd_cleanup_bh, s);
> +    migration_bh_schedule(migration_cleanup_bh, s);
>      bql_unlock();
>  }
>  
> @@ -3837,7 +3837,7 @@ fail_setup:
>      return NULL;
>  }
>  
> -void migrate_fd_connect(MigrationState *s, Error *error_in)
> +void migration_connect(MigrationState *s, Error *error_in)
>  {
>      Error *local_err = NULL;
>      uint64_t rate_limit;
> @@ -3847,24 +3847,24 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      /*
>       * If there's a previous error, free it and prepare for another one.
>       * Meanwhile if migration completes successfully, there won't have an error
> -     * dumped when calling migrate_fd_cleanup().
> +     * dumped when calling migration_cleanup().
>       */
>      migrate_error_free(s);
>  
>      s->expected_downtime = migrate_downtime_limit();
>      if (error_in) {
> -        migrate_fd_error(s, error_in);
> +        migration_set_error_state(s, error_in);
>          if (resume) {
>              /*
>               * Don't do cleanup for resume if channel is invalid, but only dump
>               * the error.  We wait for another channel connect from the user.
>               * The error_report still gives HMP user a hint on what failed.
> -             * It's normally done in migrate_fd_cleanup(), but call it here
> +             * It's normally done in migration_cleanup(), but call it here
>               * explicitly.
>               */
>              error_report_err(error_copy(s->error));
>          } else {
> -            migrate_fd_cleanup(s);
> +            migration_cleanup(s);
>          }
>          return;
>      }
> @@ -3944,7 +3944,7 @@ fail:
>      migrate_set_error(s, local_err);
>      migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>      error_report_err(local_err);
> -    migrate_fd_cleanup(s);
> +    migration_cleanup(s);
>  }
>  
>  static void migration_class_init(ObjectClass *klass, void *data)
> diff --git a/migration/migration.h b/migration/migration.h
> index 3547420c52..76afef34c4 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -484,7 +484,7 @@ bool  migration_has_all_channels(void);
>  void migrate_set_error(MigrationState *s, const Error *error);
>  bool migrate_has_error(MigrationState *s);
>  
> -void migrate_fd_connect(MigrationState *s, Error *error_in);
> +void migration_connect(MigrationState *s, Error *error_in);
>  
>  int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>                               Error **errp);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index ab73d6d984..d7ea8668ad 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -444,7 +444,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>           * channels have no I/O handler callback registered when reaching
>           * here, because migration thread will wait for all multifd channel
>           * establishments to complete during setup.  Since
> -         * migrate_fd_cleanup() will be scheduled in main thread too, all
> +         * migration_cleanup() will be scheduled in main thread too, all
>           * previous callbacks should guarantee to be completed when
>           * reaching here.  See multifd_send_state.channels_created and its
>           * usage.  In the future, we could replace this with an assert
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 855753c671..76fb034923 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -4174,7 +4174,7 @@ void rdma_start_outgoing_migration(void *opaque,
>  
>      s->to_dst_file = rdma_new_output(rdma);
>      s->rdma_migration = true;
> -    migrate_fd_connect(s, NULL);
> +    migration_connect(s, NULL);
>      return;
>  return_path_err:
>      qemu_rdma_cleanup(rdma);
> diff --git a/migration/trace-events b/migration/trace-events
> index d22600abe6..58c0f07f5b 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -154,7 +154,7 @@ multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostnam
>  
>  # migration.c
>  migrate_set_state(const char *new_state) "new state %s"
> -migrate_fd_cleanup(void) ""
> +migration_cleanup(void) ""
>  migrate_error(const char *error_desc) "error=%s"
>  migration_cancel(void) ""
>  migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v2 4/9] migration: Fix hang after error in destination setup phase

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:31PM -0300, Fabiano Rosas wrote:
> If the destination side fails at migration_ioc_process_incoming()
> before starting the coroutine, it will report the error but QEMU will
> not exit.
> 
> Set the migration state to FAILED and exit the process if
> exit-on-error allows.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2633
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 5/9] migration: Reject qmp_migrate_cancel after postcopy

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:32PM -0300, Fabiano Rosas wrote:
> After postcopy has started, it's not possible to recover the source
> machine in case a migration error occurs because the destination has
> already been changing the state of the machine. For that same reason,
> it doesn't make sense to try to cancel the migration after postcopy
> has started. Reject the cancel command during postcopy.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 6/9] migration: Don't set FAILED state when cancelling

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:33PM -0300, Fabiano Rosas wrote:
> It's possible that the migration is cancelled during
> migration_switchover_start(). In that case, don't set the migration
> state FAILED in migration_completion().
> 
> Fixes: 3dde8fdbad ("migration: Merge precopy/postcopy on switchover start")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

I remember I paid some attention on this one when working on the commit,
where it has:

static bool migration_switchover_prepare(MigrationState *s)
{
    /* Concurrent cancellation?  Quit */
    if (s->state == MIGRATION_STATUS_CANCELLING) {   <================= [1]
        return false;
    }
    ...
    bql_unlock();

    qemu_sem_wait(&s->pause_sem);

    bql_lock();
    /*
     * After BQL released and retaken, the state can be CANCELLING if it
     * happend during sem_wait().. Only change the state if it's still
     * pre-switchover.
     */
    migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER, <====== [2]
                      MIGRATION_STATUS_DEVICE);

    return s->state == MIGRATION_STATUS_DEVICE;
}

So when holding BQL logically it can't change to CANCELLING, it'll check
first [1] making sure no prior CANCELLING.  Then after release and retake
BQL it'll check again [2] (see the comment above [2], it's done by passing
in explicit old_state to not change it if it's CANCELLING).

Any hint on how this could be triggered?

OTOH, when looking at this.. I seem to have found a bug indeed (which could
be another?), where I may have forgot to touch up the old_state in
migrate_set_state() after switching to always use DEVICE..

diff --git a/migration/migration.c b/migration/migration.c
index 74c50cc72c..513e5955cc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2793,8 +2793,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
 fail_closefb:
     qemu_fclose(fb);
 fail:
-    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
+    if (ms->state != MIGRATION_STATUS_CANCELLING) {
+        migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
+    }
     migration_block_activate(NULL);
     migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
     bql_unlock();

I'm not sure whether it's relevant to what you hit, though.. since you're
looking at this, I'd rely on you help figuring it out before I do.. :)

> ---
>  migration/migration.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 375de6d460..5dc43bcdc0 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2986,7 +2986,9 @@ fail:
>          error_free(local_err);
>      }
>  
> -    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +    if (s->state != MIGRATION_STATUS_CANCELLING) {
> +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +    }
>  }
>  
>  /**
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v2 6/9] migration: Don't set FAILED state when cancelling

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Feb 11, 2025 at 12:01:33PM -0300, Fabiano Rosas wrote:
>> It's possible that the migration is cancelled during
>> migration_switchover_start(). In that case, don't set the migration
>> state FAILED in migration_completion().
>> 
>> Fixes: 3dde8fdbad ("migration: Merge precopy/postcopy on switchover start")
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> I remember I paid some attention on this one when working on the commit,
> where it has:
>
> static bool migration_switchover_prepare(MigrationState *s)
> {
>     /* Concurrent cancellation?  Quit */
>     if (s->state == MIGRATION_STATUS_CANCELLING) {   <================= [1]
>         return false;
>     }
>     ...
>     bql_unlock();
>
>     qemu_sem_wait(&s->pause_sem);
>
>     bql_lock();
>     /*
>      * After BQL released and retaken, the state can be CANCELLING if it
>      * happend during sem_wait().. Only change the state if it's still
>      * pre-switchover.
>      */
>     migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER, <====== [2]
>                       MIGRATION_STATUS_DEVICE);
>
>     return s->state == MIGRATION_STATUS_DEVICE;
> }
>
> So when holding BQL logically it can't change to CANCELLING, it'll check
> first [1] making sure no prior CANCELLING.  Then after release and retake
> BQL it'll check again [2] (see the comment above [2], it's done by passing
> in explicit old_state to not change it if it's CANCELLING).

Right, it doesn't change the state. But the function returns false and
someone else changes to FAILED. That's what both my patch and your
snippet below fix.

>
> Any hint on how this could be triggered?
>
> OTOH, when looking at this.. I seem to have found a bug indeed (which could
> be another?), where I may have forgot to touch up the old_state in
> migrate_set_state() after switching to always use DEVICE..
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 74c50cc72c..513e5955cc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2793,8 +2793,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>  fail_closefb:
>      qemu_fclose(fb);
>  fail:
> -    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> -                          MIGRATION_STATUS_FAILED);
> +    if (ms->state != MIGRATION_STATUS_CANCELLING) {
> +        migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
> +    }

Now that I think about it, we should probably just use the skip at
migrate_set_state() always. Isn't this^ the same as:

migrate_set_state(&ms->state, MIGRATION_STATUS_DEVICE,
MIGRATION_STATUS_FAILED);

Better to list the state explicitly, no?

Or... do we want to incorporate this into migrate_set_state()?

void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
                       MigrationStatus new_state)
{
    assert(new_state < MIGRATION_STATUS__MAX);

    if (qatomic_read(state) == CANCELLING && new_state != CANCELLED) {
        /* Once it's cancelling, there's no way back, it must finish cancel */
        return;
    }

    if (qatomic_cmpxchg(state, old_state, new_state) == old_state) {
        trace_migrate_set_state(MigrationStatus_str(new_state));
        migrate_generate_event(new_state);
    }
}

>      migration_block_activate(NULL);
>      migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
>      bql_unlock();
>
> I'm not sure whether it's relevant to what you hit, though.. since you're
> looking at this, I'd rely on you help figuring it out before I do.. :)
>
>> ---
>>  migration/migration.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 375de6d460..5dc43bcdc0 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2986,7 +2986,9 @@ fail:
>>          error_free(local_err);
>>      }
>>  
>> -    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> +    if (s->state != MIGRATION_STATUS_CANCELLING) {
>> +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> +    }
>>  }
>>  
>>  /**
>> -- 
>> 2.35.3
>> 

Re: [PATCH v2 6/9] migration: Don't set FAILED state when cancelling

[Peter Xu]

On Tue, Feb 11, 2025 at 03:04:37PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Feb 11, 2025 at 12:01:33PM -0300, Fabiano Rosas wrote:
> >> It's possible that the migration is cancelled during
> >> migration_switchover_start(). In that case, don't set the migration
> >> state FAILED in migration_completion().
> >> 
> >> Fixes: 3dde8fdbad ("migration: Merge precopy/postcopy on switchover start")
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >
> > I remember I paid some attention on this one when working on the commit,
> > where it has:
> >
> > static bool migration_switchover_prepare(MigrationState *s)
> > {
> >     /* Concurrent cancellation?  Quit */
> >     if (s->state == MIGRATION_STATUS_CANCELLING) {   <================= [1]
> >         return false;
> >     }
> >     ...
> >     bql_unlock();
> >
> >     qemu_sem_wait(&s->pause_sem);
> >
> >     bql_lock();
> >     /*
> >      * After BQL released and retaken, the state can be CANCELLING if it
> >      * happend during sem_wait().. Only change the state if it's still
> >      * pre-switchover.
> >      */
> >     migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER, <====== [2]
> >                       MIGRATION_STATUS_DEVICE);
> >
> >     return s->state == MIGRATION_STATUS_DEVICE;
> > }
> >
> > So when holding BQL logically it can't change to CANCELLING, it'll check
> > first [1] making sure no prior CANCELLING.  Then after release and retake
> > BQL it'll check again [2] (see the comment above [2], it's done by passing
> > in explicit old_state to not change it if it's CANCELLING).
> 
> Right, it doesn't change the state. But the function returns false and
> someone else changes to FAILED. That's what both my patch and your
> snippet below fix.
> 
> >
> > Any hint on how this could be triggered?
> >
> > OTOH, when looking at this.. I seem to have found a bug indeed (which could
> > be another?), where I may have forgot to touch up the old_state in
> > migrate_set_state() after switching to always use DEVICE..
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 74c50cc72c..513e5955cc 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2793,8 +2793,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
> >  fail_closefb:
> >      qemu_fclose(fb);
> >  fail:
> > -    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > -                          MIGRATION_STATUS_FAILED);
> > +    if (ms->state != MIGRATION_STATUS_CANCELLING) {
> > +        migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
> > +    }
> 
> Now that I think about it, we should probably just use the skip at
> migrate_set_state() always. Isn't this^ the same as:
> 
> migrate_set_state(&ms->state, MIGRATION_STATUS_DEVICE,
> MIGRATION_STATUS_FAILED);
> 
> Better to list the state explicitly, no?

There's one case where it can be in ACTIVE rather than DEVICE,
unfortunately:

    ret = migration_stop_vm(ms, RUN_STATE_FINISH_MIGRATE);
    if (ret < 0) {
        error_setg_errno(errp, -ret, "%s: Failed to stop the VM", __func__);
        goto fail;
    }

> 
> Or... do we want to incorporate this into migrate_set_state()?
> 
> void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>                        MigrationStatus new_state)
> {
>     assert(new_state < MIGRATION_STATUS__MAX);
> 
>     if (qatomic_read(state) == CANCELLING && new_state != CANCELLED) {
>         /* Once it's cancelling, there's no way back, it must finish cancel */
>         return;
>     }
> 
>     if (qatomic_cmpxchg(state, old_state, new_state) == old_state) {
>         trace_migrate_set_state(MigrationStatus_str(new_state));
>         migrate_generate_event(new_state);
>     }
> }

IMHO we'll need the original migrate_set_state() more or less, e.g. when
setting CANCELLING->CANCELLED in migration_[fd_]cleanup().  So maybe it's
slightly easier we keep it.

Said that, maybe we could have a few helpers for the state transitions,
like:

  migrate_set_state_failure(MigrationState *s)

Which can consider CANCELLING.

Also, we have a portion of such state transitions not caring about current
state, so we could also have some helper for that, like:

  migrate_set_state_always(MigrationState *s, MigrationStatus status)

Or rename old migrate_set_state() into migrate_set_state_atomic(), then
make migrate_set_state() to ignore current state.

> 
> >      migration_block_activate(NULL);
> >      migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
> >      bql_unlock();
> >
> > I'm not sure whether it's relevant to what you hit, though.. since you're
> > looking at this, I'd rely on you help figuring it out before I do.. :)
> >
> >> ---
> >>  migration/migration.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 375de6d460..5dc43bcdc0 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2986,7 +2986,9 @@ fail:
> >>          error_free(local_err);
> >>      }
> >>  
> >> -    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> >> +    if (s->state != MIGRATION_STATUS_CANCELLING) {
> >> +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> >> +    }
> >>  }
> >>  
> >>  /**
> >> -- 
> >> 2.35.3
> >> 
> 

-- 
Peter Xu

Re: [PATCH v2 7/9] tests/qtest/migration: Introduce migration_test_add_suffix

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:34PM -0300, Fabiano Rosas wrote:
> Introduce a new migration_test_add_suffix to allow programmatic
> creation of tests based on a suffix. Pass the test name into the test
> so it can know which variant to run.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 8/9] tests/qtest/migration: Add a cancel test

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:35PM -0300, Fabiano Rosas wrote:
> The qmp_migrate_cancel() command is poorly tested and code inspection
> reveals that there might be concurrency issues with its usage. Add a
> test that runs a migration and calls qmp_migrate_cancel() at specific
> moments.
> 
> In order to make the test more deterministic, instead of calling
> qmp_migrate_cancel() at random moments during migration, do it after
> the migration status change events are seen.
> 
> The expected result is that qmp_migrate_cancel() on the source ends
> migration on the source with the "cancelled" state and ends migration
> on the destination with the "failed" state. The only exception is that
> a failed migration should continue in the failed state.
> 
> Cancelling is not allowed during postcopy (no test is added for this
> because it's a trivial check in the code).
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration/precopy-tests.c | 176 ++++++++++++++++++++++++++
>  1 file changed, 176 insertions(+)
> 
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index 162fa69531..ba273d10b9 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -20,6 +20,7 @@
>  #include "migration/migration-util.h"
>  #include "ppc-util.h"
>  #include "qobject/qlist.h"
> +#include "qapi-types-migration.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/range.h"
> @@ -536,6 +537,161 @@ static void test_multifd_tcp_cancel(void)
>      migrate_end(from, to2, true);
>  }
>  
> +static void test_cancel_src_after_failed(QTestState *from, QTestState *to,
> +                                         const char *uri, const char *phase)
> +{
> +    /*
> +     * No migrate_incoming_qmp() at the start to force source into
> +     * failed state during migrate_qmp().
> +     */
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +
> +    /* cancelling will not move the migration out of 'failed' */
> +
> +    wait_for_migration_status(from, "failed",
> +                              (const char * []) { "completed", NULL });
> +
> +    /*
> +     * Not waiting for the destination because it never started
> +     * migration.
> +     */
> +}
> +
> +static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to,
> +                                            const char *uri, const char *phase)
> +{
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    /* To move to cancelled/cancelling */
> +    migrate_cancel(from);
> +    migration_event_wait(from, phase);
> +
> +    /* The migrate_cancel under test */
> +    migrate_cancel(from);
> +
> +    wait_for_migration_status(from, "cancelled",
> +                              (const char * []) { "completed", NULL });
> +
> +    wait_for_migration_status(to, "failed",
> +                              (const char * []) { "completed", NULL });
> +}
> +
> +static void test_cancel_src_after_complete(QTestState *from, QTestState *to,
> +                                           const char *uri, const char *phase)
> +{
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +
> +    /*
> +     * qmp_migrate_cancel() exits early if migration is not running
> +     * anymore, the status will not change to cancelled.
> +     */
> +    wait_for_migration_complete(from);
> +    wait_for_migration_complete(to);
> +}
> +
> +static void test_cancel_src_after_none(QTestState *from, QTestState *to,
> +                                       const char *uri, const char *phase)
> +{
> +    /*
> +     * Test that cancelling without a migration happening does not
> +     * affect subsequent migrations
> +     */
> +    migrate_cancel(to);
> +
> +    wait_for_serial("src_serial");
> +    migrate_cancel(from);
> +
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    migrate_ensure_converge(from);
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    wait_for_migration_complete(from);
> +    wait_for_migration_complete(to);
> +}
> +
> +static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> +                                           const char *uri, const char *phase)
> +{
> +    migrate_set_capability(from, "pause-before-switchover", true);
> +    migrate_set_capability(to, "pause-before-switchover", true);
> +
> +    migrate_set_capability(from, "multifd", true);
> +    migrate_set_capability(to, "multifd", true);
> +
> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
> +
> +    wait_for_serial("src_serial");
> +    migrate_ensure_converge(from);
> +
> +    migrate_qmp(from, to, uri, NULL, "{}");
> +
> +    migration_event_wait(from, phase);
> +    migrate_cancel(from);
> +    migration_event_wait(from, "cancelling");
> +
> +    wait_for_migration_status(from, "cancelled",
> +                              (const char * []) { "completed", NULL });
> +
> +    wait_for_migration_status(to, "failed",
> +                              (const char * []) { "completed", NULL });
> +}
> +
> +static void test_cancel_src_after_status(void *opaque)
> +{
> +    const char *test_path = opaque;
> +    g_autofree char *phase = g_path_get_basename(test_path);
> +    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> +    QTestState *from, *to;
> +    MigrateStart args = {
> +        .hide_stderr = true,
> +    };
> +
> +    if (migrate_start(&from, &to, "defer", &args)) {
> +        return;
> +    }
> +
> +    if (g_str_equal(phase, "cancelling") ||
> +        g_str_equal(phase, "cancelled")) {
> +        test_cancel_src_after_cancelled(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "completed")) {
> +        test_cancel_src_after_complete(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "failed")) {
> +        test_cancel_src_after_failed(from, to, uri, phase);
> +
> +    } else if (g_str_equal(phase, "none")) {
> +        test_cancel_src_after_none(from, to, uri, phase);
> +
> +    } else {
> +        /* any state that comes before pre-switchover */
> +        test_cancel_src_pre_switchover(from, to, uri, phase);

[1]

> +    }
> +
> +    migrate_end(from, to, false);
> +}

I'm OK with the current status, considering it at least enlarge our cancel
testcases so definitely good to have:

Reviewed-by: Peter Xu <peterx@redhat.com>

Though one thing to mention is the new "test_full()" abstraction doesn't
yet look like to benefit us a huge lot, IMHO.

These are the new tests:

# /x86_64/migration/cancel/src/after/none
# /x86_64/migration/cancel/src/after/setup             [*]
# /x86_64/migration/cancel/src/after/cancelling
# /x86_64/migration/cancel/src/after/cancelled
# /x86_64/migration/cancel/src/after/active
# /x86_64/migration/cancel/src/after/completed
# /x86_64/migration/cancel/src/after/failed
# /x86_64/migration/cancel/src/after/pre-switchover    [*]

We have only one abstracted path [1] to test random status, but that so far
only covers two cases marked with [*].  It is hard to say whether the
abstraction is necessary, or maybe it's easier we always register separate
test cases.  So it's still slightly debatable whether we could make all
above "if .. if else .. if else ... else" into separate tests.

> +
>  static void calc_dirty_rate(QTestState *who, uint64_t calc_time)
>  {
>      qtest_qmp_assert_success(who,
> @@ -1018,4 +1174,24 @@ void migration_test_add_precopy(MigrationTestEnv *env)
>                                 test_vcpu_dirty_limit);
>          }
>      }
> +
> +    /* ensure new status don't go unnoticed */
> +    assert(MIGRATION_STATUS__MAX == 15);
> +
> +    for (int i = MIGRATION_STATUS_NONE; i < MIGRATION_STATUS__MAX; i++) {
> +        switch (i) {
> +        case MIGRATION_STATUS_DEVICE: /* happens too fast */
> +        case MIGRATION_STATUS_WAIT_UNPLUG: /* no support in tests */
> +        case MIGRATION_STATUS_COLO: /* no support in tests */
> +        case MIGRATION_STATUS_POSTCOPY_ACTIVE: /* postcopy can't be cancelled */
> +        case MIGRATION_STATUS_POSTCOPY_PAUSED:
> +        case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
> +        case MIGRATION_STATUS_POSTCOPY_RECOVER:
> +            continue;
> +        default:
> +            migration_test_add_suffix("/migration/cancel/src/after/",
> +                                      MigrationStatus_str(i),
> +                                      test_cancel_src_after_status);
> +        }
> +    }
>  }
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v2 9/9] migration: Update migrate_cancel documentation

[Peter Xu]

On Tue, Feb 11, 2025 at 12:01:36PM -0300, Fabiano Rosas wrote:
> Update the migrate_cancel command documentation with a few words about
> postcopy and the expected state of the machine after migration.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 6/9] migration: Don't set FAILED state when cancelling

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Feb 11, 2025 at 03:04:37PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Feb 11, 2025 at 12:01:33PM -0300, Fabiano Rosas wrote:
>> >> It's possible that the migration is cancelled during
>> >> migration_switchover_start(). In that case, don't set the migration
>> >> state FAILED in migration_completion().
>> >> 
>> >> Fixes: 3dde8fdbad ("migration: Merge precopy/postcopy on switchover start")
>> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >
>> > I remember I paid some attention on this one when working on the commit,
>> > where it has:
>> >
>> > static bool migration_switchover_prepare(MigrationState *s)
>> > {
>> >     /* Concurrent cancellation?  Quit */
>> >     if (s->state == MIGRATION_STATUS_CANCELLING) {   <================= [1]
>> >         return false;
>> >     }
>> >     ...
>> >     bql_unlock();
>> >
>> >     qemu_sem_wait(&s->pause_sem);
>> >
>> >     bql_lock();
>> >     /*
>> >      * After BQL released and retaken, the state can be CANCELLING if it
>> >      * happend during sem_wait().. Only change the state if it's still
>> >      * pre-switchover.
>> >      */
>> >     migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER, <====== [2]
>> >                       MIGRATION_STATUS_DEVICE);
>> >
>> >     return s->state == MIGRATION_STATUS_DEVICE;
>> > }
>> >
>> > So when holding BQL logically it can't change to CANCELLING, it'll check
>> > first [1] making sure no prior CANCELLING.  Then after release and retake
>> > BQL it'll check again [2] (see the comment above [2], it's done by passing
>> > in explicit old_state to not change it if it's CANCELLING).
>> 
>> Right, it doesn't change the state. But the function returns false and
>> someone else changes to FAILED. That's what both my patch and your
>> snippet below fix.
>> 
>> >
>> > Any hint on how this could be triggered?
>> >
>> > OTOH, when looking at this.. I seem to have found a bug indeed (which could
>> > be another?), where I may have forgot to touch up the old_state in
>> > migrate_set_state() after switching to always use DEVICE..
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 74c50cc72c..513e5955cc 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -2793,8 +2793,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>> >  fail_closefb:
>> >      qemu_fclose(fb);
>> >  fail:
>> > -    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>> > -                          MIGRATION_STATUS_FAILED);
>> > +    if (ms->state != MIGRATION_STATUS_CANCELLING) {
>> > +        migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
>> > +    }
>> 
>> Now that I think about it, we should probably just use the skip at
>> migrate_set_state() always. Isn't this^ the same as:
>> 
>> migrate_set_state(&ms->state, MIGRATION_STATUS_DEVICE,
>> MIGRATION_STATUS_FAILED);
>> 
>> Better to list the state explicitly, no?
>
> There's one case where it can be in ACTIVE rather than DEVICE,
> unfortunately:
>
>     ret = migration_stop_vm(ms, RUN_STATE_FINISH_MIGRATE);
>     if (ret < 0) {
>         error_setg_errno(errp, -ret, "%s: Failed to stop the VM", __func__);
>         goto fail;
>     }
>
>> 
>> Or... do we want to incorporate this into migrate_set_state()?
>> 
>> void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>>                        MigrationStatus new_state)
>> {
>>     assert(new_state < MIGRATION_STATUS__MAX);
>> 
>>     if (qatomic_read(state) == CANCELLING && new_state != CANCELLED) {
>>         /* Once it's cancelling, there's no way back, it must finish cancel */
>>         return;
>>     }
>> 
>>     if (qatomic_cmpxchg(state, old_state, new_state) == old_state) {
>>         trace_migrate_set_state(MigrationStatus_str(new_state));
>>         migrate_generate_event(new_state);
>>     }
>> }
>
> IMHO we'll need the original migrate_set_state() more or less, e.g. when
> setting CANCELLING->CANCELLED in migration_[fd_]cleanup().  So maybe it's
> slightly easier we keep it.
>
> Said that, maybe we could have a few helpers for the state transitions,
> like:
>
>   migrate_set_state_failure(MigrationState *s)
>
> Which can consider CANCELLING.
>
> Also, we have a portion of such state transitions not caring about current
> state, so we could also have some helper for that, like:
>
>   migrate_set_state_always(MigrationState *s, MigrationStatus status)
>
> Or rename old migrate_set_state() into migrate_set_state_atomic(), then
> make migrate_set_state() to ignore current state.
>

Thanks for the input.

I think for this series I'll stick with the

if (s->state != MIGRATION_STATUS_CANCELLING)

and prepare a new series with generic improvements to the migration
state code. I want to also fix the nomenclature of status vs. state.

>> 
>> >      migration_block_activate(NULL);
>> >      migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
>> >      bql_unlock();
>> >
>> > I'm not sure whether it's relevant to what you hit, though.. since you're
>> > looking at this, I'd rely on you help figuring it out before I do.. :)
>> >
>> >> ---
>> >>  migration/migration.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/migration/migration.c b/migration/migration.c
>> >> index 375de6d460..5dc43bcdc0 100644
>> >> --- a/migration/migration.c
>> >> +++ b/migration/migration.c
>> >> @@ -2986,7 +2986,9 @@ fail:
>> >>          error_free(local_err);
>> >>      }
>> >>  
>> >> -    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> >> +    if (s->state != MIGRATION_STATUS_CANCELLING) {
>> >> +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> >> +    }
>> >>  }
>> >>  
>> >>  /**
>> >> -- 
>> >> 2.35.3
>> >> 
>> 

Re: [PATCH v2 8/9] tests/qtest/migration: Add a cancel test

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Feb 11, 2025 at 12:01:35PM -0300, Fabiano Rosas wrote:
>> The qmp_migrate_cancel() command is poorly tested and code inspection
>> reveals that there might be concurrency issues with its usage. Add a
>> test that runs a migration and calls qmp_migrate_cancel() at specific
>> moments.
>> 
>> In order to make the test more deterministic, instead of calling
>> qmp_migrate_cancel() at random moments during migration, do it after
>> the migration status change events are seen.
>> 
>> The expected result is that qmp_migrate_cancel() on the source ends
>> migration on the source with the "cancelled" state and ends migration
>> on the destination with the "failed" state. The only exception is that
>> a failed migration should continue in the failed state.
>> 
>> Cancelling is not allowed during postcopy (no test is added for this
>> because it's a trivial check in the code).
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration/precopy-tests.c | 176 ++++++++++++++++++++++++++
>>  1 file changed, 176 insertions(+)
>> 
>> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
>> index 162fa69531..ba273d10b9 100644
>> --- a/tests/qtest/migration/precopy-tests.c
>> +++ b/tests/qtest/migration/precopy-tests.c
>> @@ -20,6 +20,7 @@
>>  #include "migration/migration-util.h"
>>  #include "ppc-util.h"
>>  #include "qobject/qlist.h"
>> +#include "qapi-types-migration.h"
>>  #include "qemu/module.h"
>>  #include "qemu/option.h"
>>  #include "qemu/range.h"
>> @@ -536,6 +537,161 @@ static void test_multifd_tcp_cancel(void)
>>      migrate_end(from, to2, true);
>>  }
>>  
>> +static void test_cancel_src_after_failed(QTestState *from, QTestState *to,
>> +                                         const char *uri, const char *phase)
>> +{
>> +    /*
>> +     * No migrate_incoming_qmp() at the start to force source into
>> +     * failed state during migrate_qmp().
>> +     */
>> +
>> +    wait_for_serial("src_serial");
>> +    migrate_ensure_converge(from);
>> +
>> +    migrate_qmp(from, to, uri, NULL, "{}");
>> +
>> +    migration_event_wait(from, phase);
>> +    migrate_cancel(from);
>> +
>> +    /* cancelling will not move the migration out of 'failed' */
>> +
>> +    wait_for_migration_status(from, "failed",
>> +                              (const char * []) { "completed", NULL });
>> +
>> +    /*
>> +     * Not waiting for the destination because it never started
>> +     * migration.
>> +     */
>> +}
>> +
>> +static void test_cancel_src_after_cancelled(QTestState *from, QTestState *to,
>> +                                            const char *uri, const char *phase)
>> +{
>> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
>> +
>> +    wait_for_serial("src_serial");
>> +    migrate_ensure_converge(from);
>> +
>> +    migrate_qmp(from, to, uri, NULL, "{}");
>> +
>> +    /* To move to cancelled/cancelling */
>> +    migrate_cancel(from);
>> +    migration_event_wait(from, phase);
>> +
>> +    /* The migrate_cancel under test */
>> +    migrate_cancel(from);
>> +
>> +    wait_for_migration_status(from, "cancelled",
>> +                              (const char * []) { "completed", NULL });
>> +
>> +    wait_for_migration_status(to, "failed",
>> +                              (const char * []) { "completed", NULL });
>> +}
>> +
>> +static void test_cancel_src_after_complete(QTestState *from, QTestState *to,
>> +                                           const char *uri, const char *phase)
>> +{
>> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
>> +
>> +    wait_for_serial("src_serial");
>> +    migrate_ensure_converge(from);
>> +
>> +    migrate_qmp(from, to, uri, NULL, "{}");
>> +
>> +    migration_event_wait(from, phase);
>> +    migrate_cancel(from);
>> +
>> +    /*
>> +     * qmp_migrate_cancel() exits early if migration is not running
>> +     * anymore, the status will not change to cancelled.
>> +     */
>> +    wait_for_migration_complete(from);
>> +    wait_for_migration_complete(to);
>> +}
>> +
>> +static void test_cancel_src_after_none(QTestState *from, QTestState *to,
>> +                                       const char *uri, const char *phase)
>> +{
>> +    /*
>> +     * Test that cancelling without a migration happening does not
>> +     * affect subsequent migrations
>> +     */
>> +    migrate_cancel(to);
>> +
>> +    wait_for_serial("src_serial");
>> +    migrate_cancel(from);
>> +
>> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
>> +
>> +    migrate_ensure_converge(from);
>> +    migrate_qmp(from, to, uri, NULL, "{}");
>> +
>> +    wait_for_migration_complete(from);
>> +    wait_for_migration_complete(to);
>> +}
>> +
>> +static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
>> +                                           const char *uri, const char *phase)
>> +{
>> +    migrate_set_capability(from, "pause-before-switchover", true);
>> +    migrate_set_capability(to, "pause-before-switchover", true);
>> +
>> +    migrate_set_capability(from, "multifd", true);
>> +    migrate_set_capability(to, "multifd", true);
>> +
>> +    migrate_incoming_qmp(to, uri, NULL, "{ 'exit-on-error': false }");
>> +
>> +    wait_for_serial("src_serial");
>> +    migrate_ensure_converge(from);
>> +
>> +    migrate_qmp(from, to, uri, NULL, "{}");
>> +
>> +    migration_event_wait(from, phase);
>> +    migrate_cancel(from);
>> +    migration_event_wait(from, "cancelling");
>> +
>> +    wait_for_migration_status(from, "cancelled",
>> +                              (const char * []) { "completed", NULL });
>> +
>> +    wait_for_migration_status(to, "failed",
>> +                              (const char * []) { "completed", NULL });
>> +}
>> +
>> +static void test_cancel_src_after_status(void *opaque)
>> +{
>> +    const char *test_path = opaque;
>> +    g_autofree char *phase = g_path_get_basename(test_path);
>> +    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>> +    QTestState *from, *to;
>> +    MigrateStart args = {
>> +        .hide_stderr = true,
>> +    };
>> +
>> +    if (migrate_start(&from, &to, "defer", &args)) {
>> +        return;
>> +    }
>> +
>> +    if (g_str_equal(phase, "cancelling") ||
>> +        g_str_equal(phase, "cancelled")) {
>> +        test_cancel_src_after_cancelled(from, to, uri, phase);
>> +
>> +    } else if (g_str_equal(phase, "completed")) {
>> +        test_cancel_src_after_complete(from, to, uri, phase);
>> +
>> +    } else if (g_str_equal(phase, "failed")) {
>> +        test_cancel_src_after_failed(from, to, uri, phase);
>> +
>> +    } else if (g_str_equal(phase, "none")) {
>> +        test_cancel_src_after_none(from, to, uri, phase);
>> +
>> +    } else {
>> +        /* any state that comes before pre-switchover */
>> +        test_cancel_src_pre_switchover(from, to, uri, phase);
>
> [1]
>
>> +    }
>> +
>> +    migrate_end(from, to, false);
>> +}
>
> I'm OK with the current status, considering it at least enlarge our cancel
> testcases so definitely good to have:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Though one thing to mention is the new "test_full()" abstraction doesn't
> yet look like to benefit us a huge lot, IMHO.
>
> These are the new tests:
>
> # /x86_64/migration/cancel/src/after/none
> # /x86_64/migration/cancel/src/after/setup             [*]
> # /x86_64/migration/cancel/src/after/cancelling
> # /x86_64/migration/cancel/src/after/cancelled
> # /x86_64/migration/cancel/src/after/active
> # /x86_64/migration/cancel/src/after/completed
> # /x86_64/migration/cancel/src/after/failed
> # /x86_64/migration/cancel/src/after/pre-switchover    [*]
>
> We have only one abstracted path [1] to test random status, but that so far
> only covers two cases marked with [*].  It is hard to say whether the
> abstraction is necessary, or maybe it's easier we always register separate
> test cases.  So it's still slightly debatable whether we could make all
> above "if .. if else .. if else ... else" into separate tests.
>

It gets super boilerplatey:


    for (int i = MIGRATION_STATUS_NONE; i < MIGRATION_STATUS__MAX; i++) {
        switch (i) {
        case MIGRATION_STATUS_DEVICE:          /* happens too fast */
        case MIGRATION_STATUS_WAIT_UNPLUG:     /* no support in tests */
        case MIGRATION_STATUS_COLO:            /* no support in tests */
        case MIGRATION_STATUS_POSTCOPY_ACTIVE: /* postcopy can't be cancelled */
        case MIGRATION_STATUS_POSTCOPY_PAUSED:
        case MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP:
        case MIGRATION_STATUS_POSTCOPY_RECOVER:
            continue;
        case MIGRATION_STATUS_NONE:
            migration_test_add("/migration/cancel/src/after/none",
                               test_cancel_src_after_none);
            break;
        case MIGRATION_STATUS_SETUP:
            migration_test_add("/migration/cancel/src/after/setup",
                               test_cancel_src_after_setup);
            break;
        case MIGRATION_STATUS_CANCELLING:
            migration_test_add("/migration/cancel/src/after/cancelling",
                               test_cancel_src_after_cancelling);
            break;
        case MIGRATION_STATUS_CANCELLED:
            migration_test_add("/migration/cancel/src/after/cancelled",
                               test_cancel_src_after_cancelled);
            break;
        case MIGRATION_STATUS_ACTIVE:
            migration_test_add("/migration/cancel/src/after/active",
                               test_cancel_src_after_active);
            break;
        case MIGRATION_STATUS_COMPLETED:
            migration_test_add("/migration/cancel/src/after/completed",
                               test_cancel_src_after_completed);
            break;
        case MIGRATION_STATUS_FAILED:
            migration_test_add("/migration/cancel/src/after/failed",
                               test_cancel_src_after_failed);
            break;
        case MIGRATION_STATUS_PRE_SWITCHOVER:
            migration_test_add("/migration/cancel/src/after/pre-switchover",
                               test_cancel_src_after_pre_switchover);
            break;
        }
    }

}

void test_cancel_src_after_cancelling(void)
{
    test_cancel_src_after_cancel("cancelling");
}

void test_cancel_src_after_cancelled(void)
{
    test_cancel_src_after_cancel("cancelled");
}

void test_cancel_src_after_setup(void)
{
    test_cancel_src_after("setup");
}

void test_cancel_src_after_active(void)
{
    test_cancel_src_after("active");
}

void test_cancel_src_after_pre_switchover(void)
{
    test_cancel_src_after("pre-switchover");
}

static void test_cancel_src_after_failed(void)
{
    ...
    migration_event_wait(from, "failed");
    ...
}

static void test_cancel_src_after_cancel(const char *phase)
{
    ...    
    migration_event_wait(from, phase);
    ...
}

static void test_cancel_src_after_complete(void)
{
    migration_event_wait(from, "complete");
    ...
}

static void test_cancel_src_after_none(void)
{
    ...
}

static void test_cancel_src_after(const char *phase)
{
   ...
   migration_event_wait(from, phase);
   ...
}

Re: [PATCH v2 8/9] tests/qtest/migration: Add a cancel test

[Peter Xu]

On Tue, Feb 11, 2025 at 06:23:20PM -0300, Fabiano Rosas wrote:
> It gets super boilerplatey:

Let's go with what we have.

Thanks,

-- 
Peter Xu