[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate |
Date: |
Mon, 10 Jan 2011 23:19:07 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> >
> > 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);
>
>
- [Qemu-devel] Re: [PATCH V2] savevm: use error_report for vmstate_save error, (continued)
[Qemu-devel] [PATCH v3] savevm: Fix no_migrate, Alex Williamson, 2011/01/07
[Qemu-devel] [PATCH v4] savevm: Fix no_migrate, Alex Williamson, 2011/01/07
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate, Michael S. Tsirkin, 2011/01/09
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate, Michael S. Tsirkin, 2011/01/09
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate, Alex Williamson, 2011/01/10
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate,
Michael S. Tsirkin <=
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate, Daniel P. Berrange, 2011/01/10
[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate, Alex Williamson, 2011/01/10
[Qemu-devel] [PATCH v5] savevm: Fix no_migrate, Alex Williamson, 2011/01/11
[Qemu-devel] Re: [PATCH v5] savevm: Fix no_migrate, Michael S. Tsirkin, 2011/01/11