* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-07 22:13 ` [Qemu-devel] [PATCH v4] " Alex Williamson
@ 2011-01-09 10:02 ` Michael S. Tsirkin
2011-01-09 10:47 ` Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2011-01-09 10:02 UTC (permalink / raw)
To: Alex Williamson; +Cc: jan.kiszka, qemu-devel, quintela
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> The no_migrate save state flag is currently only checked in the
> last phase of migration. This means that we potentially waste
> a lot of time and bandwidth with the live state handlers before
> we ever check the no_migrate flags. The error message printed
> when we catch a non-migratable device doesn't get printed for
> a detached migration. And, no_migrate does nothing to prevent
> an incoming migration to a target that includes a non-migratable
> device. This attempts to fix all of these.
Nod. Sounds good.
> One notable difference in behavior is that an outgoing migration
> now checks for non-migratable devices before ever connecting to
> the target system. This means the target will remain listening
> rather than exit from failure.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Looks like a feature, but it won't be hard to make it exit
if we want it for compatibility....
> ---
>
> v4:
> - fix braces noted by Jan
> - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> at qemu_loadvm_state(), since it'a already using errno values
>
> v3:
>
> Daniel, adding you to see if libvirt cares about the difference in
> whether the target exits on migration failure as noted above.
>
> Also adding Michael as he's complained about the no_migrate check
> happening so late in the process.
>
> migration.c | 4 ++++
> savevm.c | 41 +++++++++++++++++++++++++++--------------
> sysemu.h | 1 +
> 3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index e5ba51c..d593b1d 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> return -1;
> }
>
> + if (qemu_savevm_state_blocked(mon)) {
> + return -1;
> + }
> +
> if (strstart(uri, "tcp:", &p)) {
> s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
> blk, inc);
> diff --git a/savevm.c b/savevm.c
> index 90aa237..34c0d1a 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
> return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
> }
>
> -static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> +static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> {
> - if (se->no_migrate) {
> - return -1;
> - }
> -
> if (!se->vmsd) { /* Old style */
> se->save_state(f, se->opaque);
> - return 0;
> + return;
> }
> vmstate_save_state(f,se->vmsd, se->opaque);
> -
> - return 0;
> }
>
> #define QEMU_VM_FILE_MAGIC 0x5145564d
> @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> #define QEMU_VM_SECTION_FULL 0x04
> #define QEMU_VM_SUBSECTION 0x05
>
> +int qemu_savevm_state_blocked(Monitor *mon)
> +{
> + SaveStateEntry *se;
> +
> + QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> + if (se->no_migrate) {
> + monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> + se->idstr);
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
> +
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared)
> {
> @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> {
> SaveStateEntry *se;
> - int r;
>
> cpu_synchronize_all_states();
>
> @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> qemu_put_be32(f, se->instance_id);
> qemu_put_be32(f, se->version_id);
>
> - r = vmstate_save(f, se);
> - if (r < 0) {
> - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> - return r;
> - }
> + vmstate_save(f, se);
> }
>
> qemu_put_byte(f, QEMU_VM_EOF);
> @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> saved_vm_running = vm_running;
> vm_stop(0);
>
> + ret = qemu_savevm_state_blocked(mon);
> + if (ret < 0) {
> + goto out;
> + }
> +
> ret = qemu_savevm_state_begin(mon, f, 0, 0);
> if (ret < 0)
> goto out;
> @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f)
> unsigned int v;
> int ret;
>
> + ret = qemu_savevm_state_blocked(default_mon);
> + if (ret < 0) {
> + return ret;
> + }
> +
> v = qemu_get_be32(f);
> if (v != QEMU_VM_FILE_MAGIC)
> return -EINVAL;
> diff --git a/sysemu.h b/sysemu.h
> index e728ea1..eefaba5 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -75,6 +75,7 @@ void qemu_announce_self(void);
>
> void main_loop_wait(int nonblocking);
>
> +int qemu_savevm_state_blocked(Monitor *mon);
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared);
> int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-07 22:13 ` [Qemu-devel] [PATCH v4] " Alex Williamson
2011-01-09 10:02 ` [Qemu-devel] " Michael S. Tsirkin
@ 2011-01-09 10:47 ` Michael S. Tsirkin
2011-01-10 17:47 ` Alex Williamson
2011-01-10 10:24 ` Daniel P. Berrange
2011-01-11 21:39 ` [Qemu-devel] [PATCH v5] " Alex Williamson
3 siblings, 1 reply; 23+ messages in thread
From: Michael S. Tsirkin @ 2011-01-09 10:47 UTC (permalink / raw)
To: Alex Williamson; +Cc: jan.kiszka, qemu-devel, quintela
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> The no_migrate save state flag is currently only checked in the
> last phase of migration. This means that we potentially waste
> a lot of time and bandwidth with the live state handlers before
> we ever check the no_migrate flags. The error message printed
> when we catch a non-migratable device doesn't get printed for
> a detached migration. And, no_migrate does nothing to prevent
> an incoming migration to a target that includes a non-migratable
> device. This attempts to fix all of these.
>
> One notable difference in behavior is that an outgoing migration
> now checks for non-migratable devices before ever connecting to
> the target system. This means the target will remain listening
> rather than exit from failure.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Minor nit:
The API of qemu_savevm_state_blocked is a bit strange.
functions should either return 0/1 values or 0 on success
negative on failure or a bool.
This API asks "is it blocked" and returns -EINVAL to
mean "yes". _blocked is also a bit ambigious:
it seems to imply a temporary condition.
How about we reverse the logic, call the new API
qemu_savevm_state_supported, qemu_savevm_state_enabled,
something like this?
Then you can return 0 if migration is possible,
-1 if not.
> ---
>
> v4:
> - fix braces noted by Jan
> - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> at qemu_loadvm_state(), since it'a already using errno values
>
> v3:
>
> Daniel, adding you to see if libvirt cares about the difference in
> whether the target exits on migration failure as noted above.
>
> Also adding Michael as he's complained about the no_migrate check
> happening so late in the process.
>
> migration.c | 4 ++++
> savevm.c | 41 +++++++++++++++++++++++++++--------------
> sysemu.h | 1 +
> 3 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index e5ba51c..d593b1d 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> return -1;
> }
>
> + if (qemu_savevm_state_blocked(mon)) {
> + return -1;
> + }
> +
> if (strstart(uri, "tcp:", &p)) {
> s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
> blk, inc);
> diff --git a/savevm.c b/savevm.c
> index 90aa237..34c0d1a 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
> return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
> }
>
> -static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> +static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> {
> - if (se->no_migrate) {
> - return -1;
> - }
> -
> if (!se->vmsd) { /* Old style */
> se->save_state(f, se->opaque);
> - return 0;
> + return;
> }
> vmstate_save_state(f,se->vmsd, se->opaque);
> -
> - return 0;
> }
>
> #define QEMU_VM_FILE_MAGIC 0x5145564d
> @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> #define QEMU_VM_SECTION_FULL 0x04
> #define QEMU_VM_SUBSECTION 0x05
>
> +int qemu_savevm_state_blocked(Monitor *mon)
> +{
> + SaveStateEntry *se;
> +
> + QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> + if (se->no_migrate) {
> + monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> + se->idstr);
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
> +
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared)
> {
> @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> {
> SaveStateEntry *se;
> - int r;
>
> cpu_synchronize_all_states();
>
> @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> qemu_put_be32(f, se->instance_id);
> qemu_put_be32(f, se->version_id);
>
> - r = vmstate_save(f, se);
> - if (r < 0) {
> - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> - return r;
> - }
> + vmstate_save(f, se);
> }
>
> qemu_put_byte(f, QEMU_VM_EOF);
> @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> saved_vm_running = vm_running;
> vm_stop(0);
>
> + ret = qemu_savevm_state_blocked(mon);
> + if (ret < 0) {
> + goto out;
> + }
> +
> ret = qemu_savevm_state_begin(mon, f, 0, 0);
> if (ret < 0)
> goto out;
> @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f)
> unsigned int v;
> int ret;
>
> + ret = qemu_savevm_state_blocked(default_mon);
> + if (ret < 0) {
> + return ret;
> + }
> +
> v = qemu_get_be32(f);
> if (v != QEMU_VM_FILE_MAGIC)
> return -EINVAL;
> diff --git a/sysemu.h b/sysemu.h
> index e728ea1..eefaba5 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -75,6 +75,7 @@ void qemu_announce_self(void);
>
> void main_loop_wait(int nonblocking);
>
> +int qemu_savevm_state_blocked(Monitor *mon);
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared);
> int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-09 10:47 ` Michael S. Tsirkin
@ 2011-01-10 17:47 ` Alex Williamson
2011-01-10 21:19 ` Michael S. Tsirkin
0 siblings, 1 reply; 23+ messages in thread
From: Alex Williamson @ 2011-01-10 17:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jan.kiszka, qemu-devel, quintela
On Sun, 2011-01-09 at 12:47 +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> > The no_migrate save state flag is currently only checked in the
> > last phase of migration. This means that we potentially waste
> > a lot of time and bandwidth with the live state handlers before
> > we ever check the no_migrate flags. The error message printed
> > when we catch a non-migratable device doesn't get printed for
> > a detached migration. And, no_migrate does nothing to prevent
> > an incoming migration to a target that includes a non-migratable
> > device. This attempts to fix all of these.
> >
> > One notable difference in behavior is that an outgoing migration
> > now checks for non-migratable devices before ever connecting to
> > the target system. This means the target will remain listening
> > rather than exit from failure.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>
> Minor nit:
>
> The API of qemu_savevm_state_blocked is a bit strange.
> functions should either return 0/1 values or 0 on success
> negative on failure or a bool.
> This API asks "is it blocked" and returns -EINVAL to
> mean "yes". _blocked is also a bit ambigious:
> it seems to imply a temporary condition.
But it very well could be a temporary condition. If I have an assigned
device, it will block migration/savevm, but removing the device allows
it to proceed.
> How about we reverse the logic, call the new API
> qemu_savevm_state_supported, qemu_savevm_state_enabled,
> something like this?
> Then you can return 0 if migration is possible,
> -1 if not.
I tend to like qemu_savevm_state_blocked() as "supported" and "enabled"
invoke different ideas for me. I will concede that the errno return
value is a little awkward. How about if I make it a bool function
instead? Thanks,
Alex
> > ---
> >
> > v4:
> > - fix braces noted by Jan
> > - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> > at qemu_loadvm_state(), since it'a already using errno values
> >
> > v3:
> >
> > Daniel, adding you to see if libvirt cares about the difference in
> > whether the target exits on migration failure as noted above.
> >
> > Also adding Michael as he's complained about the no_migrate check
> > happening so late in the process.
> >
> > migration.c | 4 ++++
> > savevm.c | 41 +++++++++++++++++++++++++++--------------
> > sysemu.h | 1 +
> > 3 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/migration.c b/migration.c
> > index e5ba51c..d593b1d 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > return -1;
> > }
> >
> > + if (qemu_savevm_state_blocked(mon)) {
> > + return -1;
> > + }
> > +
> > if (strstart(uri, "tcp:", &p)) {
> > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
> > blk, inc);
> > diff --git a/savevm.c b/savevm.c
> > index 90aa237..34c0d1a 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
> > return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
> > }
> >
> > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > {
> > - if (se->no_migrate) {
> > - return -1;
> > - }
> > -
> > if (!se->vmsd) { /* Old style */
> > se->save_state(f, se->opaque);
> > - return 0;
> > + return;
> > }
> > vmstate_save_state(f,se->vmsd, se->opaque);
> > -
> > - return 0;
> > }
> >
> > #define QEMU_VM_FILE_MAGIC 0x5145564d
> > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > #define QEMU_VM_SECTION_FULL 0x04
> > #define QEMU_VM_SUBSECTION 0x05
> >
> > +int qemu_savevm_state_blocked(Monitor *mon)
> > +{
> > + SaveStateEntry *se;
> > +
> > + QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> > + if (se->no_migrate) {
> > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> > + se->idstr);
> > + return -EINVAL;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> > int shared)
> > {
> > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> > {
> > SaveStateEntry *se;
> > - int r;
> >
> > cpu_synchronize_all_states();
> >
> > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> > qemu_put_be32(f, se->instance_id);
> > qemu_put_be32(f, se->version_id);
> >
> > - r = vmstate_save(f, se);
> > - if (r < 0) {
> > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> > - return r;
> > - }
> > + vmstate_save(f, se);
> > }
> >
> > qemu_put_byte(f, QEMU_VM_EOF);
> > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> > saved_vm_running = vm_running;
> > vm_stop(0);
> >
> > + ret = qemu_savevm_state_blocked(mon);
> > + if (ret < 0) {
> > + goto out;
> > + }
> > +
> > ret = qemu_savevm_state_begin(mon, f, 0, 0);
> > if (ret < 0)
> > goto out;
> > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f)
> > unsigned int v;
> > int ret;
> >
> > + ret = qemu_savevm_state_blocked(default_mon);
> > + if (ret < 0) {
> > + return ret;
> > + }
> > +
> > v = qemu_get_be32(f);
> > if (v != QEMU_VM_FILE_MAGIC)
> > return -EINVAL;
> > diff --git a/sysemu.h b/sysemu.h
> > index e728ea1..eefaba5 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -75,6 +75,7 @@ void qemu_announce_self(void);
> >
> > void main_loop_wait(int nonblocking);
> >
> > +int qemu_savevm_state_blocked(Monitor *mon);
> > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> > int shared);
> > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-10 17:47 ` Alex Williamson
@ 2011-01-10 21:19 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2011-01-10 21:19 UTC (permalink / raw)
To: Alex Williamson; +Cc: jan.kiszka, qemu-devel, quintela
On Mon, Jan 10, 2011 at 10:47:58AM -0700, Alex Williamson wrote:
> On Sun, 2011-01-09 at 12:47 +0200, Michael S. Tsirkin wrote:
> > On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> > > The no_migrate save state flag is currently only checked in the
> > > last phase of migration. This means that we potentially waste
> > > a lot of time and bandwidth with the live state handlers before
> > > we ever check the no_migrate flags. The error message printed
> > > when we catch a non-migratable device doesn't get printed for
> > > a detached migration. And, no_migrate does nothing to prevent
> > > an incoming migration to a target that includes a non-migratable
> > > device. This attempts to fix all of these.
> > >
> > > One notable difference in behavior is that an outgoing migration
> > > now checks for non-migratable devices before ever connecting to
> > > the target system. This means the target will remain listening
> > > rather than exit from failure.
> > >
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >
> > Minor nit:
> >
> > The API of qemu_savevm_state_blocked is a bit strange.
> > functions should either return 0/1 values or 0 on success
> > negative on failure or a bool.
> > This API asks "is it blocked" and returns -EINVAL to
> > mean "yes". _blocked is also a bit ambigious:
> > it seems to imply a temporary condition.
>
> But it very well could be a temporary condition. If I have an assigned
> device, it will block migration/savevm, but removing the device allows
> it to proceed.
Well to me blocked it means migration will not fail, but will be blocked
and unblocked later when device is removed.
> > How about we reverse the logic, call the new API
> > qemu_savevm_state_supported, qemu_savevm_state_enabled,
> > something like this?
> > Then you can return 0 if migration is possible,
> > -1 if not.
>
> I tend to like qemu_savevm_state_blocked() as "supported" and "enabled"
> invoke different ideas for me. I will concede that the errno return
> value is a little awkward. How about if I make it a bool function
> instead? Thanks,
>
> Alex
That's better.
> > > ---
> > >
> > > v4:
> > > - fix braces noted by Jan
> > > - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> > > at qemu_loadvm_state(), since it'a already using errno values
> > >
> > > v3:
> > >
> > > Daniel, adding you to see if libvirt cares about the difference in
> > > whether the target exits on migration failure as noted above.
> > >
> > > Also adding Michael as he's complained about the no_migrate check
> > > happening so late in the process.
> > >
> > > migration.c | 4 ++++
> > > savevm.c | 41 +++++++++++++++++++++++++++--------------
> > > sysemu.h | 1 +
> > > 3 files changed, 32 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/migration.c b/migration.c
> > > index e5ba51c..d593b1d 100644
> > > --- a/migration.c
> > > +++ b/migration.c
> > > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > > return -1;
> > > }
> > >
> > > + if (qemu_savevm_state_blocked(mon)) {
> > > + return -1;
> > > + }
> > > +
> > > if (strstart(uri, "tcp:", &p)) {
> > > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
> > > blk, inc);
> > > diff --git a/savevm.c b/savevm.c
> > > index 90aa237..34c0d1a 100644
> > > --- a/savevm.c
> > > +++ b/savevm.c
> > > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
> > > return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
> > > }
> > >
> > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > > {
> > > - if (se->no_migrate) {
> > > - return -1;
> > > - }
> > > -
> > > if (!se->vmsd) { /* Old style */
> > > se->save_state(f, se->opaque);
> > > - return 0;
> > > + return;
> > > }
> > > vmstate_save_state(f,se->vmsd, se->opaque);
> > > -
> > > - return 0;
> > > }
> > >
> > > #define QEMU_VM_FILE_MAGIC 0x5145564d
> > > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> > > #define QEMU_VM_SECTION_FULL 0x04
> > > #define QEMU_VM_SUBSECTION 0x05
> > >
> > > +int qemu_savevm_state_blocked(Monitor *mon)
> > > +{
> > > + SaveStateEntry *se;
> > > +
> > > + QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> > > + if (se->no_migrate) {
> > > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> > > + se->idstr);
> > > + return -EINVAL;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> > > int shared)
> > > {
> > > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> > > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> > > {
> > > SaveStateEntry *se;
> > > - int r;
> > >
> > > cpu_synchronize_all_states();
> > >
> > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> > > qemu_put_be32(f, se->instance_id);
> > > qemu_put_be32(f, se->version_id);
> > >
> > > - r = vmstate_save(f, se);
> > > - if (r < 0) {
> > > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> > > - return r;
> > > - }
> > > + vmstate_save(f, se);
> > > }
> > >
> > > qemu_put_byte(f, QEMU_VM_EOF);
> > > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> > > saved_vm_running = vm_running;
> > > vm_stop(0);
> > >
> > > + ret = qemu_savevm_state_blocked(mon);
> > > + if (ret < 0) {
> > > + goto out;
> > > + }
> > > +
> > > ret = qemu_savevm_state_begin(mon, f, 0, 0);
> > > if (ret < 0)
> > > goto out;
> > > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f)
> > > unsigned int v;
> > > int ret;
> > >
> > > + ret = qemu_savevm_state_blocked(default_mon);
> > > + if (ret < 0) {
> > > + return ret;
> > > + }
> > > +
> > > v = qemu_get_be32(f);
> > > if (v != QEMU_VM_FILE_MAGIC)
> > > return -EINVAL;
> > > diff --git a/sysemu.h b/sysemu.h
> > > index e728ea1..eefaba5 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -75,6 +75,7 @@ void qemu_announce_self(void);
> > >
> > > void main_loop_wait(int nonblocking);
> > >
> > > +int qemu_savevm_state_blocked(Monitor *mon);
> > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> > > int shared);
> > > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-07 22:13 ` [Qemu-devel] [PATCH v4] " Alex Williamson
2011-01-09 10:02 ` [Qemu-devel] " Michael S. Tsirkin
2011-01-09 10:47 ` Michael S. Tsirkin
@ 2011-01-10 10:24 ` Daniel P. Berrange
2011-01-10 14:52 ` Alex Williamson
2011-01-11 21:39 ` [Qemu-devel] [PATCH v5] " Alex Williamson
3 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2011-01-10 10:24 UTC (permalink / raw)
To: Alex Williamson; +Cc: mst, jan.kiszka, qemu-devel, quintela
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> The no_migrate save state flag is currently only checked in the
> last phase of migration. This means that we potentially waste
> a lot of time and bandwidth with the live state handlers before
> we ever check the no_migrate flags. The error message printed
> when we catch a non-migratable device doesn't get printed for
> a detached migration. And, no_migrate does nothing to prevent
> an incoming migration to a target that includes a non-migratable
> device. This attempts to fix all of these.
>
> One notable difference in behavior is that an outgoing migration
> now checks for non-migratable devices before ever connecting to
> the target system. This means the target will remain listening
> rather than exit from failure.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> v4:
> - fix braces noted by Jan
> - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> at qemu_loadvm_state(), since it'a already using errno values
>
> v3:
>
> Daniel, adding you to see if libvirt cares about the difference in
> whether the target exits on migration failure as noted above.
If the 'migrate' command on the source QEMU returns an error,
then libvirt will teardown the target QEMU automatically, so
that's not a problem.
Regards,
Daniel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
2011-01-10 10:24 ` Daniel P. Berrange
@ 2011-01-10 14:52 ` Alex Williamson
0 siblings, 0 replies; 23+ messages in thread
From: Alex Williamson @ 2011-01-10 14:52 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: mst, jan.kiszka, qemu-devel, quintela
On Mon, 2011-01-10 at 10:24 +0000, Daniel P. Berrange wrote:
> On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote:
> > The no_migrate save state flag is currently only checked in the
> > last phase of migration. This means that we potentially waste
> > a lot of time and bandwidth with the live state handlers before
> > we ever check the no_migrate flags. The error message printed
> > when we catch a non-migratable device doesn't get printed for
> > a detached migration. And, no_migrate does nothing to prevent
> > an incoming migration to a target that includes a non-migratable
> > device. This attempts to fix all of these.
> >
> > One notable difference in behavior is that an outgoing migration
> > now checks for non-migratable devices before ever connecting to
> > the target system. This means the target will remain listening
> > rather than exit from failure.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> > v4:
> > - fix braces noted by Jan
> > - return error from qemu_savevm_state_blocked rather than fixed EINVAL
> > at qemu_loadvm_state(), since it'a already using errno values
> >
> > v3:
> >
> > Daniel, adding you to see if libvirt cares about the difference in
> > whether the target exits on migration failure as noted above.
>
> If the 'migrate' command on the source QEMU returns an error,
> then libvirt will teardown the target QEMU automatically, so
> that's not a problem.
Thanks, that's the way I was hoping it would work.
Alex
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v5] savevm: Fix no_migrate
2011-01-07 22:13 ` [Qemu-devel] [PATCH v4] " Alex Williamson
` (2 preceding siblings ...)
2011-01-10 10:24 ` Daniel P. Berrange
@ 2011-01-11 21:39 ` Alex Williamson
2011-01-11 22:39 ` [Qemu-devel] " Michael S. Tsirkin
3 siblings, 1 reply; 23+ messages in thread
From: Alex Williamson @ 2011-01-11 21:39 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.williamson, mst, jan.kiszka, quintela
The no_migrate save state flag is currently only checked in the
last phase of migration. This means that we potentially waste
a lot of time and bandwidth with the live state handlers before
we ever check the no_migrate flags. The error message printed
when we catch a non-migratable device doesn't get printed for
a detached migration. And, no_migrate does nothing to prevent
an incoming migration to a target that includes a non-migratable
device. This attempts to fix all of these.
One notable difference in behavior is that an outgoing migration
now checks for non-migratable devices before ever connecting to
the target system. This means the target will remain listening
rather than exit from failure.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
v5:
- change qemu_savevm_state_blocked() to return bool
- Daniel confirmed libvirt will not have a problem with the behavior
change on the target end.
migration.c | 4 ++++
savevm.c | 40 ++++++++++++++++++++++++++--------------
sysemu.h | 1 +
3 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/migration.c b/migration.c
index e5ba51c..d593b1d 100644
--- a/migration.c
+++ b/migration.c
@@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
return -1;
}
+ if (qemu_savevm_state_blocked(mon)) {
+ return -1;
+ }
+
if (strstart(uri, "tcp:", &p)) {
s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
blk, inc);
diff --git a/savevm.c b/savevm.c
index 90aa237..fcd8db4 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
}
-static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
+static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
{
- if (se->no_migrate) {
- return -1;
- }
-
if (!se->vmsd) { /* Old style */
se->save_state(f, se->opaque);
- return 0;
+ return;
}
vmstate_save_state(f,se->vmsd, se->opaque);
-
- return 0;
}
#define QEMU_VM_FILE_MAGIC 0x5145564d
@@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
#define QEMU_VM_SECTION_FULL 0x04
#define QEMU_VM_SUBSECTION 0x05
+bool qemu_savevm_state_blocked(Monitor *mon)
+{
+ SaveStateEntry *se;
+
+ QTAILQ_FOREACH(se, &savevm_handlers, entry) {
+ if (se->no_migrate) {
+ monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
+ se->idstr);
+ return true;
+ }
+ }
+ return false;
+}
+
int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
int shared)
{
@@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
{
SaveStateEntry *se;
- int r;
cpu_synchronize_all_states();
@@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
qemu_put_be32(f, se->instance_id);
qemu_put_be32(f, se->version_id);
- r = vmstate_save(f, se);
- if (r < 0) {
- monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
- return r;
- }
+ vmstate_save(f, se);
}
qemu_put_byte(f, QEMU_VM_EOF);
@@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
saved_vm_running = vm_running;
vm_stop(0);
+ if (qemu_savevm_state_blocked(mon)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
ret = qemu_savevm_state_begin(mon, f, 0, 0);
if (ret < 0)
goto out;
@@ -1692,6 +1700,10 @@ int qemu_loadvm_state(QEMUFile *f)
unsigned int v;
int ret;
+ if (qemu_savevm_state_blocked(default_mon)) {
+ return -EINVAL;
+ }
+
v = qemu_get_be32(f);
if (v != QEMU_VM_FILE_MAGIC)
return -EINVAL;
diff --git a/sysemu.h b/sysemu.h
index d8fceec..0c969f2 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -75,6 +75,7 @@ void qemu_announce_self(void);
void main_loop_wait(int nonblocking);
+bool qemu_savevm_state_blocked(Monitor *mon);
int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
int shared);
int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH v5] savevm: Fix no_migrate
2011-01-11 21:39 ` [Qemu-devel] [PATCH v5] " Alex Williamson
@ 2011-01-11 22:39 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2011-01-11 22:39 UTC (permalink / raw)
To: Alex Williamson; +Cc: jan.kiszka, qemu-devel, quintela
On Tue, Jan 11, 2011 at 02:39:43PM -0700, Alex Williamson wrote:
> The no_migrate save state flag is currently only checked in the
> last phase of migration. This means that we potentially waste
> a lot of time and bandwidth with the live state handlers before
> we ever check the no_migrate flags. The error message printed
> when we catch a non-migratable device doesn't get printed for
> a detached migration. And, no_migrate does nothing to prevent
> an incoming migration to a target that includes a non-migratable
> device. This attempts to fix all of these.
>
> One notable difference in behavior is that an outgoing migration
> now checks for non-migratable devices before ever connecting to
> the target system. This means the target will remain listening
> rather than exit from failure.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> v5:
> - change qemu_savevm_state_blocked() to return bool
> - Daniel confirmed libvirt will not have a problem with the behavior
> change on the target end.
>
> migration.c | 4 ++++
> savevm.c | 40 ++++++++++++++++++++++++++--------------
> sysemu.h | 1 +
> 3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index e5ba51c..d593b1d 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> return -1;
> }
>
> + if (qemu_savevm_state_blocked(mon)) {
> + return -1;
> + }
> +
> if (strstart(uri, "tcp:", &p)) {
> s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
> blk, inc);
> diff --git a/savevm.c b/savevm.c
> index 90aa237..fcd8db4 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id)
> return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
> }
>
> -static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> +static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
> {
> - if (se->no_migrate) {
> - return -1;
> - }
> -
> if (!se->vmsd) { /* Old style */
> se->save_state(f, se->opaque);
> - return 0;
> + return;
> }
> vmstate_save_state(f,se->vmsd, se->opaque);
> -
> - return 0;
> }
>
> #define QEMU_VM_FILE_MAGIC 0x5145564d
> @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se)
> #define QEMU_VM_SECTION_FULL 0x04
> #define QEMU_VM_SUBSECTION 0x05
>
> +bool qemu_savevm_state_blocked(Monitor *mon)
> +{
> + SaveStateEntry *se;
> +
> + QTAILQ_FOREACH(se, &savevm_handlers, entry) {
> + if (se->no_migrate) {
> + monitor_printf(mon, "state blocked by non-migratable device '%s'\n",
> + se->idstr);
> + return true;
> + }
> + }
> + return false;
> +}
> +
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared)
> {
> @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
> int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> {
> SaveStateEntry *se;
> - int r;
>
> cpu_synchronize_all_states();
>
> @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f)
> qemu_put_be32(f, se->instance_id);
> qemu_put_be32(f, se->version_id);
>
> - r = vmstate_save(f, se);
> - if (r < 0) {
> - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr);
> - return r;
> - }
> + vmstate_save(f, se);
> }
>
> qemu_put_byte(f, QEMU_VM_EOF);
> @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
> saved_vm_running = vm_running;
> vm_stop(0);
>
> + if (qemu_savevm_state_blocked(mon)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> ret = qemu_savevm_state_begin(mon, f, 0, 0);
> if (ret < 0)
> goto out;
> @@ -1692,6 +1700,10 @@ int qemu_loadvm_state(QEMUFile *f)
> unsigned int v;
> int ret;
>
> + if (qemu_savevm_state_blocked(default_mon)) {
> + return -EINVAL;
> + }
> +
> v = qemu_get_be32(f);
> if (v != QEMU_VM_FILE_MAGIC)
> return -EINVAL;
> diff --git a/sysemu.h b/sysemu.h
> index d8fceec..0c969f2 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -75,6 +75,7 @@ void qemu_announce_self(void);
>
> void main_loop_wait(int nonblocking);
>
> +bool qemu_savevm_state_blocked(Monitor *mon);
> int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
> int shared);
> int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
^ permalink raw reply [flat|nested] 23+ messages in thread