On Fri, Mar 08, 2024 at 02:55:30PM +0100, Cédric Le Goater wrote:
On 3/8/24 14:39, Cédric Le Goater wrote:
On 3/8/24 14:14, Cédric Le Goater wrote:
On 3/8/24 13:56, Peter Xu wrote:
On Wed, Mar 06, 2024 at 02:34:25PM +0100, Cédric Le Goater wrote:
This prepares ground for the changes coming next which add an Error**
argument to the .save_setup() handler. Callers of qemu_savevm_state_setup()
now handle the error and fail earlier setting the migration state from
MIGRATION_STATUS_SETUP to MIGRATION_STATUS_FAILED.
In qemu_savevm_state(), move the cleanup to preserve the error
reported by .save_setup() handlers.
Since the previous behavior was to ignore errors at this step of
migration, this change should be examined closely to check that
cleanups are still correctly done.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
Changes in v4:
- Merged cleanup change in qemu_savevm_state()
Changes in v3:
- Set migration state to MIGRATION_STATUS_FAILED
- Fixed error handling to be done under lock in bg_migration_thread()
- Made sure an error is always set in case of failure in
qemu_savevm_state_setup()
migration/savevm.h | 2 +-
migration/migration.c | 27 ++++++++++++++++++++++++---
migration/savevm.c | 26 +++++++++++++++-----------
3 files changed, 40 insertions(+), 15 deletions(-)
diff --git a/migration/savevm.h b/migration/savevm.h
index
74669733dd63a080b765866c703234a5c4939223..9ec96a995c93a42aad621595f0ed58596c532328
100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -32,7 +32,7 @@
bool qemu_savevm_state_blocked(Error **errp);
void qemu_savevm_non_migratable_list(strList **reasons);
int qemu_savevm_state_prepare(Error **errp);
-void qemu_savevm_state_setup(QEMUFile *f);
+int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
bool qemu_savevm_state_guest_unplug_pending(void);
int qemu_savevm_state_resume_prepare(MigrationState *s);
void qemu_savevm_state_header(QEMUFile *f);
diff --git a/migration/migration.c b/migration/migration.c
index
a49fcd53ee19df1ce0182bc99d7e064968f0317b..6d1544224e96f5edfe56939a9c8395d88ef29581
100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3408,6 +3408,8 @@ static void *migration_thread(void *opaque)
int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
MigThrError thr_error;
bool urgent = false;
+ Error *local_err = NULL;
+ int ret;
thread = migration_threads_add("live_migration", qemu_get_thread_id());
@@ -3451,9 +3453,17 @@ static void *migration_thread(void *opaque)
}
bql_lock();
- qemu_savevm_state_setup(s->to_dst_file);
+ ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
bql_unlock();
+ if (ret) {
+ migrate_set_error(s, local_err);
+ error_free(local_err);
+ migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+ MIGRATION_STATUS_FAILED);
+ goto out;
+ }
There's a small indent issue, I can fix it.
checkpatch did report anything.
The bigger problem is I _think_ this will trigger a ci failure in the
virtio-net-failover test:
▶ 121/464 ERROR:../tests/qtest/virtio-net-failover.c:1203:test_migrate_abort_wait_unplug: assertion failed
(status == "cancelling"): ("cancelled" == "cancelling") ERROR
121/464 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover ERROR
4.77s killed by signal 6 SIGABRT
PYTHON=/builds/peterx/qemu/build/pyvenv/bin/python3.8
G_TEST_DBUS_DAEMON=/builds/peterx/qemu/tests/dbus-vmstate-daemon.sh
MALLOC_PERTURB_=161 QTEST_QEMU_IMG=./qemu-img
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
QTEST_QEMU_BINARY=./qemu-system-x86_64
/builds/peterx/qemu/build/tests/qtest/virtio-net-failover --tap -k
――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-x86_64: ram_save_setup failed: Input/output error
**
ERROR:../tests/qtest/virtio-net-failover.c:1203:test_migrate_abort_wait_unplug: assertion failed (status ==
"cancelling"): ("cancelled" == "cancelling")
(test program exited with status code -6)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
I am not familiar enough with the failover code, and may not have time
today to follow this up, copy Laurent. Cedric, if you have time, please
have a look.
Sure. Weird because I usually run make check on x86_64, s390x, ppc64 and
aarch64. Let me check again.
I see one timeout error on s390x but not always. See below. It occurs with
or without this patchset. the other x86_64, ppc64 arches run fine (a part
from one io test failing from time to time)
Ah ! I got this once on aarch64 :
161/486
ERROR:../tests/qtest/virtio-net-failover.c:1222:test_migrate_abort_wait_unplug:
'device' should not be NULL ERROR
161/486 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover
ERROR 5.98s killed by signal 6 SIGABRT
G_TEST_DBUS_DAEMON=/home/legoater/work/qemu/qemu.git/tests/dbus-vmstate-daemon.sh
MALLOC_PERTURB_=119 QTEST_QEMU_BINARY=./qemu-system-x86_64
QTEST_QEMU_IMG=./qemu-img
PYTHON=/home/legoater/work/qemu/qemu.git/build/pyvenv/bin/python3
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
/home/legoater/work/qemu/qemu.git/build/tests/qtest/virtio-net-failover --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-x86_64: ram_save_setup failed: Input/output error
**
ERROR:../tests/qtest/virtio-net-failover.c:1222:test_migrate_abort_wait_unplug:
'device' should not be NULL
(test program exited with status code -6)
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Hmm, this one seems different..
I couldn't reproduce yet :/
I never reproduced it locally on x86, and my failure is always at checking
"cancelling" v.s. "cancelled" rather than the NULL check. It's much easier
to trigger on CI in check-system-centos (I don't know why centos..):
https://gitlab.com/peterx/qemu/-/jobs/6351020546
I think at least for the error I hit, the problem is the failover test will
cancel the migration, but if it cancels too fast and during setup now it
can already fail it (while it won't fail before when we ignore
qemu_savevm_state_setup() errors), and I think it'll skip:
qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
MIGRATION_STATUS_ACTIVE);
It seems the test wants the "cancelling" to hold until later:
/* while the card is not ejected, we must be in "cancelling" state */
ret = migrate_status(qts);
status = qdict_get_str(ret, "status");
g_assert_cmpstr(status, ==, "cancelling");
qobject_unref(ret);
/* OS unplugs the cards, QEMU can move from wait-unplug state */
qtest_outl(qts, ACPI_PCIHP_ADDR_ICH9 + PCI_EJ_BASE, 1);
Again, since I'll need to read the failover code, not much I can tell.
Laurent might have a clue.