* Jens Freimann (jfreim...@redhat.com) wrote: > On Fri, Oct 11, 2019 at 06:11:33PM +0100, Dr. David Alan Gilbert wrote: > > * Jens Freimann (jfreim...@redhat.com) wrote: > > > This patch adds a new migration state called wait-unplug. It is entered > > > after the SETUP state and will transition into ACTIVE once all devices > > > were succesfully unplugged from the guest. > > > > > > So if a guest doesn't respond or takes long to honor the unplug request > > > the user will see the migration state 'wait-unplug'. > > > > > > In the migration thread we query failover devices if they're are still > > > pending the guest unplug. When all are unplugged the migration > > > continues. We give it a defined number of iterations including small > > > waiting periods before we proceed. > > > > > > Signed-off-by: Jens Freimann <jfreim...@redhat.com> > [..] > > > @@ -3260,6 +3271,27 @@ static void *migration_thread(void *opaque) > > > > > > qemu_savevm_state_setup(s->to_dst_file); > > > > > > + migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > > > + MIGRATION_STATUS_WAIT_UNPLUG); > > > > I think I'd prefer if you only went into this state if you had any > > devices that were going to need unplugging. > > Sure, that makes sense. I'll change it. > > > > + while (i < FAILOVER_UNPLUG_RETRIES && > > > + s->state == MIGRATION_STATUS_WAIT_UNPLUG) { > > > + i++; > > > + qemu_sem_timedwait(&s->wait_unplug_sem, > > > FAILOVER_GUEST_UNPLUG_WAIT); > > > + all_unplugged = qemu_savevm_state_guest_unplug_pending(); > > > + if (all_unplugged) { > > > + break; > > > + } > > > + } > > > + > > > + if (all_unplugged) { > > > + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, > > > + MIGRATION_STATUS_ACTIVE); > > > + } else { > > > + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, > > > + MIGRATION_STATUS_CANCELLING); > > > + } > > > > I think you can get rid of both the timeout and the count and just make > > sure that migrate_cancel works at this point. > > I see, I need to add the new state to migration_is_setup_or_active() or > a cancel won't work.
You probably need to do that anyway given all the other places is_setup_or_active is called. > > This pushes the problem up a layer, which I think is fine. > > Seems good to me. To be clear, you're saying I should just poll on > the device unplugged state? Like > > while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && > !qemu_savevm_state_guest_unplug_pending()) { > _ /* This block intentionally left blank */ > } I'd keep the qemu_sem_timedwait in there, but with a short time out (e.g. 250ms say); that way it doesn't eat cpu, but also the cancel still happens quickly. Dave > > regards, > Jens -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK