Re: [PATCH v6 11/18] tools/libxl: Add back channel to allow migration target send data back

[Wen Congyang <wency@cn.fujitsu.com>]

On 01/26/2016 03:17 AM, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 30, 2015 at 10:29:01AM +0800, Wen Congyang wrote:
>> In colo mode, slave needs to send data to master, but the io_fd
> 
> 
> 
> In previous patches you used COLO in all caps, can that be uniform
> across the patches?

OK, I will check it.

> 
> Also, slave == secondary and master == primary? Perhaps you
> could s/slave/secondary/ s/master/primary/ to sync up with
> the other patches?

OK, I will do it.

> 
> Thank you!
>> only can be written in master, and only can be read in slave.
> 
> 
> 
> Could you mention what kind of data the secondary has to send
> to the primary? In the previous patch (] tools/libxl:
> introduce libxl__domain_common_switch_qemu_logdirty()) it mentioned
> dirty page. Is that the case here? If so can you mention
> that as well here?

OK. Will fix it in the next version.

> 
> 
>> Save recv_fd in domain_suspend_state, and send_fd in
>> domain_create_state.
>> Extend libxl_domain_create_restore API, add a send_fd param to
>> it.
>> Add LIBXL_HAVE_CREATE_RESTORE_SEND_FD to indicate the API change.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>>  tools/libxl/libxl.c                  |  2 +-
>>  tools/libxl/libxl.h                  | 30 ++++++++++++++++++++++++++++--
>>  tools/libxl/libxl_create.c           |  9 +++++----
>>  tools/libxl/libxl_internal.h         |  2 ++
>>  tools/libxl/libxl_types.idl          |  1 +
>>  tools/libxl/xl_cmdimpl.c             |  8 +++++++-
>>  tools/ocaml/libs/xl/xenlight_stubs.c |  2 +-
>>  7 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 2faea4d..69c8047 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -872,7 +872,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>>      dss->callback = remus_failover_cb;
>>      dss->domid = domid;
>>      dss->fd = send_fd;
>> -    /* TODO do something with recv_fd */
>> +    dss->recv_fd = recv_fd;
>>      dss->type = type;
>>      dss->live = 1;
>>      dss->debug = 0;
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index a01e448..67a4ad7 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -630,6 +630,15 @@ typedef struct libxl__ctx libxl_ctx;
>>  #define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS 1
>>  
>>  /*
>> + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_FD 1
>> + *
>> + * If this is defined, libxl_domain_create_restore()'s API has changed to
>> + * include a send_fd param which used for libxl migration back channel
>> + * during COLO FT.
> 
> FT? Could you explain that acronym please?

Fault Tolerance. COLO is a FT solution.
In the comment, I think FT can be removed.

>> + */
>> +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_FD 1
>> +
>> +/*
>>   * LIBXL_HAVE_CREATEINFO_PVH
>>   * If this is defined, then libxl supports creation of a PVH guest.
>>   */
>> @@ -1143,7 +1152,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
>>                              const libxl_asyncprogress_how *aop_console_how)
>>                              LIBXL_EXTERNAL_CALLERS_ONLY;
>>  int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
>> -                                uint32_t *domid, int restore_fd,
>> +                                uint32_t *domid, int restore_fd, int send_fd,
>>                                  const libxl_domain_restore_params *params,
>>                                  const libxl_asyncop_how *ao_how,
>>                                  const libxl_asyncprogress_how *aop_console_how)
>> @@ -1164,7 +1173,7 @@ int static inline libxl_domain_create_restore_0x040200(
>>      libxl_domain_restore_params_init(&params);
>>  
>>      ret = libxl_domain_create_restore(
>> -        ctx, d_config, domid, restore_fd, &params, ao_how, aop_console_how);
>> +        ctx, d_config, domid, restore_fd, -1, &params, ao_how, aop_console_how);
>>  
>>      libxl_domain_restore_params_dispose(&params);
>>      return ret;
>> @@ -1172,6 +1181,23 @@ int static inline libxl_domain_create_restore_0x040200(
>>  
>>  #define libxl_domain_create_restore libxl_domain_create_restore_0x040200
>>  
>> +#elif defined(LIBXL_API_VERSION) && LIBXL_API_VERSION >= 0x040400 \
>> +                                 && LIBXL_API_VERSION < 0x040600
> 
> s/4060/4070? Or is that suppose to be <= 040600 ?

It is 4070 here. I just rebase this series, and forgot to update it.
Thanks for pointing it out.

>> +
> .. snip..
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 9aa94be..c5d5d40 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -232,6 +232,7 @@ libxl_hdtype = Enumeration("hdtype", [
>>  libxl_checkpointed_stream = Enumeration("checkpointed_stream", [
>>      (0, "NONE"),
>>      (1, "REMUS"),
>> +    (2, "COLO"),
> 
> You should also update the migration_stream enum with the extra enum.
> 
> And if you follow my idea of adding an assertion in xc_domain_save
> for the different checkpointed_stream types then that would need to
> be expanded to include support for COLO type as well.

Yes, but I don't find the codes that use this new type. I wil check it.

Thanks
Wen Congyang

> 
> 
> 
> .
>