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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* zhanghailiang (zhang.zhanghailiang@huawei.com) 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 <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
> 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?

(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 / dgilbert@redhat.com / Manchester, UK