Re: [Qemu-devel] [PATCH RFC v3 04/27] migration: Integrate COLO checkpoint process into migration

[zhanghailiang]

On 2015/2/17 7:27, Eric Blake wrote:
> On 02/11/2015 08:16 PM, zhanghailiang wrote:
> > Add a migrate state: MIG_STATE_COLO, enter this migration state
> > after the first live migration successfully finished.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Gonglei <address@hidden>
> > Signed-off-by: Lai Jiangshan <address@hidden>
> > ---
> >   include/migration/migration-colo.h |  2 ++
> >   include/migration/migration.h      | 13 +++++++
> >   migration/Makefile.objs            |  1 +
> >   migration/colo.c                   | 72 ++++++++++++++++++++++++++++++++++++++
> >   migration/migration.c              | 38 +++++++++++---------
> >   stubs/Makefile.objs                |  1 +
> >   stubs/migration-colo.c             | 17 +++++++++
> >   7 files changed, 128 insertions(+), 16 deletions(-)
> >   create mode 100644 migration/colo.c
> >   create mode 100644 stubs/migration-colo.c
>
> > +++ b/include/migration/migration.h
> > @@ -65,6 +65,19 @@ struct MigrationState
> >       int64_t dirty_sync_count;
> >   };
> >
> > +enum {
> > +    MIG_STATE_ERROR = -1,
> > +    MIG_STATE_NONE,
> > +    MIG_STATE_SETUP,
> > +    MIG_STATE_CANCELLING,
> > +    MIG_STATE_CANCELLED,
> > +    MIG_STATE_ACTIVE,
> > +    MIG_STATE_COLO,
> > +    MIG_STATE_COMPLETED,
> > +};
>
> Is the new state intended to be user-visible?  If so, wouldn't it be
> better to expose this enum via qapi-schema.json?

No, for now it is only used internally.

> > +
> > +/* #define DEBUG_COLO */
> > +
> > +#ifdef DEBUG_COLO
> > +#define DPRINTF(fmt, ...) \
> > +do { fprintf(stdout, "colo: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while (0)
> > +#endif
> > +
>
> Same comment as in 3/27 about avoiding bit-rotting debug statements.  Or
> even better,...

OK, will fix it.

> > +static QEMUBH *colo_bh;
> > +
> > +static void *colo_thread(void *opaque)
> > +{
> > +    MigrationState *s = opaque;
> > +
> > +    qemu_mutex_lock_iothread();
> > +    vm_start();
> > +    qemu_mutex_unlock_iothread();
> > +    DPRINTF("vm resume to run\n");
>
> ...why not add tracepoints instead of using DPRINTF?

Hmm, we will change it to using tracepoints, for now, we use DPRINTF just for 
convenience.

> > @@ -227,6 +218,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >
> >           get_xbzrle_cache_stats(info);
> >           break;
> > +    case MIG_STATE_COLO:
> > +        info->has_status = true;
> > +        info->status = g_strdup("colo");
> > +        /* TODO: display COLO specific informations(checkpoint info etc.),*/
> > +        break;
>
> Uggh.  We REALLY need to fix MigrationInfo to convert 'status' to use an
> enum type, instead of an open-coded 'str' (such a conversion is
> backwards compatible, and better documented).  Then it would be more
> obvious that you are adding an enum value.  Doing the conversion would
> be a good prerequisite patch.

Good idea, i will do this, send a patch like that. ;)

> s/informations(checkpoint info etc.),/information (checkpoint info etc.)/

Will fix it, thanks.