[libvirt] [PATCH v2 2/2] add interface for blkio.weight_device

[Daniel P. Berrange]

On Thu, Sep 08, 2011 at 02:59:56PM +0800, Hu Tao wrote:
> This patch adds a parameter --weight-device to virsh command
> blkiotune for setting/getting blkio.weight_device.
> ---
>  daemon/remote.c              |    5 +
>  include/libvirt/libvirt.h.in |    9 ++
>  src/conf/domain_conf.c       |  142 ++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h       |   15 ++++
>  src/libvirt_private.syms     |    1 +
>  src/qemu/qemu_cgroup.c       |   22 ++++++
>  src/qemu/qemu_driver.c       |  170 +++++++++++++++++++++++++++++++++++++++++-
>  src/util/cgroup.c            |   33 ++++++++
>  src/util/cgroup.h            |    3 +
>  tools/virsh.c                |   31 ++++++++
>  tools/virsh.pod              |    5 +-
>  11 files changed, 430 insertions(+), 6 deletions(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index a9d0daa..ec91526 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1503,6 +1503,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
>      int nparams = args->nparams;
>      unsigned int flags;
>      int rv = -1;
> +    int i;
>      struct daemonClientPrivate *priv =
>          virNetServerClientGetPrivateData(client);
>  
> @@ -1547,6 +1548,10 @@ success:
>  cleanup:
>      if (rv < 0)
>          virNetMessageSaveError(rerr);
> +    for (i = 0; i < nparams; i++) {
> +        if (params[i].type == VIR_TYPED_PARAM_STRING)
> +            VIR_FREE(params[i].value.s);
> +    }
>      VIR_FREE(params);
>      if (dom)
>          virDomainFree(dom);
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index e57241c..c65d8f7 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1137,6 +1137,15 @@ char *                  virDomainGetSchedulerType(virDomainPtr domain,
>  
>  #define VIR_DOMAIN_BLKIO_WEIGHT "weight"
>  
> +/**
> + * VIR_DOMAIN_BLKIO_WEIGHT_DEVICE:
> + *
> + * Macro for the blkio tunable weight_device: it represents the
> + * per device weight.
> + */
> +
> +#define VIR_DOMAIN_BLKIO_WEIGHT_DEVICE "weight_device"
> +
>  /* Set Blkio tunables for the domain*/
>  int     virDomainSetBlkioParameters(virDomainPtr domain,
>                                      virTypedParameterPtr params,
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 74f8d6a..d10e30c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -565,6 +565,108 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST,
>  #define VIR_DOMAIN_XML_WRITE_FLAGS  VIR_DOMAIN_XML_SECURE
>  #define VIR_DOMAIN_XML_READ_FLAGS   VIR_DOMAIN_XML_INACTIVE
>  
> +/**
> + * virBlkioWeightDeviceToStr:
> + *
> + * This function returns a string representing device weights that is
> + * suitable for writing to /cgroup/blkio/blkio.weight_device, given
> + * a list of weight devices.
> + */
> +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices,
> +                              int ndevices,
> +                              char **result)
> +{
> +    int len = 0;
> +    int ret = -1;
> +    int i, j;
> +    char **weight_devices;
> +    char *str;
> +
> +    if (VIR_ALLOC_N(weight_devices, ndevices) < 0) {
> +        goto fail_nomem1;
> +    }
> +    for (i = 0; i < ndevices; i++) {
> +        int tmp;
> +        tmp = virAsprintf(&weight_devices[i], "%d:%d %d",
> +                          weightdevices[i].major,
> +                          weightdevices[i].minor,
> +                          weightdevices[i].weight);
> +        if (tmp < 0) {
> +            goto fail_nomem2;
> +        }
> +        len += tmp + 1; /* 1 for '\n' and the trailing '\0' */
> +    }
> +
> +    if (VIR_ALLOC_N(str, len) < 0) {
> +        goto fail_nomem2;
> +    }
> +    for (i = 0; i < ndevices; i++) {
> +        strcat(str, weight_devices[i]);
> +        strcat(str, "\n");
> +    }
> +    str[len-1] = '\0';
> +
> +    *result = str;
> +
> +    ret = 0;
> +
> +fail_nomem2:
> +    for (j = 0; j < i; j++)
> +        VIR_FREE(weight_devices[i]);
> +    VIR_FREE(weight_devices);
> +fail_nomem1:
> +    if (ret != 0)
> +        virReportOOMError();
> +    return ret;
> +}

I think this method would become alot simpler if you switch
over to use virBufferPtr for all the string concatenation
and formatting.

> +
> +/**
> + * virDomainBlkioWeightDeviceParseXML
> + *
> + * this function parses a XML node:
> + *
> + *   <device>
> + *     <major>major</major>
> + *     <minor>minor</minor>
> + *     <weight>weight</weight>
> + *   </device>
> + *
> + * and fills a virBlkioWeightDevice struct.
> + */

I'm not really seeing the benefit in using major, minor in the XML for
this. The <disk> element is using the /dev/hda1 path for the host
device, so I'd expect the same path to be usable for the block I/O
tuning.

How does the scope work here, does major,minor have to refer to a block
device, or can it refer to a partition ?  If we have multiple <device>
elements, each giving a different partition on the same device can we
set different weight for each partition ?

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|