| Message ID | 20210629155007.629086-2-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: failover: continue to wait card unplug on error | expand |
* Laurent Vivier (lvivier@redhat.com) wrote: > The loop is used in migration_thread() and bg_migration_thread(), > so we can move it to its own function and call it from these both places. > > Moreover, in migration_thread() we have a wrong state transition from > SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly > managed in bg_migration_thread() so use this code instead. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/migration.c | 54 +++++++++++++++++++++---------------------- > 1 file changed, 26 insertions(+), 28 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 4228635d1880..3e92c405a2b6 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -3664,6 +3664,28 @@ bool migration_rate_limit(void) > return urgent; > } > > +/* > + * if failover devices are present, wait they are completely > + * unplugged > + */ > + > +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, > + int new_state) > +{ > + if (qemu_savevm_state_guest_unplug_pending()) { > + migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG); > + > + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > + qemu_savevm_state_guest_unplug_pending()) { > + qemu_sem_timedwait(&s->wait_unplug_sem, 250); > + } > + > + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state); > + } else { > + migrate_set_state(&s->state, old_state, new_state); > + } > +} > + > /* > * Master migration thread on the source VM. > * It drives the migration and pumps the data down the outgoing channel. > @@ -3710,22 +3732,10 @@ static void *migration_thread(void *opaque) > > qemu_savevm_state_setup(s->to_dst_file); > > - if (qemu_savevm_state_guest_unplug_pending()) { > - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > - MIGRATION_STATUS_WAIT_UNPLUG); > - > - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > - qemu_savevm_state_guest_unplug_pending()) { > - qemu_sem_timedwait(&s->wait_unplug_sem, 250); > - } > - > - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, > - MIGRATION_STATUS_ACTIVE); > - } > + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, > + MIGRATION_STATUS_ACTIVE); > > s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; > - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > - MIGRATION_STATUS_ACTIVE); > > trace_migration_thread_setup_complete(); > > @@ -3833,21 +3843,9 @@ static void *bg_migration_thread(void *opaque) > qemu_savevm_state_header(s->to_dst_file); > qemu_savevm_state_setup(s->to_dst_file); > > - if (qemu_savevm_state_guest_unplug_pending()) { > - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > - MIGRATION_STATUS_WAIT_UNPLUG); > - > - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > - qemu_savevm_state_guest_unplug_pending()) { > - qemu_sem_timedwait(&s->wait_unplug_sem, 250); > - } > + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, > + MIGRATION_STATUS_ACTIVE); > > - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, > - MIGRATION_STATUS_ACTIVE); > - } else { > - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > - MIGRATION_STATUS_ACTIVE); > - } > s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; > > trace_migration_thread_setup_complete(); > -- > 2.31.1 >
Laurent Vivier <lvivier@redhat.com> wrote: > The loop is used in migration_thread() and bg_migration_thread(), > so we can move it to its own function and call it from these both places. > > Moreover, in migration_thread() we have a wrong state transition from > SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly > managed in bg_migration_thread() so use this code instead. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> If you have to repost: > +/* > + * if failover devices are present, wait they are completely > + * unplugged > + */ > + > +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, > + int new_state) old_state and new state are always the same. SETUP -> ACTIVE. I think we can hardcode them. > +{ > + if (qemu_savevm_state_guest_unplug_pending()) { > + migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG); > + > + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > + qemu_savevm_state_guest_unplug_pending()) { > + qemu_sem_timedwait(&s->wait_unplug_sem, 250); I still don't understand why are we using a semaphore when we just want a timer :-( Yes, this is independent of this patch.
* Juan Quintela (quintela@redhat.com) wrote: > Laurent Vivier <lvivier@redhat.com> wrote: > > The loop is used in migration_thread() and bg_migration_thread(), > > so we can move it to its own function and call it from these both places. > > > > Moreover, in migration_thread() we have a wrong state transition from > > SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly > > managed in bg_migration_thread() so use this code instead. > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > If you have to repost: > > > > +/* > > + * if failover devices are present, wait they are completely > > + * unplugged > > + */ > > + > > +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, > > + int new_state) > > old_state and new state are always the same. SETUP -> ACTIVE. I think > we can hardcode them. > > > > +{ > > + if (qemu_savevm_state_guest_unplug_pending()) { > > + migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG); > > + > > + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > > + qemu_savevm_state_guest_unplug_pending()) { > > + qemu_sem_timedwait(&s->wait_unplug_sem, 250); > > I still don't understand why are we using a semaphore when we just want > a timer :-( > > Yes, this is independent of this patch. So yes I was going to ask on the 2nd patch; no one anywhere seems to set that semaphore? Dave
diff --git a/migration/migration.c b/migration/migration.c index 4228635d1880..3e92c405a2b6 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3664,6 +3664,28 @@ bool migration_rate_limit(void) return urgent; } +/* + * if failover devices are present, wait they are completely + * unplugged + */ + +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, + int new_state) +{ + if (qemu_savevm_state_guest_unplug_pending()) { + migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG); + + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && + qemu_savevm_state_guest_unplug_pending()) { + qemu_sem_timedwait(&s->wait_unplug_sem, 250); + } + + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state); + } else { + migrate_set_state(&s->state, old_state, new_state); + } +} + /* * Master migration thread on the source VM. * It drives the migration and pumps the data down the outgoing channel. @@ -3710,22 +3732,10 @@ static void *migration_thread(void *opaque) qemu_savevm_state_setup(s->to_dst_file); - if (qemu_savevm_state_guest_unplug_pending()) { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_WAIT_UNPLUG); - - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && - qemu_savevm_state_guest_unplug_pending()) { - qemu_sem_timedwait(&s->wait_unplug_sem, 250); - } - - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, - MIGRATION_STATUS_ACTIVE); - } + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, + MIGRATION_STATUS_ACTIVE); s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_ACTIVE); trace_migration_thread_setup_complete(); @@ -3833,21 +3843,9 @@ static void *bg_migration_thread(void *opaque) qemu_savevm_state_header(s->to_dst_file); qemu_savevm_state_setup(s->to_dst_file); - if (qemu_savevm_state_guest_unplug_pending()) { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_WAIT_UNPLUG); - - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && - qemu_savevm_state_guest_unplug_pending()) { - qemu_sem_timedwait(&s->wait_unplug_sem, 250); - } + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, + MIGRATION_STATUS_ACTIVE); - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, - MIGRATION_STATUS_ACTIVE); - } else { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_ACTIVE); - } s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; trace_migration_thread_setup_complete();
The loop is used in migration_thread() and bg_migration_thread(), so we can move it to its own function and call it from these both places. Moreover, in migration_thread() we have a wrong state transition from SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly managed in bg_migration_thread() so use this code instead. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- migration/migration.c | 54 +++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 28 deletions(-)