From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
There's a race if someone does a 'stop' near the end of migrate;
the migration process goes through two runstates:
'finish migrate'
'postmigrate'
If the user issues a 'stop' between the two we end up with invalid
state transitions.
Add the transitions as valid.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
vl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/vl.c b/vl.c
index 99fcfa0442..bacb03f49d 100644
--- a/vl.c
+++ b/vl.c
@@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_PAUSED, RUN_STATE_RUNNING },
{ RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
+ { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
{ RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
{ RUN_STATE_PAUSED, RUN_STATE_COLO},
@@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
{ RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
+ { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
--
2.13.3
On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > There's a race if someone does a 'stop' near the end of migrate; > the migration process goes through two runstates: > 'finish migrate' > 'postmigrate' > > If the user issues a 'stop' between the two we end up with invalid > state transitions. > Add the transitions as valid. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > vl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vl.c b/vl.c > index 99fcfa0442..bacb03f49d 100644 > --- a/vl.c > +++ b/vl.c > @@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, > { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, > + { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, > { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, > { RUN_STATE_PAUSED, RUN_STATE_COLO}, > > @@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = { > { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE }, > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING }, > + { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO}, Do we need this as well? { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED }, -- Peter Xu
* Peter Xu (peterx@redhat.com) wrote: > On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > There's a race if someone does a 'stop' near the end of migrate; > > the migration process goes through two runstates: > > 'finish migrate' > > 'postmigrate' > > > > If the user issues a 'stop' between the two we end up with invalid > > state transitions. > > Add the transitions as valid. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > vl.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/vl.c b/vl.c > > index 99fcfa0442..bacb03f49d 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > > > { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, > > { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, > > + { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, > > { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, > > { RUN_STATE_PAUSED, RUN_STATE_COLO}, > > > > @@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE }, > > > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING }, > > + { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED }, > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE }, > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH }, > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO}, > > Do we need this as well? > > { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED }, Apparently not: (qemu) migrate "exec:cat > /dev/null" (qemu) infomigrate unknown command: 'infomigrate' (qemu) info status VM status: paused (postmigrate) (qemu) stop (qemu) info status VM status: paused (postmigrate) (qemu) so doing a stop at that point doesn't seem to cause any problem. Dave > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Mon, Aug 07, 2017 at 01:25:29PM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > There's a race if someone does a 'stop' near the end of migrate; > > > the migration process goes through two runstates: > > > 'finish migrate' > > > 'postmigrate' > > > > > > If the user issues a 'stop' between the two we end up with invalid > > > state transitions. > > > Add the transitions as valid. > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > vl.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/vl.c b/vl.c > > > index 99fcfa0442..bacb03f49d 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > > > > > { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, > > > { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, > > > + { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, > > > { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, > > > { RUN_STATE_PAUSED, RUN_STATE_COLO}, > > > > > > @@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > > { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE }, > > > > > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING }, > > > + { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED }, > > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE }, > > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH }, > > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO}, > > > > Do we need this as well? > > > > { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED }, > > Apparently not: > > (qemu) migrate "exec:cat > /dev/null" > (qemu) infomigrate > unknown command: 'infomigrate' > (qemu) info status > VM status: paused (postmigrate) > (qemu) stop > (qemu) info status > VM status: paused (postmigrate) > (qemu) > > > so doing a stop at that point doesn't seem to cause any problem. Yes. Thanks. Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > There's a race if someone does a 'stop' near the end of migrate; > the migration process goes through two runstates: > 'finish migrate' > 'postmigrate' > > If the user issues a 'stop' between the two we end up with invalid > state transitions. > Add the transitions as valid. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> To answer Peter question: int vm_stop(RunState state) { .... we don't care about this case .... return do_vm_stop(state); } static int do_vm_stop(RunState state) { int ret = 0; if (runstate_is_running()) { cpu_disable_ticks(); pause_all_vcpus(); runstate_set(state); vm_state_notify(0, state); qapi_event_send_stop(&error_abort); } bdrv_drain_all(); replay_disable_events(); ret = bdrv_flush_all(); return ret; } int runstate_is_running(void) { return runstate_check(RUN_STATE_RUNNING); } So, "stop" only changes states when we are in RUNNING state. The idea was that after migration, the only valid command (as in the film "it should do something")is "run". Later, Juan.
On Tue, Aug 08, 2017 at 09:02:54AM +0200, Juan Quintela wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > There's a race if someone does a 'stop' near the end of migrate; > > the migration process goes through two runstates: > > 'finish migrate' > > 'postmigrate' > > > > If the user issues a 'stop' between the two we end up with invalid > > state transitions. > > Add the transitions as valid. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > To answer Peter question: > > int vm_stop(RunState state) > { > .... we don't care about this case .... > > return do_vm_stop(state); > } > > static int do_vm_stop(RunState state) > { > int ret = 0; > > if (runstate_is_running()) { > cpu_disable_ticks(); > pause_all_vcpus(); > runstate_set(state); > vm_state_notify(0, state); > qapi_event_send_stop(&error_abort); > } > > bdrv_drain_all(); > replay_disable_events(); > ret = bdrv_flush_all(); > > return ret; > } > > > int runstate_is_running(void) > { > return runstate_check(RUN_STATE_RUNNING); > } > > > So, "stop" only changes states when we are in RUNNING state. > The idea was that after migration, the only valid command (as in the > film "it should do something")is "run". Thanks for the details. :) -- Peter Xu
* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > There's a race if someone does a 'stop' near the end of migrate; > the migration process goes through two runstates: > 'finish migrate' > 'postmigrate' > > If the user issues a 'stop' between the two we end up with invalid > state transitions. > Add the transitions as valid. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Queued > --- > vl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vl.c b/vl.c > index 99fcfa0442..bacb03f49d 100644 > --- a/vl.c > +++ b/vl.c > @@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = { > > { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, > { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, > + { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, > { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, > { RUN_STATE_PAUSED, RUN_STATE_COLO}, > > @@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = { > { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE }, > > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING }, > + { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH }, > { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO}, > -- > 2.13.3 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/09/2017 15:40, Dr. David Alan Gilbert wrote: >> There's a race if someone does a 'stop' near the end of migrate; >> the migration process goes through two runstates: >> 'finish migrate' >> 'postmigrate' >> >> If the user issues a 'stop' between the two we end up with invalid >> state transitions. >> Add the transitions as valid. >> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Queued > >> --- >> vl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index 99fcfa0442..bacb03f49d 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] = { >> >> { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, >> { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, >> + { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE }, There is: if (s->state == MIGRATION_STATUS_COMPLETED) { ... runstate_set(RUN_STATE_POSTMIGRATE); } else { ... if (old_vm_running && !entered_postcopy) { vm_start(); } else { if (runstate_check(RUN_STATE_FINISH_MIGRATE)) { runstate_set(RUN_STATE_POSTMIGRATE); } } } Maybe the runstate_check should be in the "then" branch too, instead of adding this transition? Paolo >> { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH }, >> { RUN_STATE_PAUSED, RUN_STATE_COLO}, >> >> @@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = { >> { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE }, >> >> { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING }, >> + { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED }, >> { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE }, >> { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH }, >> { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO}, >> -- >> 2.13.3 >> >>
© 2016 - 2024 Red Hat, Inc.