[PATCH 0/5] migration: Fix the BDRV_O_INACTIVE assertion

[PATCH 0/5] migration: Fix the BDRV_O_INACTIVE assertion

[Fabiano Rosas]

At this point everyone is probably familiar with the assertion:

  bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
  BDRV_O_INACTIVE)' failed.

The issue is that at the end of migration, the block layer is
deactivated to release the locks on the disks so that in a shared
storage scenario the migration destination can take over. Trying to
deactivate twice leads to the above assertion.

The reason deactivation is happening twice is due to the migration
capability "late-block-activate", which delays activation until the
destination VM receives a qmp_cont command. The reasoning for that cap
is:

    Activating the block devices causes the locks to be taken on
    the backing file.  If we're running with -S and the destination libvirt
    hasn't started the destination with 'cont', it's expecting the locks are
    still untaken.

    Don't activate the block devices if we're not going to autostart the VM;
    'cont' already will do that anyway.   This change is tied to the new
    migration capability 'late-block-activate' that defaults to off, keeping
    the old behaviour by default.

    bz: https://bugzilla.redhat.com/show_bug.cgi?id=1560854

However, the expected qmp_cont command never happens, because the user
may never decide to continue the VM and instead attempt another
migration while the VM is paused.

Fix this by assuming that the qmp_migrate command having been issued
is enough indication that it is now ok to take the locks again
(similarly to qmp_cont) and call bdrv_activate_all() at that
moment. This is the least invasive fix that won't require us to add
new qmp commands and go change libvirt, etc.

I'm also copying the block list (for obvious reasons, but also)
because Vladimir pointed out that asserts of this kind might not only
happen during migration. I know of at least
bdrv_co_write_req_prepare() which will assert if reached when a guest
is paused after a migration that used late-block-activate.

Patches 1-3 are just shuffling code;

Patch 4 is the fix;

Patch 5 is the test. To reproduce the issue revert patch 4 and run:

QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test -p \
/x86_64/migration/precopy/ping-pong/late-block-activate

References:
https://gitlab.com/qemu-project/qemu/-/issues/686
https://gitlab.com/qemu-project/qemu/-/issues/2395
https://lore.kernel.org/r/20240924125611.664315-1-andrey.drobyshev@virtuozzo.com

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

Fabiano Rosas (5):
  tests/qtest/migration: Move more code under only_target
  tests/qtest/migration: Don't use hardcoded strings for -serial
  tests/qtest/migration: Support cleaning up only one side of migration
  migration: Activate block devices if VM is paused when migrating
  tests/qtest/migration: Test successive migrations

 migration/migration.c           |  19 +++
 tests/qtest/migration-helpers.c |   8 +
 tests/qtest/migration-helpers.h |   2 +
 tests/qtest/migration-test.c    | 252 +++++++++++++++++++++++++-------
 4 files changed, 226 insertions(+), 55 deletions(-)


base-commit: 134b443512825bed401b6e141447b8cdc22d2efe
-- 
2.35.3

[PATCH 1/5] tests/qtest/migration: Move more code under only_target

[Fabiano Rosas]

The only_target option's purpose is to make sure only the destination
QTestState machine is initialized. This allows the test code to retain
an already initialized source machine (e.g. for doing ping pong
migration).

We have drifted from that a bit when adding new code, so move some
lines under only_target to restore the functionality.

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e6a2803e71..d56894dd97 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -717,7 +717,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     g_autofree gchar *arch_target = NULL;
     /* options for source and target */
     g_autofree gchar *arch_opts = NULL;
-    g_autofree gchar *cmd_source = NULL;
     g_autofree gchar *cmd_target = NULL;
     const gchar *ignore_stderr;
     g_autofree char *shmem_opts = NULL;
@@ -735,10 +734,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         }
     }
 
-    dst_state = (QTestMigrationState) { };
-    src_state = (QTestMigrationState) { };
     bootfile_create(tmpfs, args->suspend_me);
-    src_state.suspend_me = args->suspend_me;
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         memory_size = "150M";
@@ -817,27 +813,35 @@ static int test_migrate_start(QTestState **from, QTestState **to,
 
     g_test_message("Using machine type: %s", machine);
 
-    cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
-                                 "-machine %s,%s "
-                                 "-name source,debug-threads=on "
-                                 "-m %s "
-                                 "-serial file:%s/src_serial "
-                                 "%s %s %s %s %s",
-                                 kvm_opts ? kvm_opts : "",
-                                 machine, machine_opts,
-                                 memory_size, tmpfs,
-                                 arch_opts ? arch_opts : "",
-                                 arch_source ? arch_source : "",
-                                 shmem_opts ? shmem_opts : "",
-                                 args->opts_source ? args->opts_source : "",
-                                 ignore_stderr);
     if (!args->only_target) {
+        g_autofree gchar *cmd_source = NULL;
+
+        src_state = (QTestMigrationState) { };
+        src_state.suspend_me = args->suspend_me;
+
+        cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
+                                     "-machine %s,%s "
+                                     "-name source,debug-threads=on "
+                                     "-m %s "
+                                     "-serial file:%s/src_serial "
+                                     "%s %s %s %s %s",
+                                     kvm_opts ? kvm_opts : "",
+                                     machine, machine_opts,
+                                     memory_size, tmpfs,
+                                     arch_opts ? arch_opts : "",
+                                     arch_source ? arch_source : "",
+                                     shmem_opts ? shmem_opts : "",
+                                     args->opts_source ? args->opts_source : "",
+                                     ignore_stderr);
+
         *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source);
         qtest_qmp_set_event_callback(*from,
                                      migrate_watch_for_events,
                                      &src_state);
     }
 
+    dst_state = (QTestMigrationState) { };
+
     cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
                                  "-machine %s,%s "
                                  "-name target,debug-threads=on "
@@ -870,7 +874,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
      * Always enable migration events.  Libvirt always uses it, let's try
      * to mimic as closer as that.
      */
-    migrate_set_capability(*from, "events", true);
+    if (!args->only_target) {
+        migrate_set_capability(*from, "events", true);
+    }
     migrate_set_capability(*to, "events", true);
 
     return 0;
-- 
2.35.3

[PATCH 2/5] tests/qtest/migration: Don't use hardcoded strings for -serial

[Fabiano Rosas]

Stop using hardcoded strings for -serial so we can in the next patches
perform more than one migration in a row. Having the serial path
hardcoded means we cannot reuse the code when dst becomes the new src.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/migration-helpers.c |  8 ++++
 tests/qtest/migration-helpers.h |  2 +
 tests/qtest/migration-test.c    | 68 ++++++++++++++++++---------------
 3 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 0025933883..16e940d910 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -528,3 +528,11 @@ void migration_event_wait(QTestState *s, const char *target)
         qobject_unref(response);
     } while (!found);
 }
+
+char *migrate_get_unique_serial(const char *tmpfs)
+{
+    static int i;
+
+    assert(i < INT_MAX);
+    return g_strdup_printf("%s/serial_%d", tmpfs, i++);
+}
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 72dba369fb..c7a36a33d6 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -20,6 +20,7 @@ typedef struct QTestMigrationState {
     bool resume_seen;
     bool suspend_seen;
     bool suspend_me;
+    char *serial;
 } QTestMigrationState;
 
 bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -64,5 +65,6 @@ static inline bool probe_o_direct_support(const char *tmpfs)
 #endif
 void migration_test_add(const char *path, void (*fn)(void));
 void migration_event_wait(QTestState *s, const char *target);
+char *migrate_get_unique_serial(const char *tmpfs);
 
 #endif /* MIGRATION_HELPERS_H */
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d56894dd97..f8919a083b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -192,9 +192,8 @@ static void bootfile_create(char *dir, bool suspend_me)
  * we get an 'A' followed by an endless string of 'B's
  * but on the destination we won't have the A (unless we enabled suspend/resume)
  */
-static void wait_for_serial(const char *side)
+static void wait_for_serial(const char *serialpath)
 {
-    g_autofree char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
     FILE *serialfile = fopen(serialpath, "r");
 
     do {
@@ -216,7 +215,7 @@ static void wait_for_serial(const char *side)
             break;
 
         default:
-            fprintf(stderr, "Unexpected %d on %s serial\n", readvalue, side);
+            fprintf(stderr, "Unexpected %d on %s\n", readvalue, serialpath);
             g_assert_not_reached();
         }
     } while (true);
@@ -818,16 +817,17 @@ static int test_migrate_start(QTestState **from, QTestState **to,
 
         src_state = (QTestMigrationState) { };
         src_state.suspend_me = args->suspend_me;
+        src_state.serial = migrate_get_unique_serial(tmpfs);
 
         cmd_source = g_strdup_printf("-accel kvm%s -accel tcg "
                                      "-machine %s,%s "
                                      "-name source,debug-threads=on "
                                      "-m %s "
-                                     "-serial file:%s/src_serial "
+                                     "-serial file:%s "
                                      "%s %s %s %s %s",
                                      kvm_opts ? kvm_opts : "",
                                      machine, machine_opts,
-                                     memory_size, tmpfs,
+                                     memory_size, src_state.serial,
                                      arch_opts ? arch_opts : "",
                                      arch_source ? arch_source : "",
                                      shmem_opts ? shmem_opts : "",
@@ -841,17 +841,18 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     }
 
     dst_state = (QTestMigrationState) { };
+    dst_state.serial = migrate_get_unique_serial(tmpfs);
 
     cmd_target = g_strdup_printf("-accel kvm%s -accel tcg "
                                  "-machine %s,%s "
                                  "-name target,debug-threads=on "
                                  "-m %s "
-                                 "-serial file:%s/dest_serial "
+                                 "-serial file:%s "
                                  "-incoming %s "
                                  "%s %s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
                                  machine, machine_opts,
-                                 memory_size, tmpfs, uri,
+                                 memory_size, dst_state.serial, uri,
                                  arch_opts ? arch_opts : "",
                                  arch_target ? arch_target : "",
                                  shmem_opts ? shmem_opts : "",
@@ -911,8 +912,10 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
     qtest_quit(to);
 
     cleanup("migsocket");
-    cleanup("src_serial");
-    cleanup("dest_serial");
+    unlink(src_state.serial);
+    g_free(src_state.serial);
+    unlink(dst_state.serial);
+    g_free(dst_state.serial);
     cleanup(FILE_TEST_FILENAME);
 }
 
@@ -1290,7 +1293,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
                              "                'port': '0' } } ] } }");
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
     wait_for_suspend(from, &src_state);
 
     migrate_qmp(from, to, NULL, NULL, "{}");
@@ -1314,7 +1317,7 @@ static void migrate_postcopy_complete(QTestState *from, QTestState *to,
     }
 
     /* Make sure we get at least one "B" on destination */
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
 
     if (uffd_feature_thread_id) {
         read_blocktime(to);
@@ -1717,7 +1720,7 @@ static void test_precopy_common(MigrateCommon *args)
 
     /* Wait for the first serial output from the source */
     if (args->result == MIG_TEST_SUCCEED) {
-        wait_for_serial("src_serial");
+        wait_for_serial(src_state.serial);
         wait_for_suspend(from, &src_state);
     }
 
@@ -1794,7 +1797,7 @@ static void test_precopy_common(MigrateCommon *args)
             qtest_qmp_assert_success(to, "{'execute': 'system_wakeup'}");
         }
 
-        wait_for_serial("dest_serial");
+        wait_for_serial(dst_state.serial);
     }
 
 finish:
@@ -1869,7 +1872,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
     }
 
     migrate_ensure_converge(from);
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     if (stop_src) {
         qtest_qmp_assert_success(from, "{ 'execute' : 'stop'}");
@@ -1896,7 +1899,7 @@ static void test_file_common(MigrateCommon *args, bool stop_src)
     }
     wait_for_resume(to, &dst_state);
 
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
 
     if (check_offset) {
         file_check_offset_region();
@@ -2039,7 +2042,7 @@ static void test_ignore_shared(void)
     migrate_set_capability(to, "x-ignore-shared", true);
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     migrate_qmp(from, to, uri, NULL, "{}");
 
@@ -2049,7 +2052,7 @@ static void test_ignore_shared(void)
 
     qtest_qmp_eventwait(to, "RESUME");
 
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
     wait_for_migration_complete(from);
 
     /* Check whether shared RAM has been really skipped */
@@ -2665,7 +2668,7 @@ static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
     migrate_set_capability(from, "validate-uuid", true);
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     migrate_qmp(from, to, uri, NULL, "{}");
 
@@ -2729,7 +2732,7 @@ static void do_test_validate_uri_channel(MigrateCommon *args)
     }
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     /*
      * 'uri' and 'channels' validation is checked even before the migration
@@ -2819,7 +2822,7 @@ static void test_migrate_auto_converge(void)
     migrate_set_capability(from, "pause-before-switchover", true);
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     migrate_qmp(from, to, uri, NULL, "{}");
 
@@ -2881,7 +2884,7 @@ static void test_migrate_auto_converge(void)
 
     qtest_qmp_eventwait(to, "RESUME");
 
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
     wait_for_migration_complete(from);
 
     test_migrate_end(from, to, true);
@@ -3292,7 +3295,7 @@ static void test_multifd_tcp_cancel(void)
     migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 
     migrate_qmp(from, to, NULL, NULL, "{}");
 
@@ -3303,7 +3306,8 @@ static void test_multifd_tcp_cancel(void)
     /* Make sure QEMU process "to" exited */
     qtest_set_expected_status(to, EXIT_FAILURE);
     qtest_wait_qemu(to);
-    qtest_quit(to);
+    unlink(dst_state.serial);
+    g_free(dst_state.serial);
 
     /*
      * Ensure the source QEMU finishes its cancellation process before we
@@ -3341,7 +3345,7 @@ static void test_multifd_tcp_cancel(void)
     wait_for_stop(from, &src_state);
     qtest_qmp_eventwait(to2, "RESUME");
 
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
     wait_for_migration_complete(from);
     test_migrate_end(from, to2, true);
 }
@@ -3484,13 +3488,16 @@ static QTestState *dirtylimit_start_vm(void)
     QTestState *vm = NULL;
     g_autofree gchar *cmd = NULL;
 
+    src_state = (QTestMigrationState) { };
+    src_state.serial = migrate_get_unique_serial(tmpfs);
+
     bootfile_create(tmpfs, false);
     cmd = g_strdup_printf("-accel kvm,dirty-ring-size=4096 "
                           "-name dirtylimit-test,debug-threads=on "
                           "-m 150M -smp 1 "
-                          "-serial file:%s/vm_serial "
+                          "-serial file:%s "
                           "-drive file=%s,format=raw ",
-                          tmpfs, bootpath);
+                          src_state.serial, bootpath);
 
     vm = qtest_init(cmd);
     return vm;
@@ -3499,7 +3506,8 @@ static QTestState *dirtylimit_start_vm(void)
 static void dirtylimit_stop_vm(QTestState *vm)
 {
     qtest_quit(vm);
-    cleanup("vm_serial");
+    unlink(src_state.serial);
+    g_free(src_state.serial);
 }
 
 static void test_vcpu_dirty_limit(void)
@@ -3515,7 +3523,7 @@ static void test_vcpu_dirty_limit(void)
     vm = dirtylimit_start_vm();
 
     /* Wait for the first serial output from the vm*/
-    wait_for_serial("vm_serial");
+    wait_for_serial(src_state.serial);
 
     /* Do dirtyrate measurement with calc time equals 1s */
     calc_dirty_rate(vm, 1);
@@ -3608,7 +3616,7 @@ static void migrate_dirty_limit_wait_showup(QTestState *from,
     migrate_set_capability(from, "pause-before-switchover", true);
 
     /* Wait for the serial output from the source */
-    wait_for_serial("src_serial");
+    wait_for_serial(src_state.serial);
 }
 
 /*
@@ -3744,7 +3752,7 @@ static void test_migrate_dirty_limit(void)
 
     qtest_qmp_eventwait(to, "RESUME");
 
-    wait_for_serial("dest_serial");
+    wait_for_serial(dst_state.serial);
     wait_for_migration_complete(from);
 
     test_migrate_end(from, to, true);
-- 
2.35.3

[PATCH 3/5] tests/qtest/migration: Support cleaning up only one side of migration

[Fabiano Rosas]

We don't always want to cleanup both VMs at the same time. One example
is the multifd cancel test, where there's a second migration reusing
the source VM. The next patches will add another instance, keeping the
destination VM instead.

Extract the cleanup routine from test_migrate_end() into another
function so it can be reused.

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f8919a083b..f27dd93835 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -883,13 +883,29 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     return 0;
 }
 
+static void migrate_cleanup(QTestState *from, QTestState *to)
+{
+    if (from) {
+        qtest_quit(from);
+        unlink(src_state.serial);
+        g_free(src_state.serial);
+    }
+
+    if (to) {
+        qtest_quit(to);
+        unlink(dst_state.serial);
+        g_free(dst_state.serial);
+    }
+
+    cleanup("migsocket");
+    cleanup(FILE_TEST_FILENAME);
+}
+
 static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
 {
     unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
 
-    qtest_quit(from);
-
-    if (test_dest) {
+    if (to && test_dest) {
         qtest_memread(to, start_address, &dest_byte_a, 1);
 
         /* Destination still running, wait for a byte to change */
@@ -909,14 +925,7 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
         check_guests_ram(to);
     }
 
-    qtest_quit(to);
-
-    cleanup("migsocket");
-    unlink(src_state.serial);
-    g_free(src_state.serial);
-    unlink(dst_state.serial);
-    g_free(dst_state.serial);
-    cleanup(FILE_TEST_FILENAME);
+    migrate_cleanup(from, to);
 }
 
 #ifdef CONFIG_GNUTLS
@@ -3305,9 +3314,7 @@ static void test_multifd_tcp_cancel(void)
 
     /* Make sure QEMU process "to" exited */
     qtest_set_expected_status(to, EXIT_FAILURE);
-    qtest_wait_qemu(to);
-    unlink(dst_state.serial);
-    g_free(dst_state.serial);
+    migrate_cleanup(NULL, to);
 
     /*
      * Ensure the source QEMU finishes its cancellation process before we
-- 
2.35.3

[PATCH 4/5] migration: Activate block devices if VM is paused when migrating

[Fabiano Rosas]

Currently a VM that has been target of a migration using
late-block-activate will crash at the end of a new migration (with it
as source) when releasing ownership of the disks due to the VM having
never taken ownership of the disks in the first place.

The issue is that late-block-activate expects a qmp_continue command
to be issued at some point on the destination VM after the migration
finishes. If the user decides to never continue the VM, but instead
issue a new migration, then bdrv_activate_all() will never be called
and the assert will be reached:

bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
BDRV_O_INACTIVE)' failed.

Fix the issue by checking at the start of migration if the VM is
paused and call bdrv_activate_all() before migrating. Even if the
late-block-activate capability is not in play or if the VM has been
paused manually, there is no harm calling that function again.

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

diff --git a/migration/migration.c b/migration/migration.c
index aedf7f0751..26af30137b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         return false;
     }
 
+    /*
+     * The VM might have been target of a previous migration. If it
+     * was in the paused state then nothing will have required the
+     * block layer to be activated. Do it now to ensure this QEMU
+     * instance owns the disk locks.
+     */
+    if (!resume && runstate_check(RUN_STATE_PAUSED)) {
+        Error *local_err = NULL;
+
+        g_assert(bql_locked());
+
+        bdrv_activate_all(&local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return false;
+        }
+        s->block_inactive = false;
+    }
+
     return true;
 }
 
-- 
2.35.3

Re: [PATCH 4/5] migration: Activate block devices if VM is paused when migrating

[Peter Xu]

On Mon, Nov 25, 2024 at 11:46:11AM -0300, Fabiano Rosas wrote:
> Currently a VM that has been target of a migration using
> late-block-activate will crash at the end of a new migration (with it
> as source) when releasing ownership of the disks due to the VM having
> never taken ownership of the disks in the first place.
> 
> The issue is that late-block-activate expects a qmp_continue command
> to be issued at some point on the destination VM after the migration
> finishes. If the user decides to never continue the VM, but instead
> issue a new migration, then bdrv_activate_all() will never be called
> and the assert will be reached:
> 
> bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
> BDRV_O_INACTIVE)' failed.
> 
> Fix the issue by checking at the start of migration if the VM is
> paused and call bdrv_activate_all() before migrating. Even if the
> late-block-activate capability is not in play or if the VM has been
> paused manually, there is no harm calling that function again.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index aedf7f0751..26af30137b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          return false;
>      }
>  
> +    /*
> +     * The VM might have been target of a previous migration. If it
> +     * was in the paused state then nothing will have required the
> +     * block layer to be activated. Do it now to ensure this QEMU
> +     * instance owns the disk locks.
> +     */
> +    if (!resume && runstate_check(RUN_STATE_PAUSED)) {

I hope this will cover all the cases that QEMU could overlook.. or I'm not
sure whether we could invoke bdrv_activate_all() unconditionally, as it
looks like safe to be used if all disks are active already.

I wished we don't need to bother with disk activation status at all,
because IIUC migration could work all fine even if all disks are inactivate
when preparing migration.. hence such change always looks like a workaround
of a separate issue.

> +        Error *local_err = NULL;
> +
> +        g_assert(bql_locked());
> +
> +        bdrv_activate_all(&local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return false;
> +        }
> +        s->block_inactive = false;

This var so far was only used within one migration iteration, and the var
was only set in migration_completion_precopy() so far.  Now we're resetting
it upfront of a migration.  I'm not 100% sure if it's needed, or should be
put somewhere else.

In general, I saw the mention of other places that may also try to
invalidate disks that used to be invalidated.  If that's the case, I wish
we don't need to touch migration code at all, but instead if block layer
can cope with "invalidate on top of invalidated disks" it'll be perfect.

> +    }
> +
>      return true;
>  }
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH 4/5] migration: Activate block devices if VM is paused when migrating

[Andrey Drobyshev]

On 11/25/24 7:07 PM, Peter Xu wrote:
> On Mon, Nov 25, 2024 at 11:46:11AM -0300, Fabiano Rosas wrote:
>> Currently a VM that has been target of a migration using
>> late-block-activate will crash at the end of a new migration (with it
>> as source) when releasing ownership of the disks due to the VM having
>> never taken ownership of the disks in the first place.
>>
>> The issue is that late-block-activate expects a qmp_continue command
>> to be issued at some point on the destination VM after the migration
>> finishes. If the user decides to never continue the VM, but instead
>> issue a new migration, then bdrv_activate_all() will never be called
>> and the assert will be reached:
>>
>> bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
>> BDRV_O_INACTIVE)' failed.
>>
>> Fix the issue by checking at the start of migration if the VM is
>> paused and call bdrv_activate_all() before migrating. Even if the
>> late-block-activate capability is not in play or if the VM has been
>> paused manually, there is no harm calling that function again.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index aedf7f0751..26af30137b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>          return false;
>>      }
>>  
>> +    /*
>> +     * The VM might have been target of a previous migration. If it
>> +     * was in the paused state then nothing will have required the
>> +     * block layer to be activated. Do it now to ensure this QEMU
>> +     * instance owns the disk locks.
>> +     */
>> +    if (!resume && runstate_check(RUN_STATE_PAUSED)) {
> 
> I hope this will cover all the cases that QEMU could overlook.. or I'm not
> sure whether we could invoke bdrv_activate_all() unconditionally, as it
> looks like safe to be used if all disks are active already.
> 
> I wished we don't need to bother with disk activation status at all,
> because IIUC migration could work all fine even if all disks are inactivate
> when preparing migration.. hence such change always looks like a workaround
> of a separate issue.
> 
>> +        Error *local_err = NULL;
>> +
>> +        g_assert(bql_locked());
>> +
>> +        bdrv_activate_all(&local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            return false;
>> +        }
>> +        s->block_inactive = false;
> 
> This var so far was only used within one migration iteration, and the var
> was only set in migration_completion_precopy() so far.  Now we're resetting
> it upfront of a migration.  I'm not 100% sure if it's needed, or should be
> put somewhere else.
> 

This variable is unconditionally set in migration_completion_precopy()
and used as a flag for whether or not we should be deactivating disks in
qemu_savevm_state_complete_precopy():

>     /*                                                                              
>      * Inactivate disks except in COLO, and track that we have done so in order 
>      * to remember to reactivate them if migration fails or is cancelled.           
>      */                                                                             
>     s->block_inactive = !migrate_colo();                                            
>     migration_rate_set(RATE_LIMIT_DISABLED);                                                                                                                   
>     ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,             
>                                              s->block_inactive);

Ideally we'd like to take our paused state into account right here and
skip inactivation.  However at this point during the 2nd migration (in
paused state) our current_run_state == RUN_STATE_FINISH_MIGRATE.  So if
we truly wanted to skip unnecessary bdrv_activate_all(), we'd have to
remember our paused state somewhere earlier on the call stack and pass
an additional flag for that. Personally I don't think this is any
cleaner than just blatantly calling bdrv_activate_all().

> In general, I saw the mention of other places that may also try to
> invalidate disks that used to be invalidated.  If that's the case, I wish
> we don't need to touch migration code at all, but instead if block layer
> can cope with "invalidate on top of invalidated disks" it'll be perfect.
> 

This sounds similar to my initial very naive fix, which we decided not
to take:
https://lists.nongnu.org/archive/html/qemu-devel/2024-09/msg04254.html

>> +    }
>> +
>>      return true;
>>  }
>>  
>> -- 
>> 2.35.3
>>
> 

Re: [PATCH 4/5] migration: Activate block devices if VM is paused when migrating

[Peter Xu]

On Wed, Nov 27, 2024 at 07:40:39PM +0200, Andrey Drobyshev wrote:
> On 11/25/24 7:07 PM, Peter Xu wrote:
> > On Mon, Nov 25, 2024 at 11:46:11AM -0300, Fabiano Rosas wrote:
> >> Currently a VM that has been target of a migration using
> >> late-block-activate will crash at the end of a new migration (with it
> >> as source) when releasing ownership of the disks due to the VM having
> >> never taken ownership of the disks in the first place.
> >>
> >> The issue is that late-block-activate expects a qmp_continue command
> >> to be issued at some point on the destination VM after the migration
> >> finishes. If the user decides to never continue the VM, but instead
> >> issue a new migration, then bdrv_activate_all() will never be called
> >> and the assert will be reached:
> >>
> >> bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
> >> BDRV_O_INACTIVE)' failed.
> >>
> >> Fix the issue by checking at the start of migration if the VM is
> >> paused and call bdrv_activate_all() before migrating. Even if the
> >> late-block-activate capability is not in play or if the VM has been
> >> paused manually, there is no harm calling that function again.
> >>
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >>  migration/migration.c | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index aedf7f0751..26af30137b 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
> >>          return false;
> >>      }
> >>  
> >> +    /*
> >> +     * The VM might have been target of a previous migration. If it
> >> +     * was in the paused state then nothing will have required the
> >> +     * block layer to be activated. Do it now to ensure this QEMU
> >> +     * instance owns the disk locks.
> >> +     */
> >> +    if (!resume && runstate_check(RUN_STATE_PAUSED)) {
> > 
> > I hope this will cover all the cases that QEMU could overlook.. or I'm not
> > sure whether we could invoke bdrv_activate_all() unconditionally, as it
> > looks like safe to be used if all disks are active already.
> > 
> > I wished we don't need to bother with disk activation status at all,
> > because IIUC migration could work all fine even if all disks are inactivate
> > when preparing migration.. hence such change always looks like a workaround
> > of a separate issue.
> > 
> >> +        Error *local_err = NULL;
> >> +
> >> +        g_assert(bql_locked());
> >> +
> >> +        bdrv_activate_all(&local_err);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            return false;
> >> +        }
> >> +        s->block_inactive = false;
> > 
> > This var so far was only used within one migration iteration, and the var
> > was only set in migration_completion_precopy() so far.  Now we're resetting
> > it upfront of a migration.  I'm not 100% sure if it's needed, or should be
> > put somewhere else.
> > 
> 
> This variable is unconditionally set in migration_completion_precopy()
> and used as a flag for whether or not we should be deactivating disks in
> qemu_savevm_state_complete_precopy():
> 
> >     /*                                                                              
> >      * Inactivate disks except in COLO, and track that we have done so in order 
> >      * to remember to reactivate them if migration fails or is cancelled.           
> >      */                                                                             
> >     s->block_inactive = !migrate_colo();                                            
> >     migration_rate_set(RATE_LIMIT_DISABLED);                                                                                                                   
> >     ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,             
> >                                              s->block_inactive);
> 
> Ideally we'd like to take our paused state into account right here and
> skip inactivation.  However at this point during the 2nd migration (in
> paused state) our current_run_state == RUN_STATE_FINISH_MIGRATE.  So if
> we truly wanted to skip unnecessary bdrv_activate_all(), we'd have to
> remember our paused state somewhere earlier on the call stack and pass
> an additional flag for that. Personally I don't think this is any
> cleaner than just blatantly calling bdrv_activate_all().

I did something like that.

Would you please have a look at the other series I posted?  Sorry it's
slightly longer than this patch, but it does cover more than this bug
alone (and it should also fix this bug):

https://lore.kernel.org/r/20241204005138.702289-1-peterx@redhat.com

> 
> > In general, I saw the mention of other places that may also try to
> > invalidate disks that used to be invalidated.  If that's the case, I wish
> > we don't need to touch migration code at all, but instead if block layer
> > can cope with "invalidate on top of invalidated disks" it'll be perfect.
> > 
> 
> This sounds similar to my initial very naive fix, which we decided not
> to take:
> https://lists.nongnu.org/archive/html/qemu-devel/2024-09/msg04254.html

I still wish this can work, either the original patch, or anything that can
make bdrv_inactivate_all() work on inactive-state disks, then it'll be an
easier to use block API.

I don't know block layer well, but I more or less know migration.  I hope
we can still resolve that with some other way at least in migration so that
we don't start to request disk being active for migration, because it
really shouldn't need to.

Thanks,

-- 
Peter Xu

Re: [PATCH 4/5] migration: Activate block devices if VM is paused when migrating

[Andrey Drobyshev]

On 11/25/24 4:46 PM, Fabiano Rosas wrote:
> Currently a VM that has been target of a migration using
> late-block-activate will crash at the end of a new migration (with it
> as source) when releasing ownership of the disks due to the VM having
> never taken ownership of the disks in the first place.
> 
> The issue is that late-block-activate expects a qmp_continue command
> to be issued at some point on the destination VM after the migration
> finishes. If the user decides to never continue the VM, but instead
> issue a new migration, then bdrv_activate_all() will never be called
> and the assert will be reached:
> 
> bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
> BDRV_O_INACTIVE)' failed.
> 
> Fix the issue by checking at the start of migration if the VM is
> paused and call bdrv_activate_all() before migrating. Even if the
> late-block-activate capability is not in play or if the VM has been
> paused manually, there is no harm calling that function again.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index aedf7f0751..26af30137b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          return false;
>      }
>  
> +    /*
> +     * The VM might have been target of a previous migration. If it
> +     * was in the paused state then nothing will have required the
> +     * block layer to be activated. Do it now to ensure this QEMU
> +     * instance owns the disk locks.
> +     */
> +    if (!resume && runstate_check(RUN_STATE_PAUSED)) {
> +        Error *local_err = NULL;
> +
> +        g_assert(bql_locked());
> +
> +        bdrv_activate_all(&local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return false;
> +        }
> +        s->block_inactive = false;
> +    }
> +
>      return true;
>  }
> 

Hi Fabiano,

Thans for the fix, I can confirm that on my setup (2 nodes with common
NFS share and VM's disk on the share) this patch does solve the issue.

Tested-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

[PATCH 5/5] tests/qtest/migration: Test successive migrations

[Fabiano Rosas]

Add a framework for running migrations back and forth between two
guests (aka ping-pong migration). We have seen a couple of bugs that
only reproduce when a guest is migrated and the destination machine is
used as the source of a new migration.

Add a simple test that does 2 migrations and another test that uses
the late-block-activate capability, which is one area where a bug has
been found.

Note that this does not reuse any of the existing tests
(e.g. test_precopy_common), because there is too much ambiguity
regarding how to handle the hooks, args->result, stop/continue, etc.

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

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f27dd93835..ee1422c8e8 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1817,6 +1817,104 @@ finish:
     test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED);
 }
 
+static void migrate_dst_becomes_src(QTestState **from, QTestState **to,
+                                    QTestMigrationState *src_state,
+                                    QTestMigrationState *dst_state)
+{
+    *from = *to;
+
+    *src_state = (QTestMigrationState) { };
+    src_state->serial = dst_state->serial;
+    qtest_qmp_set_event_callback(*from, migrate_watch_for_events, src_state);
+
+    src_state->stop_seen = dst_state->stop_seen;
+}
+
+static void test_precopy_ping_pong_common(MigrateCommon *args, int n,
+                                          bool late_block_activate)
+{
+    QTestState *from, *to;
+    bool keep_paused = false;
+
+    g_assert(!args->live);
+
+    g_test_message("Migrating back and forth %d times", n);
+
+    for (int i = 0; i < n; i++) {
+        g_test_message("%s (%d/%d)", i % 2 ? "pong" : "ping", i + 1, n);
+
+        if (test_migrate_start(&from, &to, args->listen_uri, &args->start)) {
+            return;
+        }
+
+        if (late_block_activate) {
+            migrate_set_capability(from, "late-block-activate", true);
+            migrate_set_capability(to, "late-block-activate", true);
+
+            /*
+             * The late-block-activate capability expects that the
+             * management layer will issue a qmp_continue command on
+             * the destination once the migration finishes. In order
+             * to test the capability properly, make sure the test is
+             * not issuing spurious continue commands.
+             */
+            keep_paused = true;
+        }
+
+        if (!src_state.stop_seen) {
+            wait_for_serial(src_state.serial);
+        }
+
+        /* because of !args->live */
+        qtest_qmp_assert_success(from, "{ 'execute' : 'stop'}");
+        wait_for_stop(from, &src_state);
+
+        migrate_ensure_converge(from);
+        migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
+
+        wait_for_migration_complete(from);
+        wait_for_migration_complete(to);
+
+        /* note check_guests_ram() requires a stopped guest */
+        check_guests_ram(to);
+
+        if (keep_paused) {
+            /*
+             * Nothing issued continue on the destination, reflect
+             * that the guest is paused in the dst_state.
+             */
+            dst_state.stop_seen = true;
+        } else {
+            qtest_qmp_assert_success(to, "{ 'execute' : 'cont'}");
+            wait_for_serial(dst_state.serial);
+            dst_state.stop_seen = false;
+        }
+
+        /*
+         * Last iteration, let the guest run to make sure there's no
+         * memory corruption. The test_migrate_end() below will check
+         * the memory one last time.
+         */
+        if (i == n - 1) {
+            qtest_qmp_assert_success(to, "{ 'execute' : 'cont'}");
+            wait_for_serial(dst_state.serial);
+            dst_state.stop_seen = false;
+            break;
+        }
+
+        /*
+         * Prepare for migrating back: clear the original source and
+         * switch source & destination.
+         */
+        migrate_cleanup(from, NULL);
+        migrate_dst_becomes_src(&from, &to, &src_state, &dst_state);
+
+        args->start.only_target = true;
+    }
+
+    test_migrate_end(from, to, true);
+}
+
 static void file_dirty_offset_region(void)
 {
     g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
@@ -3765,6 +3863,24 @@ static void test_migrate_dirty_limit(void)
     test_migrate_end(from, to, true);
 }
 
+static void test_precopy_ping_pong(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "tcp:127.0.0.1:0",
+    };
+
+    test_precopy_ping_pong_common(&args, 2, false);
+}
+
+static void test_precopy_ping_pong_late_block(void)
+{
+    MigrateCommon args = {
+        .listen_uri = "tcp:127.0.0.1:0",
+    };
+
+    test_precopy_ping_pong_common(&args, 2, true);
+}
+
 static bool kvm_dirty_ring_supported(void)
 {
 #if defined(__linux__) && defined(HOST_X86_64)
@@ -4048,6 +4164,11 @@ int main(int argc, char **argv)
         }
     }
 
+    migration_test_add("/migration/precopy/ping-pong/plain",
+                       test_precopy_ping_pong);
+    migration_test_add("/migration/precopy/ping-pong/late-block-activate",
+                       test_precopy_ping_pong_late_block);
+
     ret = g_test_run();
 
     g_assert_cmpint(ret, ==, 0);
-- 
2.35.3