Re: [PATCH v6 08/11] migration: add new migration state wait-unplug

[Dr. David Alan Gilbert]

* Jens Freimann (address@hidden) wrote:
> Dr. David Alan Gilbert <address@hidden> schrieb am Di., 29. Okt. 2019,
> 03:57:
> 
> > * Jens Freimann (address@hidden) wrote:
> > > This patch adds a new migration state called wait-unplug.  It is entered
> > > after the SETUP state if failover devices are present. It will transition
> > > into ACTIVE once all devices were succesfully unplugged from the guest.
> > >
> > > So if a guest doesn't respond or takes long to honor the unplug request
> > > the user will see the migration state 'wait-unplug'.
> > >
> > > In the migration thread we query failover devices if they're are still
> > > pending the guest unplug. When all are unplugged the migration
> > > continues. If one device won't unplug migration will stay in wait_unplug
> > > state.
> > >
> > > Signed-off-by: Jens Freimann <address@hidden>
> > > Acked-by: Cornelia Huck <address@hidden>
> >
> > I think this is OK, so
> >
> >
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> >
> > but see question below
> >
> > > ---
> > >  include/migration/vmstate.h |  2 ++
> > >  migration/migration.c       | 21 +++++++++++++++++++++
> > >  migration/migration.h       |  3 +++
> > >  migration/savevm.c          | 36 ++++++++++++++++++++++++++++++++++++
> > >  migration/savevm.h          |  2 ++
> > >  qapi/migration.json         |  5 ++++-
> > >  6 files changed, 68 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index b9ee563aa4..ac4f46a67d 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -186,6 +186,8 @@ struct VMStateDescription {
> > >      int (*pre_save)(void *opaque);
> > >      int (*post_save)(void *opaque);
> > >      bool (*needed)(void *opaque);
> > > +    bool (*dev_unplug_pending)(void *opaque);
> > > +
> > >      const VMStateField *fields;
> > >      const VMStateDescription **subsections;
> > >  };
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 3febd0f8f3..51764f2565 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -52,6 +52,7 @@
> > >  #include "hw/qdev-properties.h"
> > >  #include "monitor/monitor.h"
> > >  #include "net/announce.h"
> > > +#include "qemu/queue.h"
> > >
> > >  #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed
> > throttling */
> > >
> > > @@ -819,6 +820,7 @@ bool migration_is_setup_or_active(int state)
> > >      case MIGRATION_STATUS_SETUP:
> > >      case MIGRATION_STATUS_PRE_SWITCHOVER:
> > >      case MIGRATION_STATUS_DEVICE:
> > > +    case MIGRATION_STATUS_WAIT_UNPLUG:
> > >          return true;
> > >
> > >      default:
> > > @@ -954,6 +956,9 @@ static void fill_source_migration_info(MigrationInfo
> > *info)
> > >      case MIGRATION_STATUS_CANCELLED:
> > >          info->has_status = true;
> > >          break;
> > > +    case MIGRATION_STATUS_WAIT_UNPLUG:
> > > +        info->has_status = true;
> > > +        break;
> > >      }
> > >      info->status = s->state;
> > >  }
> > > @@ -1694,6 +1699,7 @@ bool migration_is_idle(void)
> > >      case MIGRATION_STATUS_COLO:
> > >      case MIGRATION_STATUS_PRE_SWITCHOVER:
> > >      case MIGRATION_STATUS_DEVICE:
> > > +    case MIGRATION_STATUS_WAIT_UNPLUG:
> > >          return false;
> > >      case MIGRATION_STATUS__MAX:
> > >          g_assert_not_reached();
> > > @@ -3264,6 +3270,19 @@ static void *migration_thread(void *opaque)
> > >
> > >      qemu_savevm_state_setup(s->to_dst_file);
> > >
> > > +    if (qemu_savevm_nr_failover_devices()) {
> > > +        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > > +                          MIGRATION_STATUS_WAIT_UNPLUG);
> > > +
> > > +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> > > +                !qemu_savevm_state_guest_unplug_pending()) {
> > > +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> > > +        }
> > > +
> > > +        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> > > +                MIGRATION_STATUS_ACTIVE);
> > > +    }
> > > +
> > >      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> > >      migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > >                        MIGRATION_STATUS_ACTIVE);
> > > @@ -3511,6 +3530,7 @@ static void migration_instance_finalize(Object
> > *obj)
> > >      qemu_mutex_destroy(&ms->qemu_file_lock);
> > >      g_free(params->tls_hostname);
> > >      g_free(params->tls_creds);
> > > +    qemu_sem_destroy(&ms->wait_unplug_sem);
> > >      qemu_sem_destroy(&ms->rate_limit_sem);
> > >      qemu_sem_destroy(&ms->pause_sem);
> > >      qemu_sem_destroy(&ms->postcopy_pause_sem);
> > > @@ -3556,6 +3576,7 @@ static void migration_instance_init(Object *obj)
> > >      qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
> > >      qemu_sem_init(&ms->rp_state.rp_sem, 0);
> > >      qemu_sem_init(&ms->rate_limit_sem, 0);
> > > +    qemu_sem_init(&ms->wait_unplug_sem, 0);
> > >      qemu_mutex_init(&ms->qemu_file_lock);
> > >  }
> > >
> > > diff --git a/migration/migration.h b/migration/migration.h
> > > index 4f2fe193dc..79b3dda146 100644
> > > --- a/migration/migration.h
> > > +++ b/migration/migration.h
> > > @@ -206,6 +206,9 @@ struct MigrationState
> > >      /* Flag set once the migration thread called bdrv_inactivate_all */
> > >      bool block_inactive;
> > >
> > > +    /* Migration is waiting for guest to unplug device */
> > > +    QemuSemaphore wait_unplug_sem;
> > > +
> > >      /* Migration is paused due to pause-before-switchover */
> > >      QemuSemaphore pause_sem;
> > >
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 8d95e261f6..0f18dea49e 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -1113,6 +1113,42 @@ void qemu_savevm_state_header(QEMUFile *f)
> > >      }
> > >  }
> > >
> > > +int qemu_savevm_nr_failover_devices(void)
> > > +{
> > > +    SaveStateEntry *se;
> > > +    int n = 0;
> > > +
> > > +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > > +        if (se->vmsd && se->vmsd->dev_unplug_pending) {
> > > +            n++;
> > > +        }
> > > +    }
> > > +
> > > +    return n;
> > > +}
> > > +
> > > +bool qemu_savevm_state_guest_unplug_pending(void)
> > > +{
> > > +    int nr_failover_devs;
> > > +    SaveStateEntry *se;
> > > +    bool ret = false;
> > > +    int n = 0;
> > > +
> > > +    nr_failover_devs = qemu_savevm_nr_failover_devices();
> > > +
> > > +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > > +        if (!se->vmsd || !se->vmsd->dev_unplug_pending) {
> > > +            continue;
> > > +        }
> > > +        ret = se->vmsd->dev_unplug_pending(se->opaque);
> > > +        if (!ret) {
> > > +            n++;
> > > +        }
> > > +    }
> > > +
> > > +    return n == nr_failover_devs;
> >
> > I was expecting != I think?  If all the devices say
> > they've got one pending then doesn't n==nr_failover_devs and
> > it returns true? But then what happens if only one has one pending?
> >
> 
> It's increased when unplug pending is false, which means the device is
> done. So it returns true when all devices are done with unplugging. It is
> correct but not obvious. I can reverse it in a follow up to make it more
> clear.

Yes it doesn't quite match the name of the function; and/or add some
comments!

Dave

> regards,
> Jens
> 
> >
> > Dave
> >
> > > +}
> > > +
> > >  void qemu_savevm_state_setup(QEMUFile *f)
> > >  {
> > >      SaveStateEntry *se;
> > > diff --git a/migration/savevm.h b/migration/savevm.h
> > > index 51a4b9caa8..c42b9c80ee 100644
> > > --- a/migration/savevm.h
> > > +++ b/migration/savevm.h
> > > @@ -31,6 +31,8 @@
> > >
> > >  bool qemu_savevm_state_blocked(Error **errp);
> > >  void qemu_savevm_state_setup(QEMUFile *f);
> > > +int qemu_savevm_nr_failover_devices(void);
> > > +bool qemu_savevm_state_guest_unplug_pending(void);
> > >  int qemu_savevm_state_resume_prepare(MigrationState *s);
> > >  void qemu_savevm_state_header(QEMUFile *f);
> > >  int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > index e9e7a97c03..b7348d0c8b 100644
> > > --- a/qapi/migration.json
> > > +++ b/qapi/migration.json
> > > @@ -133,6 +133,9 @@
> > >  # @device: During device serialisation when pause-before-switchover is
> > enabled
> > >  #        (since 2.11)
> > >  #
> > > +# @wait-unplug: wait for device unplug request by guest OS to be
> > completed.
> > > +#               (since 4.2)
> > > +#
> > >  # Since: 2.3
> > >  #
> > >  ##
> > > @@ -140,7 +143,7 @@
> > >    'data': [ 'none', 'setup', 'cancelling', 'cancelled',
> > >              'active', 'postcopy-active', 'postcopy-paused',
> > >              'postcopy-recover', 'completed', 'failed', 'colo',
> > > -            'pre-switchover', 'device' ] }
> > > +            'pre-switchover', 'device', 'wait-unplug' ] }
> > >
> > >  ##
> > >  # @MigrationInfo:
> > > --
> > > 2.21.0
> > >
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> >
> >
> >
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK