* [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
@ 2012-10-18 9:14 Paolo Bonzini
2012-10-18 16:42 ` Luiz Capitulino
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2012-10-18 9:14 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Right now, stop followed by an incoming migration will let the
virtual machine start. cont before an incoming migration instead
will fail.
This is bad because the actual behavior is not predictable; it is
racy with respect to the start of the incoming migration. That's
because incoming migration is blocking, and thus will delay the
processing of stop/cont until the end of the migration.
In addition, there's nothing that really prevents the user from
typing the block device's passwords before incoming migration is
done, so we may as well allow that.
Both things can be fixed by just toggling the autostart variable when
stop/cont are called in INMIGRATE state.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qmp.c | 17 +++++++++++------
1 file modificato, 11 inserzioni(+), 6 rimozioni(-)
diff --git a/qmp.c b/qmp.c
index 36c54c5..2c8d559 100644
--- a/qmp.c
+++ b/qmp.c
@@ -85,7 +85,11 @@ void qmp_quit(Error **err)
void qmp_stop(Error **errp)
{
- vm_stop(RUN_STATE_PAUSED);
+ if (runstate_check(RUN_STATE_INMIGRATE)) {
+ autostart = 0;
+ } else {
+ vm_stop(RUN_STATE_PAUSED);
+ }
}
void qmp_system_reset(Error **errp)
@@ -144,10 +148,7 @@ void qmp_cont(Error **errp)
{
Error *local_err = NULL;
- if (runstate_check(RUN_STATE_INMIGRATE)) {
- error_set(errp, QERR_MIGRATION_EXPECTED);
- return;
- } else if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
+ if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
runstate_check(RUN_STATE_SHUTDOWN)) {
error_set(errp, QERR_RESET_REQUIRED);
return;
@@ -162,7 +163,11 @@ void qmp_cont(Error **errp)
return;
}
- vm_start();
+ if (runstate_check(RUN_STATE_INMIGRATE)) {
+ autostart = 1;
+ } else {
+ vm_start();
+ }
}
void qmp_system_wakeup(Error **errp)
--
1.7.12.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
2012-10-18 9:14 [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state Paolo Bonzini
@ 2012-10-18 16:42 ` Luiz Capitulino
2012-10-19 9:27 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Capitulino @ 2012-10-18 16:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Thu, 18 Oct 2012 11:14:22 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Right now, stop followed by an incoming migration will let the
> virtual machine start. cont before an incoming migration instead
> will fail.
>
> This is bad because the actual behavior is not predictable; it is
> racy with respect to the start of the incoming migration. That's
> because incoming migration is blocking, and thus will delay the
> processing of stop/cont until the end of the migration.
That's not correct if "delay" means "it will be queued" (I guess you
didn't mean that, but "blocking" followed "will delay the processing"
gives that impression).
What happens is that a stop command while in INMIGRATE state will just
be ignored. Actually, any stops while in a state that pauses vCPUs are
ignored.
Also, I don't understand what you meant by "racy", care to elaborate?
> In addition, there's nothing that really prevents the user from
> typing the block device's passwords before incoming migration is
> done, so we may as well allow that.
Have you tried it? We seem to support that already.
> Both things can be fixed by just toggling the autostart variable when
> stop/cont are called in INMIGRATE state.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qmp.c | 17 +++++++++++------
> 1 file modificato, 11 inserzioni(+), 6 rimozioni(-)
>
> diff --git a/qmp.c b/qmp.c
> index 36c54c5..2c8d559 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -85,7 +85,11 @@ void qmp_quit(Error **err)
>
> void qmp_stop(Error **errp)
> {
> - vm_stop(RUN_STATE_PAUSED);
> + if (runstate_check(RUN_STATE_INMIGRATE)) {
> + autostart = 0;
I'm not against this change, but it has two issues. First, after the
incoming migration ends, the state will be PRELAUNCH; so we have to
decide if this is a good state for this and if it is, then we have to
update its schema doc.
The other (possible) problem is that we're changing the semantics of
the stop command on INMIGRATE. I can't tell if this actually matters, but
it could come back in the form of compatibility breakage. This is also
true for the cont change below.
> + } else {
> + vm_stop(RUN_STATE_PAUSED);
> + }
> }
>
> void qmp_system_reset(Error **errp)
> @@ -144,10 +148,7 @@ void qmp_cont(Error **errp)
> {
> Error *local_err = NULL;
>
> - if (runstate_check(RUN_STATE_INMIGRATE)) {
> - error_set(errp, QERR_MIGRATION_EXPECTED);
Please, drop QERR_MIGRATION_EXPECTED.
> - return;
> - } else if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> + if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> runstate_check(RUN_STATE_SHUTDOWN)) {
> error_set(errp, QERR_RESET_REQUIRED);
> return;
> @@ -162,7 +163,11 @@ void qmp_cont(Error **errp)
> return;
> }
>
> - vm_start();
> + if (runstate_check(RUN_STATE_INMIGRATE)) {
> + autostart = 1;
> + } else {
> + vm_start();
> + }
> }
>
> void qmp_system_wakeup(Error **errp)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
2012-10-18 16:42 ` Luiz Capitulino
@ 2012-10-19 9:27 ` Paolo Bonzini
2012-10-19 13:23 ` Luiz Capitulino
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2012-10-19 9:27 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
> What happens is that a stop command while in INMIGRATE state will just
> be ignored. Actually, any stops while in a state that pauses vCPUs
> are ignored.
>
> Also, I don't understand what you meant by "racy", care to elaborate?
Case 1: Case 2:
user runs qemu -incoming -S user runs qemu -incoming -S
source connects cont command received
cont command received source connects
In case 1, the VM is resumed at the end of migration, in case 2 it
is not and an error is reported on QMP. After this patch, it is always
resumed.
Case 1: Case 2:
user runs qemu -incoming user runs qemu -incoming
source connects stop command received
stop command received source connects
In case 1, the VM runs for a blink of an eye and then stops. In case
2 it just starts, with no error reported.
> > In addition, there's nothing that really prevents the user from
> > typing the block device's passwords before incoming migration is
> > done, so we may as well allow that.
>
> Have you tried it? We seem to support that already.
With HMP yes, with QMP no. You just get "An incoming migration is
expected before this command can be executed" and no clue that disks
are encrypted.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
2012-10-19 9:27 ` Paolo Bonzini
@ 2012-10-19 13:23 ` Luiz Capitulino
2012-10-19 13:50 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Capitulino @ 2012-10-19 13:23 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Fri, 19 Oct 2012 05:27:29 -0400 (EDT)
Paolo Bonzini <pbonzini@redhat.com> wrote:
> > What happens is that a stop command while in INMIGRATE state will just
> > be ignored. Actually, any stops while in a state that pauses vCPUs
> > are ignored.
> >
> > Also, I don't understand what you meant by "racy", care to elaborate?
>
> Case 1: Case 2:
> user runs qemu -incoming -S user runs qemu -incoming -S
> source connects cont command received
> cont command received source connects
>
> In case 1, the VM is resumed at the end of migration, in case 2 it
> is not and an error is reported on QMP. After this patch, it is always
> resumed.
>
> Case 1: Case 2:
> user runs qemu -incoming user runs qemu -incoming
> source connects stop command received
> stop command received source connects
>
> In case 1, the VM runs for a blink of an eye and then stops. In case
> 2 it just starts, with no error reported.
>
> > > In addition, there's nothing that really prevents the user from
> > > typing the block device's passwords before incoming migration is
> > > done, so we may as well allow that.
> >
> > Have you tried it? We seem to support that already.
>
> With HMP yes, with QMP no. You just get "An incoming migration is
> expected before this command can be executed" and no clue that disks
> are encrypted.
We could move the bdrv_iterate() calls before the RUN_STATE_INMIGRATE check
in qmp_cont(), but if this patch fixes all the cases you mention then let's
just apply it.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
2012-10-19 13:23 ` Luiz Capitulino
@ 2012-10-19 13:50 ` Paolo Bonzini
2012-10-19 14:02 ` Luiz Capitulino
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2012-10-19 13:50 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
Il 19/10/2012 15:23, Luiz Capitulino ha scritto:
>> >
>> > With HMP yes, with QMP no. You just get "An incoming migration is
>> > expected before this command can be executed" and no clue that disks
>> > are encrypted.
> We could move the bdrv_iterate() calls before the RUN_STATE_INMIGRATE check
> in qmp_cont(), but if this patch fixes all the cases you mention then let's
> just apply it.
I'm all for that. :)
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state
2012-10-19 13:50 ` Paolo Bonzini
@ 2012-10-19 14:02 ` Luiz Capitulino
0 siblings, 0 replies; 6+ messages in thread
From: Luiz Capitulino @ 2012-10-19 14:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Fri, 19 Oct 2012 15:50:17 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 19/10/2012 15:23, Luiz Capitulino ha scritto:
> >> >
> >> > With HMP yes, with QMP no. You just get "An incoming migration is
> >> > expected before this command can be executed" and no clue that disks
> >> > are encrypted.
> > We could move the bdrv_iterate() calls before the RUN_STATE_INMIGRATE check
> > in qmp_cont(), but if this patch fixes all the cases you mention then let's
> > just apply it.
>
> I'm all for that. :)
I guess it won't have any affects on the -S races.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-19 14:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-18 9:14 [Qemu-devel] [PATCH] qmp: handle stop/cont in INMIGRATE state Paolo Bonzini
2012-10-18 16:42 ` Luiz Capitulino
2012-10-19 9:27 ` Paolo Bonzini
2012-10-19 13:23 ` Luiz Capitulino
2012-10-19 13:50 ` Paolo Bonzini
2012-10-19 14:02 ` Luiz Capitulino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).