On 3/15/24 12:01, Peter Xu wrote:
On Fri, Mar 15, 2024 at 11:17:45AM +0100, Cédric Le Goater wrote:
migrate_set_state is also unintuitive because it ignores invalid state
transitions and we've been using that property to deal with special
states such as POSTCOPY_PAUSED and FAILED:
- After the migration goes into POSTCOPY_PAUSED, the resumed migration's
migrate_init() will try to set the state NONE->SETUP, which is not
valid.
- After save_setup fails, the migration goes into FAILED, but wait_unplug
will try to transition SETUP->ACTIVE, which is also not valid.
I am not sure I understand what the plan is. Both solutions are problematic
regarding the state transitions.
Should we consider that waiting for failover devices to unplug is an internal
step of the SETUP phase not transitioning to ACTIVE ?
If to unblock this series, IIUC the simplest solution is to do what Fabiano
suggested, that we move qemu_savevm_wait_unplug() to be before the check of
setup() ret.
The simplest is IMHO moving qemu_savevm_wait_unplug() before
qemu_savevm_state_setup() and leave patch 10 is unchanged. See
below the extra patch. It looks much cleaner than what we have
today.
In that case, the state change in qemu_savevm_wait_unplug()
should be benign and we should see a super small window it became ACTIVE
but then it should be FAILED (and IIUC the patch itself will need to use
ACTIVE as "old_state", not SETUP anymore).
OK. I will give it a try to compare.
For the long term, maybe we can remove the WAIT_UNPLUG state?
I hope so, it's an internal SETUP state for me.
The only Libvirt support seems to be here:
commit 8a226ddb3602586a2ba2359afc4448c02f566a0e
Author: Laine Stump <la...@redhat.com>
Date: Wed Jan 15 16:38:57 2020 -0500
qemu: add wait-unplug to qemu migration status enum
Considering that qemu_savevm_wait_unplug() can be a noop if the device is
already unplugged, I think it means no upper layer app should rely on this
state to present.
Thanks,
C.
@@ -3383,11 +3383,10 @@ bool migration_rate_limit(void)
* unplugged
*/
-static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
- int new_state)
+static void qemu_savevm_wait_unplug(MigrationState *s, int state)
{
if (qemu_savevm_state_guest_unplug_pending()) {
- migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
+ migrate_set_state(&s->state, state, MIGRATION_STATUS_WAIT_UNPLUG);
while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
qemu_savevm_state_guest_unplug_pending()) {
@@ -3410,9 +3409,7 @@ static void qemu_savevm_wait_unplug(Migr
}
}
- migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state);
- } else {
- migrate_set_state(&s->state, old_state, new_state);
+ migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, state);
}
}
@@ -3469,17 +3466,19 @@ static void *migration_thread(void *opaq
qemu_savevm_send_colo_enable(s->to_dst_file);
}
+ qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP);
+
bql_lock();
qemu_savevm_state_setup(s->to_dst_file);
bql_unlock();
- qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
- MIGRATION_STATUS_ACTIVE);
-
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
trace_migration_thread_setup_complete();
+ migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+ MIGRATION_STATUS_ACTIVE);
+
while (migration_is_active()) {
if (urgent || !migration_rate_exceeded(s->to_dst_file)) {
MigIterateState iter_state = migration_iteration_run(s);
@@ -3580,18 +3579,20 @@ static void *bg_migration_thread(void *o
ram_write_tracking_prepare();
#endif
+ qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP);
+
bql_lock();
qemu_savevm_state_header(s->to_dst_file);
qemu_savevm_state_setup(s->to_dst_file);
bql_unlock();
- qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
- MIGRATION_STATUS_ACTIVE);
-
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
trace_migration_thread_setup_complete();
+ migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+ MIGRATION_STATUS_ACTIVE);
+
bql_lock();
if (migration_stop_vm(s, RUN_STATE_PAUSED)) {
