1. Patchew
  2. QEMU
  3. View series

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

Dr. David Alan Gilbert (git) posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170804175011.21944-1-dgilbert@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
vl.c | 2 ++
1 file changed, 2 insertions(+)
[Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions
Posted by Dr. David Alan Gilbert (git) 6 years, 8 months ago 
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
Posted by Peter Xu 6 years, 8 months ago 
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
Posted by Dr. David Alan Gilbert 6 years, 8 months ago 
* 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
Posted by Peter Xu 6 years, 8 months ago 
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
Posted by Juan Quintela 6 years, 8 months ago 
"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
Posted by Peter Xu 6 years, 8 months ago 
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
Posted by Dr. David Alan Gilbert 6 years, 7 months ago 
* 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
Posted by Paolo Bonzini 6 years, 7 months ago 
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
>>
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.