[Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Dr. David Alan Gilbert (git)]

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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Peter Xu]

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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Peter Xu]

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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Juan Quintela]

"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.

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Peter Xu]

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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

[Paolo Bonzini]

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
>>
>>