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

[Hailiang Zhang]

On 2016/10/27 11:58, Amit Shah wrote:
> On (Wed) 26 Oct 2016 [21:49:10], Hailiang Zhang wrote:
> > 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 <address@hidden>
> > > > Signed-off-by: Li Zhijian <address@hidden>
> > > > Signed-off-by: Gonglei <address@hidden>
> > > > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > >
> > > (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 ?
>
> Yes.
>
> > > (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.
>
> Yes, please get this in soon as well.

OK, we will do it, thanks.

>                 Amit
>
> .