Re: [Qemu-devel] [PATCH COLO-Frame v10 06/38] migration: Integrate COLO checkpoint process into loadvm

[zhanghailiang]

On 2015/11/7 1:29, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) wrote:
> > Switch from normal migration loadvm process into COLO checkpoint process if
> > COLO mode is enabled.
> > We add three new members to struct MigrationIncomingState, 
> > 'have_colo_incoming_thread'
> > and 'colo_incoming_thread' record the colo related threads for secondary VM,
> > 'migration_incoming_co' records the original migration incoming coroutine.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > ---
> > v10: fix a bug about fd leak which is found by Dave.
>
> <snip>
>
> > diff --git a/migration/migration.c b/migration/migration.c
> > index cf83531..7d8cd38 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -288,6 +288,27 @@ static void process_incoming_migration_co(void *opaque)
> >                         MIGRATION_STATUS_ACTIVE);
> >       ret = qemu_loadvm_state(f);
> >
> > +    if (!ret) {
> > +        /* Make sure all file formats flush their mutable metadata */
> > +        bdrv_invalidate_cache_all(&local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            migrate_decompress_threads_join();
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
>
> Are you moving this code? Because I think the bdrv_invalidate_cache_all is a 
> few lines
> below here - just....
>
> > +    /* we get colo info, and know if we are in colo mode */
> > +    if (!ret && migration_incoming_enable_colo()) {
> > +        mis->migration_incoming_co = qemu_coroutine_self();
> > +        qemu_thread_create(&mis->colo_incoming_thread, "colo incoming",
> > +             colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
> > +        mis->have_colo_incoming_thread = true;
> > +        qemu_coroutine_yield();
> > +
> > +        /* Wait checkpoint incoming thread exit before free resource */
> > +        qemu_thread_join(&mis->colo_incoming_thread);
> > +    }
> > +
> >       qemu_fclose(f);
> >       free_xbzrle_decoded_buf();
> >       migration_incoming_state_destroy();
>
>
> .... here in my current head world; so shouldn't you be deleting
> the bdrv_invalidate_cache_all here?

Good catch! I deleted it in patch 38, which should be moved into
this patch. I will fix it in next version.

Thanks,
zhanghailiang

> (Otherwise OK)
>
> Dave
>
> > diff --git a/stubs/migration-colo.c b/stubs/migration-colo.c
> > index acddca6..c12516e 100644
> > --- a/stubs/migration-colo.c
> > +++ b/stubs/migration-colo.c
> > @@ -22,6 +22,16 @@ bool migration_in_colo_state(void)
> >       return false;
> >   }
> >
> > +bool migration_incoming_in_colo_state(void)
> > +{
> > +    return false;
> > +}
> > +
> >   void migrate_start_colo_process(MigrationState *s)
> >   {
> >   }
> > +
> > +void *colo_process_incoming_thread(void *opaque)
> > +{
> > +    return NULL;
> > +}
> > --
> > 1.8.3.1
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
> .