[Qemu-devel] Re: [PATCH v4] savevm: Fix no_migrate

[Alex Williamson]

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.

> 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);