Re: [PATCH v11 20/27] Support colo mode for qemu disk

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Changlong Xie writes ("[PATCH v11 20/27] Support colo mode for qemu disk"):
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> 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

So now I am slightly confused by the design, I think.

When you replicate a VM with COLO using xl, its memory state is
transferred over ssh.  But its disk replication is done unencrypted
and unauthenticated ?

And the disk replication is, out of band, and needs to be configured
separately ?  This is rather awkward, although maybe not a
showstopper.  (Maybe we can have a plan to fix it in the future...)

And, how does the disk replication, which doesn't depend on there
being xl running, relate to the vm state replication, which does ?  I
think at the very least I'd like to see some information about the
principles of operation - either explained, or referred to, in the
user manual.

Is it possible to use COLO with an existing full-service disk
replication service such as DRBD ?

> +(a) An example for COLO replication's configuration: disk =['...,colo,colo-host
> +=xxx,colo-port=xxx,colo-export=xxx,active-disk=xxx,hidden-disk=xxx...']
> +
> +=item B<colo-host>      :Secondary host's ip address.
> +
> +=item B<colo-port>      :Secondary host's port, we will run a nbd server on
> +secondary host, and the nbd server will listen this port.
> +
> +=item B<colo-export>    :Nbd server's disk export name of secondary host.
> +
> +=item B<active-disk>    :Secondary's guest write will be buffered in this disk,
> +and it's used by secondary.
> +
> +=item B<hidden-disk>    :Primary's modified contents will be buffered in this
> +disk, and it's used by secondary.

What would a typical configuration look like ?  I don't understand the
relationship between active-disk and hidden-disk, etc.

> +colo-host
> +---------
> +
> +Description:           Secondary host's address
> +Mandatory:             Yes when COLO enabled

Is it permitted to specify a host DNS name ?

> +    if (libxl_defbool_val(disk->colo_enable)) {
> +        tmp = xs_read(ctx->xsh, XBT_NULL,
> +                      GCSPRINTF("%s/colo-port", be_path), &len);
> +        if (!tmp) {
> +            LOG(ERROR, "Missing xenstore node %s/colo-port", be_path);
> +            goto cleanup;
> +        }
> +        disk->colo_port = tmp;

This is quite repetitive code.


> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 3610a39..bff08b0 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1800,12 +1800,29 @@ static void domain_create_cb(libxl__egc *egc,
...
> @@ -256,6 +280,36 @@ static int disk_try_backend(disk_try_backend_args *a,
>      LOG(DEBUG, "Disk vdev=%s, backend %s not compatible with script=...",
>          a->disk->vdev, libxl_disk_backend_to_string(backend));
>      return 0;
> +
> + bad_colo:
> +    LOG(DEBUG, "Disk vdev=%s, backend %s not compatible with colo",
> +        a->disk->vdev, libxl_disk_backend_to_string(backend));
> +    return 0;

This is correct here, I think.

> + bad_colo_host:
> +    LOG(DEBUG, "Disk vdev=%s, backend %s needs colo-host=... for colo",
> +        a->disk->vdev, libxl_disk_backend_to_string(backend));
> +    return 0;

I think these options should be checked later.  disk_try_backend isn't
the place for general parameter checking; it is searching for which
backend to try.

>  int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4aca38e..ba17251 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -751,6 +751,139 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
...
> +static char *qemu_disk_scsi_drive_string(libxl__gc *gc, const char *pdev_path,
> +                                         int unit, const char *format,
> +                                         const libxl_device_disk *disk,
> +                                         int colo_mode)
> +{
> +    char *drive = NULL;
> +    const char *exportname = disk->colo_export;
> +    const char *active_disk = disk->active_disk;
> +    const char *hidden_disk = disk->hidden_disk;
> +
> +    switch (colo_mode) {
> +    case LIBXL__COLO_NONE:
> +        drive = libxl__sprintf
> +            (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
> +             pdev_path, unit, format);

I think this would be a lot clearer if the refactoring was split into
a seperate patch.

>                  if (strncmp(disks[i].vdev, "sd", 2) == 0) {
> -                    drive = libxl__sprintf
> -                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,readonly=%s,cache=writeback",
> -                         pdev_path, disk, format, disks[i].readwrite ? "off" : "on");
> +                    if (colo_mode == LIBXL__COLO_SECONDARY) {
> +                        /*
> +                         * -drive if=none,driver=format,file=pdev_path,\
> +                         * id=exportname
> +                         */

I think this comment adds nothing to the code and could be profitably
omitted.

> +                        drive = libxl__sprintf
> +                            (gc, "if=none,driver=%s,file=%s,id=%s",
> +                             format, pdev_path, disks[i].colo_export);

I don't understand how this works here.  COLO_SECONDARY seems to
suppress the rest of the disk specification.

Also, this same logic seems to appear many times.  Maybe it could be
centralised.  Perhaps I would be able to advise more clearly if I
understood how this was put together.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9b0a537..a2078d1 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -575,6 +575,13 @@ libxl_device_disk = Struct("device_disk", [
>      ("is_cdrom", integer),
>      ("direct_io_safe", bool),
>      ("discard_enable", libxl_defbool),
> +    ("colo_enable", libxl_defbool),
> +    ("colo_restore_enable", libxl_defbool),
> +    ("colo_host", string),
> +    ("colo_port", string),
> +    ("colo_export", string),
> +    ("active_disk", string),
> +    ("hidden_disk", string)

In general, many these should probably not be strings.  Certainly the
port should be an integer.  I don't quite understand the semantics of
the others.

Ian.

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