[libvirt] [PATCH v3 2/2] add interface for blkio.weight_device
Eric Blake
eblake at redhat.com
Fri Sep 23 17:46:46 UTC 2011
On 09/23/2011 12:40 AM, 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 | 114 ++++++++++++++++++++++++-
> src/conf/domain_conf.h | 16 ++++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_cgroup.c | 22 +++++
> src/qemu/qemu_driver.c | 191 +++++++++++++++++++++++++++++++++++++++++-
> src/util/cgroup.c | 33 +++++++
> src/util/cgroup.h | 3 +
> tools/virsh.c | 52 ++++++++++--
> tools/virsh.pod | 5 +-
> 11 files changed, 437 insertions(+), 14 deletions(-)
>
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 82ee13b..bee1b94 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1566,6 +1566,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
> int nparams = args->nparams;
> unsigned int flags;
> int rv = -1;
> + int i;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
>
> @@ -1610,6 +1611,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);
This for loop seems like something that will be done frequently enough
that it might be worth factoring it into a util file for managing
virTypedParameters. Something like: virTypedParameterFree(params).
> if (dom)
> virDomainFree(dom);
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 448a0e7..a1f2c98 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1139,6 +1139,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.
> + */
Also mention that this name refers to a VIR_TYPED_PARAMETER_STRING.
> +/**
> + * virDomainBlkioWeightDeviceParseXML
> + *
> + * this function parses a XML node:
> + *
> + *<device>
> + *<path>/fully/qulaified/device/path</path>
s/qulaified/qualified/
> + *<weight>weight</weight>
> + *</device>
> + *
> + * and fills a virBlkioWeightDevice struct.
> + */
> +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root,
> + virBlkioWeightDevicePtr dw)
> +{
> + char *c;
> + struct stat s;
> + xmlNodePtr node;
> +
> + if (!dw)
> + return -1;
> +
> + node = root->children;
> + while (node) {
> + if (node->type == XML_ELEMENT_NODE) {
> + if (xmlStrEqual(node->name, BAD_CAST "path")) {
> + c = (char *)xmlNodeGetContent(node);
> + if (!c)
> + return -1;
> + if (stat(c,&s) == -1)
> + return -1;
This stat() ties the parse to the existence of the node. But that seems
wrong - it should be possible to define a domain with XML that refers to
a node that does not yet exist, provided that the node later exists at
the time we try to start the domain.
> + if ((s.st_mode& S_IFMT) == S_IFBLK) {
> + dw->major = major(s.st_rdev);
> + dw->minor = minor(s.st_rdev);
Also, breaking the parse into major/minor this early makes the result
unmigratable, since we can't guarantee major/minor stability across
hosts. I think you have to store the name, not the major/minor, here in
the domain_conf representation of the device weight, and only convert to
major/minor in the hypervisor that is actually enforcing the limits.
> + } else
> + return -1;
> + dw->path = (char *)xmlNodeGetContent(node);
> + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) {
> + c = (char *)xmlNodeGetContent(node);
> + dw->weight = atoi(c);
atoi is unsafe. It cannot detect errors. You should use virStrToLong_i
(or similar) instead.
> @@ -10499,10 +10591,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> def->mem.cur_balloon);
>
> /* add blkiotune only if there are any */
> - if (def->blkio.weight) {
> + if (def->blkio.weight || def->blkio.weight_devices) {
> virBufferAsprintf(buf, "<blkiotune>\n");
> - virBufferAsprintf(buf, "<weight>%u</weight>\n",
> - def->blkio.weight);
> +
> + if (def->blkio.weight)
> + virBufferAsprintf(buf, "<weight>%u</weight>\n",
(Stupid Thunderbird for munging whitespace).
This will conflict with my patch series for indenting <domainsnapshot>.
Shouldn't be too hard to figure out, but it boils down to who gets
acked first.
> +++ b/src/qemu/qemu_driver.c
> @@ -44,6 +44,7 @@
> #include<sys/ioctl.h>
> #include<sys/un.h>
> #include<byteswap.h>
> +#include<ctype.h>
This should use "c-ctype.h", not <ctype.h>.
> +/* weightDeviceStr in the form of /device/path,weight;/device/path,weight
> + * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800
> + */
> +static int parseBlkioWeightDeviceStr(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size)
> +{
> + struct stat s;
> + const char *temp;
> + int nDevice = 0;
> + int i, len;
> + virBlkioWeightDevicePtr result = NULL;
> +
> + if (!dw)
> + return -1;
> + if (*dw)
> + return -1;
This returns -1 without an error, but you later return -1 after
reporting OOM error, so the caller can't tell things apart. That means
you need to report an error here. On the other hand, since this
function is static, you could audit that all callers pass correct
parameters in the first place, and just omit this validation step.
> +
> + temp = weightDeviceStr;
> + while (temp) {
> + temp = strchr(temp, ';');
What happens if the absolute path of a device contains a literal ';'? I
guess we don't anticipate that happening.
> + i = 0;
> + temp = weightDeviceStr;
> + while (temp&& i< nDevice) {
> + const char *p = temp;
> +
> + /* device path */
> +
> + while (*p != ','&& *p != '\0')
> + ++p;
strchr(p,',') would be faster.
> + if (*p == '\0')
> + goto fail;
> + len = p - temp + 1;
> + if (VIR_ALLOC_N(result[i].path, len)< 0)
> + goto fail;
> + memcpy(result[i].path, temp, len - 1);
strndup() would be better than VIR_ALLOC_N and memcpy.
> + result[i].path[len - 1] = '\0';
> +
> + if (stat(result[i].path,&s) == -1) {
> + VIR_FREE(result[i].path);
> + goto fail;
> + }
> + if ((s.st_mode& S_IFMT) != S_IFBLK) {
> + VIR_FREE(result[i].path);
> + goto fail;
> + }
> + result[i].major = major(s.st_rdev);
> + result[i].minor = minor(s.st_rdev);
major() and minor() are non-portable; this code may need to be
conditionally compiled for non-Linux platforms. See how cgroup does
conditional compilation before accessing major and minor numbers.
> +
> + /* weight */
> +
> + temp = p + 1;
> + if (!temp)
> + goto fail;
> +
> + p = temp;
> + while (isdigit(*p)&& ++p);
s/isdigit/c_isdigit/
> + if (!p || (*p != ';'&& *p != '\0'))
> + goto fail;
> +
> + result[i].weight = atoi(temp);
No atoi. Use virStrToLong_i or similar.
> static int qemuDomainSetBlkioParameters(virDomainPtr dom,
> virTypedParameterPtr params,
> int nparams,
> @@ -5860,10 +5951,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
> ret = 0;
> if (flags& VIR_DOMAIN_AFFECT_LIVE) {
You forgot to update the virCheckFlags to allow the new
VIR_DOMAIN_TYPED_STRING_OKAY flag. (Actually, I'm starting to think it
might be better to name that flag VIR_TYPED_PARAM_STRING_OKAY, since it
only makes sense with any API that uses virTypedParameter.
> for (i = 0; i< nparams; i++) {
> + int rc;
> virTypedParameterPtr param =¶ms[i];
>
> if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) {
> - int rc;
> if (param->type != VIR_TYPED_PARAM_UINT) {
> qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> _("invalid type for blkio weight tunable, expected a 'unsigned int'"));
> @@ -5884,6 +5975,44 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
> _("unable to set blkio weight tunable"));
> ret = -1;
> }
> + } else if(STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) {
> + int ndevices;
> + virBlkioWeightDevicePtr tmp = NULL;
> + if (param->type != VIR_TYPED_PARAM_STRING) {
Here, you should also fail if flags does not include the OKAY flag, as
that indicates a client that isn't properly using the API.
> + for (i = 0; i< ndevices; i++) {
> + char *weight_device = NULL;
> +
> + virAsprintf(&weight_device, "%d:%d %d",
> + tmp[i].major,
> + tmp[i].minor,
> + tmp[i].weight);
> + if (weight_device) {
> + rc = virCgroupSetBlkioWeightDevice(group, weight_device);
> + VIR_FREE(weight_device);
> + weight_device = NULL;
> + if (rc != 0) {
> + virReportSystemError(-rc, "%s",
> + _("unable to set blkio weight_device tunable"));
> + ret = -1;
> + break;
> + }
> + }
You should report OOM errors if allocating weight_device failed, rather
than silently ignoring them.
> @@ -6012,6 +6160,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,
>
> if (flags& VIR_DOMAIN_AFFECT_LIVE) {
> for (i = 0; i< *nparams; i++) {
> + char *weight_device;
> virTypedParameterPtr param =¶ms[i];
> val = 0;
> param->value.ui = 0;
If someone calls GetBlkioParameters with a size of 0 but without the
_OKAY flag, to learn how large to allocate the array, then you need to
return 1, not 2. I don't see that in your patch (I only see where if
they allocate 2, but didn't pass _OKAY, that you don't populate the
second entry).
> +++ b/tools/virsh.c
> @@ -4616,6 +4616,8 @@ static const vshCmdOptDef opts_blkiotune[] = {
> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> {"weight", VSH_OT_INT, VSH_OFLAG_NONE,
> N_("IO Weight in range [100, 1000]")},
> + {"weight-device", VSH_OT_STRING, VSH_OFLAG_NONE,
> + N_("per device IO Weight, in the form of major:minor,weight")},
s/per device/per-device/
Do not expose major:minor,weight through virsh. Rather, you should
match the xml, and expose this via /path/to/device,weight (you got it
right in virsh.pod).
>
> + flags |= VIR_DOMAIN_TYPED_STRING_OKAY;
> +
> if (!vshConnectionUsability(ctl, ctl->conn))
> return false;
>
> @@ -4670,12 +4675,28 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> }
> }
>
> + rv = vshCommandOptString(cmd, "weight-device",&weight_device);
> + if (rv< 0) {
> + vshError(ctl, "%s",
> + _("Unable to parse string parameter"));
> + goto cleanup;
> + }
> + if (rv> 0) {
> + nparams++;
> + }
> +
> if (nparams == 0) {
> /* get the number of blkio parameters */
> + /* old libvirtd doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, we
> + * give it a second try with this flag disabled in the case of an
> + * old libvirtd. */
Won't work as written. nparams will be -1, not 0, if the old server
didn't understand the _OKAY flag. Also, you should check
last_error->code, and only try the fallback if the error is
VIR_ERR_INVALID_ARG (any other error is fatal without having to try the
fallback).
> if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) {
> - vshError(ctl, "%s",
> - _("Unable to get number of blkio parameters"));
> - goto cleanup;
> + flags&= ~VIR_DOMAIN_TYPED_STRING_OKAY;
> + if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) {
> + vshError(ctl, "%s",
> + _("Unable to get number of blkio parameters"));
> + goto cleanup;
> + }
> }
>
> if (nparams == 0) {
> @@ -4717,6 +4738,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> vshPrint(ctl, "%-15s: %d\n", params[i].field,
> params[i].value.b);
> break;
> + case VIR_TYPED_PARAM_STRING:
> + vshPrint(ctl, "%-15s: %s\n", params[i].field,
> + params[i].value.s);
> + break;
> default:
> vshPrint(ctl, "unimplemented blkio parameter type\n");
> }
> @@ -4737,11 +4762,24 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> sizeof(temp->field));
> weight = 0;
> }
> +
> + if (weight_device) {
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + virBufferAsprintf(&buf, "%s", weight_device);
> + temp->value.s = virBufferContentAndReset(&buf);
Why not just: temp->value.s = weight_device, instead of going through a
virBuffer?
> + temp->type = VIR_TYPED_PARAM_STRING;
> + strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE,
> + sizeof(temp->field));
> + }
> + }
> + ret = true;
> + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) {
You need to fix the logic to pass _OKAY if weight_device was specified.
Also, I think your logic is not quite right in that you pre-set flags
to include _OKAY even if you are not passing weight_device, which will
cause needless problems when talking to an older server if you aren't
even going to be changing device weight.
> Display or set the blkio parameters. QEMU/KVM supports I<--weight>.
> I<--weight> is in range [100, 1000].
>
> +B<weight-device> is in the format of /device/patch,weight;/device/patch,weight.
s/patch/path/g
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list