Re: [Xen-devel] [PATCH v10 24/31] Support colo mode for qemu disk

[Wen Congyang <wency@xxxxxxxxxxxxxx>]

On 03/02/2016 11:04 PM, Wei Liu wrote:
> On Mon, Feb 22, 2016 at 10:52:28AM +0800, Wen Congyang wrote:
>> Usage: disk = 
>> ['...,colo,colo-host=xxx,colo-port=xxx,colo-export=xxx,active-disk=xxx,hidden-disk=xxx...']
>> For QEMU block replication details:
>> http://wiki.qemu.org/Features/BlockReplication
>>
>> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
>> Signed-off-by: Yang Hongyang <hongyang.yang@xxxxxxxxxxxx>
>> ---
>>  docs/man/xl.pod.1                   |   2 +-
>>  docs/misc/xl-disk-configuration.txt |  50 ++++++++++
>>  tools/libxl/libxl.c                 |  62 +++++++++++-
>>  tools/libxl/libxl_create.c          |  25 ++++-
>>  tools/libxl/libxl_device.c          |  54 +++++++++++
>>  tools/libxl/libxl_dm.c              | 184 
>> ++++++++++++++++++++++++++++++++++--
>>  tools/libxl/libxl_types.idl         |   7 ++
>>  tools/libxl/libxlu_disk_l.l         |   7 ++
>>  8 files changed, 382 insertions(+), 9 deletions(-)
>>
>> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
>> index 1c6dd87..4f1901d 100644
>> --- a/docs/man/xl.pod.1
>> +++ b/docs/man/xl.pod.1
>> @@ -454,7 +454,7 @@ N.B: Remus support in xl is still in experimental 
>> (proof-of-concept) phase.
>>       Disk replication support is limited to DRBD disks.
>>  
>>       COLO support in xl is still in experimental (proof-of-concept) phase.
>> -     There is no support for network or disk at the moment.
>> +     There is no support for network at the moment.
> 
> You need some document here for the syntax, otherwise users have no clue
> how to configure disk replicate support. I also won't be able to
> meaningfully review this patch without a reference.

OK. will fix it in the next version.

> 
>>  
>>  B<OPTIONS>
>>  
>> diff --git a/docs/misc/xl-disk-configuration.txt 
>> b/docs/misc/xl-disk-configuration.txt
>> index 29f6ddb..6f23c2d 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -234,6 +234,56 @@ were intentionally created non-sparse to avoid 
>> fragmentation of the
>>  file.
>>  
>>  
> 
> Some nitpicking about the format below.
> 
>> +===============
>> +COLO PARAMETERS
>> +===============
>> +
>> +
>> +colo
>> +----
>> +
>> +Enable COLO HA for disk. For better understanding block replication on
>> +QEMU, please refer to:
>> +http://wiki.qemu.org/Features/BlockReplication
>> +
>> +
>> +colo-host
>> +---------
> 
> Blank line here please.
> 
>> +Description:           Secondary host's address
>> +Mandatory:             Yes when COLO enabled
>> +
>> +
>> +colo-port
>> +---------
> 
> Ditto.
> 
>> +Description:           Secondary port
>> +                       We will run a nbd server on secondary host,
>> +                       and the nbd server will listen this port.
>> +Mandatory:             Yes when COLO enabled
>> +
>> +
>> +colo-export
>> +---------
> 
> Here as well. And some more "-"s to match "colo-export".
> 
>> +Description:           We will run a nbd server on secondary host,
>> +                       exportname is the nbd server's disk export name.
>> +Mandatory:             Yes when COLO enabled
>> +
>> +
>> +active-disk
>> +-----------
>> +
>> +Description:           This is used by secondary. Secondary guest's write
>> +                       will be buffered in this disk.
>> +Mandatory:             Yes when COLO enabled
>> +
>> +
>> +hidden-disk
>> +-----------
>> +
>> +Description:           This is used by secondary. It buffers the original
>> +                       content that is modified by the primary VM.
>> +Mandatory:             Yes when COLO enabled
>> +
>> +
> 
> The rest of the patch is mainly for manipulating QEMU parameters. I've
> skipped it for now.

If you want to know about how qemu block repication works, you can see:
http://wiki.qemu.org/Features/BlockReplication

> 
>>  ============================================
>>  DEPRECATED PARAMETERS, PREFIXES AND SYNTAXES
>>  ============================================
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 12df81a..f691628 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2309,6 +2309,8 @@ int libxl__device_disk_setdefault(libxl__gc *gc, 
>> libxl_device_disk *disk)
>>      int rc;
>>  
>>      libxl_defbool_setdefault(&disk->discard_enable, !!disk->readwrite);
>> +    libxl_defbool_setdefault(&disk->colo_enable, false);
>> +    libxl_defbool_setdefault(&disk->colo_restore_enable, false);
>>  
>>      rc = libxl__resolve_domid(gc, disk->backend_domname, 
>> &disk->backend_domid);
>>      if (rc < 0) return rc;
>> @@ -2507,6 +2509,18 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
>> domid,
>>                  flexarray_append(back, "params");
>>                  flexarray_append(back, GCSPRINTF("%s:%s",
>>                                
>> libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
>> +                if (libxl_defbool_val(disk->colo_enable)) {
>> +                    flexarray_append(back, "colo-host");
>> +                    flexarray_append(back, libxl__sprintf(gc, "%s", 
>> disk->colo_host));
>> +                    flexarray_append(back, "colo-port");
>> +                    flexarray_append(back, libxl__sprintf(gc, "%s", 
>> disk->colo_port));
>> +                    flexarray_append(back, "colo-export");
>> +                    flexarray_append(back, libxl__sprintf(gc, "%s", 
>> disk->colo_export));
>> +                    flexarray_append(back, "active-disk");
>> +                    flexarray_append(back, libxl__sprintf(gc, "%s", 
>> disk->active_disk));
>> +                    flexarray_append(back, "hidden-disk");
>> +                    flexarray_append(back, libxl__sprintf(gc, "%s", 
>> disk->hidden_disk));
>> +                }
>>                  assert(device->backend_kind == LIBXL__DEVICE_KIND_QDISK);
>>                  break;
>>              default:
>> @@ -2622,7 +2636,10 @@ static int libxl__device_disk_from_xs_be(libxl__gc 
>> *gc,
>>          goto cleanup;
>>      }
>>  
>> -    /* "params" may not be present; but everything else must be. */
>> +    /*
>> +     * "params" and "colo-host" may not be present; but everything
>> +     * else must be.
> 
> I don't think this is correct. You have certainly have colo disabled.
> I think you can just avoid modifying this comment.

Hmm, add the following comments:
    /* colo releated entries(colo-host, colo-port, colo-export,
     * active-disk and hidden-disk) are present only if colo is
     * disabled.
     */

Thanks
Wen Congyang
> 
> Wei.
> 
> 
> .
> 




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel