On 3/15/24 14:09, Peter Xu wrote:
On Fri, Mar 15, 2024 at 01:20:49PM +0100, Cédric Le Goater wrote:
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.
Yes it looks cleaner indeed, it's just that then we'll have one more
possible state conversions like SETUP->UNPLUG->SETUP. I'd say it's fine,
but let's also copy Laruent and Laine if it's going to be posted formally.
OK. I just sent the alternative implementation. The code looks a little
ugly :
bql_lock(); |
qemu_savevm_state_header(s->to_dst_file); |
ret = qemu_savevm_state_setup(s->to_dst_file, &local_err); | in
SETUP state
bql_unlock(); |
|
qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, | SETUP
-> ACTIVE transition
MIGRATION_STATUS_ACTIVE); |
|
/* |
* Handle SETUP failures after waiting for virtio-net-failover |
* devices to unplug. This to preserve migration state transitions. |
*/ |
if (ret) { |
migrate_set_error(s, local_err); |
handling SETUP errors in ACTIVE
error_free(local_err); |
migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, |
MIGRATION_STATUS_FAILED); |
goto fail_setup; |
} |
|
s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; |
SETUP duration
|
trace_migration_thread_setup_complete(); |
SETUP trace event
Thanks,
C.
