Re: [Qemu-devel] [PATCH COLO-Frame (Base) v21 03/17] migration: Enter into COLO mode after migration if COLO is enabled

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

On 2016/10/26 12:50, Amit Shah wrote:
> On (Tue) 18 Oct 2016 [20:09:59], zhanghailiang wrote:
>> Add a new migration state: MIGRATION_STATUS_COLO. Migration source side
>> enters this state after the first live migration successfully finished
>> if COLO is enabled by command 'migrate_set_capability x-colo on'.
>>
>> We reuse migration thread, so the process of checkpointing will be handled
>> in migration thread.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> (snip)
>
>> +static void colo_process_checkpoint(MigrationState *s)
>> +{
>> +    qemu_mutex_lock_iothread();
>> +    vm_start();
>> +    qemu_mutex_unlock_iothread();
>> +    trace_colo_vm_state_change("stop", "run");
>> +
>> +    /* TODO: COLO checkpoint savevm loop */
>> +
>> +    migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
>> +                      MIGRATION_STATUS_COMPLETED);
>
> Is this just a temporary thing that'll be removed in the next patches?

Yes, you are right, we will move this codes into failover process in the next
patch, because after failover, we should finish the original migration, there
are still some cleanup work need to be done.

> I guess so - because once you enter COLO state, you want to remain in
> it, right?
>

Yes.

> I think the commit message implies that.  So the commit msg and the
> code are not in sync.
>

Hmm, i'll remove it here in this patch, is it OK ?

> (snip)
>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index f7dd9c6..462007d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -695,6 +695,10 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>
>>           get_xbzrle_cache_stats(info);
>>           break;
>> +    case MIGRATION_STATUS_COLO:
>> +        info->has_status = true;
>> +        /* TODO: display COLO specific information (checkpoint info etc.) */
>> +        break;
>
> When do you plan to add this?  I guess it's important for debugging
> and also to get the state of the system while colo is active.  What
> info do you have planned to display here?
>

IIRC, we have such patch which implemented this specific information in the previous
version long time ago. Yes, it is quit useful, for example, the average/max time of
pause while do checkpoint, the average/max number of dirty pages transferred to SVM,
the amount time of VM in COLO state, the total checkpoint times, the count of
checkpointing because of inconsistency of network packages compare.

Thanks,
Hailiang

>
> 		Amit
>
> .
>