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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

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 <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>   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.