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

[Wei Liu <wei.liu2@citrix.com>]

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@cn.fujitsu.com>
> Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
> ---
>  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.

>  
>  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.

>  ============================================
>  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.

Wei.

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