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

[Eric Blake]

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?


> +
> +/* #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,...

> +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?


> @@ -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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature