Re: [Qemu-devel] [PATCH] runstate: do not discard runstate changes when paused

[Luiz Capitulino]

On Tue,  4 Oct 2011 14:04:45 +0200
Paolo Bonzini <address@hidden> wrote:

> Trying to migrate a paused machine fails.  The reason is that
> the RSTATE_PRE_MIGRATE is reached with vm_stop, and this
> transition is eaten when the vm is already paused.  This patch
> fixes the problem by always going through runstate_set and
> always notifying the new state.

Let's start over, this time CC'ing Jan, Anthony and Avi.

Basically, what Paolo is describing above is this:

 1. The user issues the stop command. vm_stop() will set the state to
    RSTATE_PAUSED

 2. The user starts a migration. migrate_fd_put_ready() will call
    vm_stop(RSTATE_PRE_MIGRATE). However, the VM is already stopped
    so vm_stop() just returns (IOW, the state is still RSTATE_PAUSED)

 3. The migration process completes. migrate_fd_put_ready() will now
    call runstate_set(RSTATE_POST_MIGRATE), which in turn causes the
    transition RSTATE_PAUSED -> RSTATE_POST_MIGRATE, which is invalid
    and the world of qemu ends

Now, we have three options to fix this but I don't know which one to choose:

 1. We could just add the transition RSTATE_PAUSED -> RSTATE_POST_MIGRATE
    as valid. Not sure this is a good thing to do though, as it seems a silly
    workaround for the fact that the transition to RSTATE_PRE_MIGRATE has
    never occurred

 2. This patch makes vm_stop() do the state transition even if the VM
    is already stopped. Seems good enough, except that I fear two things.
    First, today we know that vm_stop() is a no-op if the VM is already
    stopped, so there's a semantic change that could turn out to be trap.
    Second, I also fear people using vm_stop() as a way to change the
    VM status, just like runstate_set() (which can also become an horrible
    trap)

 3. Avi suggested we should keep a reference count, so that states are
    not discarded:

        http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00595.html

    That solution seemed to be the perfect one, except for one important
    detail: how should we implement vm_start() (and thus 'cont')?

    In order to maintain how we behave with the external world, the only
    option is that vm_start() will set the stop count to 0. Ie, doesn't
    matter if we have stopped the VM 500 times at some point, a vm_start()
    call will discard all stored states.

    Not sure if that's what you expected, but the first time I read Avi's
    idea I had the impression that it would be a good idea that vm_start()
    decremented the ref count only once, ie. vm_stop() and vm_start() calls
    have to match.

> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  cpus.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 8978779..eab8ff6 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -128,6 +128,8 @@ static void do_vm_stop(RunState state)
>          qemu_aio_flush();
>          bdrv_flush_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
> +    } else {
> +        runstate_set(state);
>      }
>  }
>