From: Eric Blake&lt;eblake(a)redhat.com&gt; This allows strings to be transported between client and server in the context of name-type-value virTypedParameter functions. For compatibility, o new clients will not send strings to old servers, based on a feature check o new servers will not send strings to old clients without the flag VIR_TYPED_PARAM_STRING_OKAY; this will be enforced at the RPC layer in the next patch, so that drivers need not worry about it in general. The one exception is that virDomainGetSchedulerParameters lacks a flags argument, so it must not return a string; drivers that forward that function on to virDomainGetSchedulerParametersFlags will have to pay attention to the flag. o the flag VIR_TYPED_PARAM_STRING_OKAY is set automatically, based on a feature check (so far, no driver implements it), so clients do not have to worry about it Future patches can then enable the feature on a per-driver basis. This patch also ensures that drivers can blindly strdup() field names (previously, a malicious client could stuff 80 non-NUL bytes into field and cause a read overrun). * src/libvirt_internal.h (VIR_DRV_FEATURE_TYPED_PARAM_STRING): New driver feature. * src/libvirt.c (virTypedParameterValidateSet) (virTypedParameterSanitizeGet): New helper functions. (virDomainSetMemoryParameters, virDomainSetBlkioParameters) (virDomainSetSchedulerParameters) (virDomainSetSchedulerParametersFlags) (virDomainGetMemoryParameters, virDomainGetBlkioParameters) (virDomainGetSchedulerParameters) (virDomainGetSchedulerParametersFlags, virDomainBlockStatsFlags): Use them. * src/util/util.h (virTypedParameterArrayClear): New helper function. * src/util/util.c (virTypedParameterArrayClear): Implement it. * src/libvirt_private.syms (util.h): Export it. Based on an initial patch by Hu Tao, with feedback from Daniel P. Berrange. Signed-off-by: Eric Blake&lt;eblake(a)redhat.com&gt; --- include/libvirt/libvirt.h.in | 32 ++++++++++++++- src/libvirt.c | 92 ++++++++++++++++++++++++++++++++++++------ src/libvirt_internal.h | 9 +++- src/libvirt_private.syms | 1 + src/util/util.c | 14 ++++++ src/util/util.h | 2 + 6 files changed, 134 insertions(+), 16 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa320b6..2ab89f5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -200,11 +200,14 @@ typedef virDomainControlInfo *virDomainControlInfoPtr; * current domain state. VIR_DOMAIN_AFFECT_LIVE requires a running * domain, and VIR_DOMAIN_AFFECT_CONFIG requires a persistent domain * (whether or not it is running). + * + * These enums should not conflict with those of virTypedParameterFlags. */ typedef enum { VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. */ VIR_DOMAIN_AFFECT_LIVE = 1<< 0, /* Affect running domain state. */ VIR_DOMAIN_AFFECT_CONFIG = 1<< 1, /* Affect persistent domain state. */ + /* 1<< 2 is reserved for virTypedParameterFlags */ } virDomainModificationImpact; /** @@ -489,10 +492,36 @@ typedef enum { VIR_TYPED_PARAM_LLONG = 3, /* long long case */ VIR_TYPED_PARAM_ULLONG = 4, /* unsigned long long case */ VIR_TYPED_PARAM_DOUBLE = 5, /* double case */ - VIR_TYPED_PARAM_BOOLEAN = 6 /* boolean(character) case */ + VIR_TYPED_PARAM_BOOLEAN = 6, /* boolean(character) case */ + VIR_TYPED_PARAM_STRING = 7, /* string case */ } virTypedParameterType; /** + * virTypedParameterFlags: + * + * Flags related to libvirt APIs that use virTypedParameter. + * + * These enums should not conflict with those of virDomainModificationImpact. + */ +typedef enum { + /* 1<< 0 is reserved for virDomainModificationImpact */ + /* 1<< 1 is reserved for virDomainModificationImpact */ + + /* Older servers lacked the ability to handle string typed + * parameters. Attempts to set a string parameter with an older + * server will fail at the client, but attempts to retrieve + * parameters must not return strings from a new server to an + * older client, so this flag exists to identify newer clients to + * newer servers. This flag is automatically set when needed, so + * the user does not have to worry about it; however, manually + * setting the flag can be used to reject servers that cannot + * return typed strings, even if no strings would be returned. + */ + VIR_TYPED_PARAM_STRING_OKAY = 1<< 2, + +} virTypedParameterFlags; + +/** * VIR_TYPED_PARAM_FIELD_LENGTH: * * Macro providing the field length of virTypedParameter name @@ -520,6 +549,7 @@ struct _virTypedParameter { unsigned long long int ul; /* type is ULLONG */ double d; /* type is DOUBLE */ char b; /* type is BOOLEAN */ + char *s; /* type is STRING, may not be NULL */ } value; /* parameter value */ }; diff --git a/src/libvirt.c b/src/libvirt.c index b0d1e01..1518ed2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3583,6 +3583,50 @@ error: return -1; } +/* Helper function called to validate incoming client array on any + * interface that sets typed parameters in the hypervisor. */ +static int +virTypedParameterValidateSet(virDomainPtr domain, + virTypedParameterPtr params, + int nparams) +{ + bool string_okay; + int i; + + string_okay = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, + domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING); + for (i = 0; i< nparams; i++) { + if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) == + VIR_TYPED_PARAM_FIELD_LENGTH) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("string parameter name '%.*s' too long"), + VIR_TYPED_PARAM_FIELD_LENGTH, + params[i].field); + virDispatchError(NULL); + return -1; + } + if (params[i].type == VIR_TYPED_PARAM_STRING) { + if (string_okay) { + if (!params[i].value.s) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("NULL string parameter '%s'"), + params[i].field); + virDispatchError(NULL); + return -1; + } + } else { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("string parameter '%s' unsupported"), + params[i].field); + virDispatchError(NULL); + return -1; + } + } + } + return 0; +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3621,6 +3665,9 @@ virDomainSetMemoryParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (virTypedParameterValidateSet(domain, params, nparams)< 0) + return -1; + conn = domain->conn; if (conn->driver->domainSetMemoryParameters) { @@ -3644,7 +3691,7 @@ error: * @params: pointer to memory parameter object * (return value, allocated by the caller) * @nparams: pointer to number of memory parameters; input and output - * @flags: one of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get all memory parameters. On input, @nparams gives the size of the * @params array; on output, @nparams gives how many slots were filled @@ -3695,6 +3742,9 @@ virDomainGetMemoryParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = domain->conn; if (conn->driver->domainGetMemoryParameters) { @@ -3717,7 +3767,7 @@ error: * @params: pointer to blkio parameter objects * @nparams: number of blkio parameters (this value can be the same or * less than the number of parameters supported) - * @flags: an OR'ed set of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact * * Change all or a subset of the blkio tunables. * This function may require privileged access to the hypervisor. @@ -3749,6 +3799,9 @@ virDomainSetBlkioParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (virTypedParameterValidateSet(domain, params, nparams)< 0) + return -1; + conn = domain->conn; if (conn->driver->domainSetBlkioParameters) { @@ -3772,7 +3825,7 @@ error: * @params: pointer to blkio parameter object * (return value, allocated by the caller) * @nparams: pointer to number of blkio parameters; input and output - * @flags: an OR'ed set of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get all blkio parameters. On input, @nparams gives the size of the * @params array; on output, @nparams gives how many slots were filled @@ -3814,6 +3867,9 @@ virDomainGetBlkioParameters(virDomainPtr domain, virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = domain->conn; if (conn->driver->domainGetBlkioParameters) { @@ -6410,7 +6466,7 @@ error: * @nparams: pointer to number of scheduler parameter * (this value should be same than the returned value * nparams of virDomainGetSchedulerType()); input and output - * @flags: one of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get all scheduler parameters. On input, @nparams gives the size of the * @params array; on output, @nparams gives how many slots were filled @@ -6456,6 +6512,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = domain->conn; if (conn->driver->domainGetSchedulerParametersFlags) { @@ -6505,15 +6564,17 @@ virDomainSetSchedulerParameters(virDomainPtr domain, return -1; } + if (domain->conn->flags& VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } if (params == NULL || nparams< 0) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (virTypedParameterValidateSet(domain, params, nparams)< 0) + return -1; - if (domain->conn->flags& VIR_CONNECT_RO) { - virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); - goto error; - } conn = domain->conn; if (conn->driver->domainSetSchedulerParameters) { @@ -6568,15 +6629,17 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, return -1; } + if (domain->conn->flags& VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } if (params == NULL || nparams< 0) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (virTypedParameterValidateSet(domain, params, nparams)< 0) + return -1; - if (domain->conn->flags& VIR_CONNECT_RO) { - virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); - goto error; - } conn = domain->conn; if (conn->driver->domainSetSchedulerParametersFlags) { @@ -6665,7 +6728,7 @@ error: * @params: pointer to block stats parameter object * (return value) * @nparams: pointer to number of block stats; input and output - * @flags: unused, always pass 0 + * @flags: bitwise-OR of virTypedParameterFlags * * This function is to get block stats parameters for block * devices attached to the domain. @@ -6715,6 +6778,9 @@ int virDomainBlockStatsFlags(virDomainPtr dom, virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } + if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; conn = dom->conn; if (conn->driver->domainBlockStatsFlags) { diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h index 0117c5b..2550d76 100644 --- a/src/libvirt_internal.h +++ b/src/libvirt_internal.h @@ -1,7 +1,7 @@ /* * libvirt.h: publically exported APIs, not for public use * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2008, 2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -83,7 +83,12 @@ enum { /* * Support for file descriptor passing */ - VIR_DRV_FEATURE_FD_PASSING = 8 + VIR_DRV_FEATURE_FD_PASSING = 8, + + /* + * Support for VIR_TYPED_PARAM_STRING + */ + VIR_DRV_FEATURE_TYPED_PARAM_STRING = 9, }; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6a1562e..2185294 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1162,6 +1162,7 @@ virStrncpy; virTimeMs; virTimestamp; virTrimSpaces; +virTypedParameterArrayClear; virVasprintf; diff --git a/src/util/util.c b/src/util/util.c index 959c224..9ecfa9d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2607,3 +2607,17 @@ or other application using the libvirt API.\n\ return 0; } + +void +virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) +{ + int i; + + if (!params) + return; + + for (i = 0; i< nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s); + } +} diff --git a/src/util/util.h b/src/util/util.h index d8176a8..3295ce8 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -258,4 +258,6 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); int virEmitXMLWarning(int fd, const char *name, const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); #endif /* __VIR_UTIL_H__ */

This adds per-device weights to<blkiotune>. Note that the cgroups implementation only supports weights per block device, and not per-file within the device; hence this option must be global to the domain definition rather than tied to individual <device>/<disk> entries: <domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ... This patch also adds a parameter --device-weight to virsh command blkiotune for setting/getting blkiotune.weight_device for any hypervisor that supports it. All<device> entries under <blkiotune> are concatenated into a single string attribute under virDomain{Get,Set}BlkioParameters, named "device_weight". Signed-off-by: Hu Tao&lt;hutao(a)cn.fujitsu.com&gt; Signed-off-by: Eric Blake&lt;eblake(a)redhat.com&gt; --- docs/formatdomain.html.in | 29 ++++++++- docs/schemas/domaincommon.rng | 12 ++++ include/libvirt/libvirt.h.in | 10 +++ src/conf/domain_conf.c | 138 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 17 +++++ src/libvirt_private.syms | 2 + tools/virsh.c | 44 ++++++++++++-- tools/virsh.pod | 6 ++- 8 files changed, 246 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..99c5add 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -505,6 +505,14 @@ ... &lt;blkiotune&gt; &lt;weight&gt;800&lt;/weight&gt; +&lt;device&gt; +&lt;path&gt;/dev/sda&lt;/path&gt; +&lt;weight&gt;1000&lt;/weight&gt; +&lt;/device&gt; +&lt;device&gt; +&lt;path&gt;/dev/sdb&lt;/path&gt; +&lt;weight&gt;500&lt;/weight&gt; +&lt;/device&gt; &lt;/blkiotune&gt; ... &lt;/domain&gt; @@ -514,10 +522,25 @@ <dt><code>blkiotune</code></dt> <dd> The optional<code>blkiotune</code> element provides the ability to tune Blkio cgroup tunable parameters for the domain. If this is - omitted, it defaults to the OS provided defaults.</dd> + omitted, it defaults to the OS provided + defaults.<span class="since">Since 0.8.8</span></dd> <dt><code>weight</code></dt> -<dd> The optional<code>weight</code> element is the I/O weight of the - guest. The value should be in range [100, 1000].</dd> +<dd> The optional<code>weight</code> element is the overall I/O + weight of the guest. The value should be in the range [100, + 1000].</dd> +<dt><code>device</code></dt> +<dd>The domain may have multiple<code>device</code> elements + that further tune the weights for each host block device in + use by the domain. Note that + multiple<a href="#elementsDisks">guest disks</a> can share a + single host block device, if they are backed by files within + the same host file system, which is why this tuning parameter + is at the global domain level rather than associated with each + guest disk device. Each<code>device</code> element has two + mandatory sub-elements,<code>path</code> describing the + absolute path of the device, and<code>weight</code> giving + the relative weight of that device, in the range [100, + 1000].<span class="since">Since 0.9.8</span></dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..347af1a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -328,6 +328,18 @@ <ref name="weight"/> </element> </optional> +<zeroOrMore> +<element name="device"> +<interleave> +<element name="path"> +<ref name="absFilePath"/> +</element> +<element name="weight"> +<ref name="weight"/> +</element> +</interleave> +</element> +</zeroOrMore> </element> </optional> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2ab89f5..0fd9302 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1236,6 +1236,16 @@ char * virDomainGetSchedulerType(virDomainPtr domain, #define VIR_DOMAIN_BLKIO_WEIGHT "weight" +/** + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight, as a string. The string is parsed as a + * series of /path/to/device,weight elements, separated by ';'. + */ + +#define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight" + /* 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 a85f837..f0b324d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -603,6 +603,104 @@ VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE + +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices) +{ + int i; + + for (i = 0; i< ndevices; i++) + VIR_FREE(deviceWeights[i].path); +} + +/** + * virBlkioDeviceWeightToStr: + * + * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of per-device weights. + */ +#if defined(major)&& defined(minor) +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i< ndevices; i++) { + if (stat(devices[i].path,&s) == -1) + return -1; + if ((s.st_mode& S_IFMT) != S_IFBLK) + return -1; + virBufferAsprintf(&buf, "%d:%d %d\n", + major(s.st_rdev), + minor(s.st_rdev), + devices[i].weight); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } + + *result = virBufferContentAndReset(&buf); + return 0; +} +#else +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1; +} +#endif + +/** + * virDomainBlkioDeviceWeightParseXML + * + * this function parses a XML node: + * + *<device> + *<path>/fully/qualified/device/path</path> + *<weight>weight</weight> + *</device> + * + * and fills a virBlkioDeviceWeight struct. + */ +static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) +{ + char *c; + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + dw->path = (char *)xmlNodeGetContent(node); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + if (virStrToLong_ui(c, NULL, 10,&dw->weight)< 0) { + VIR_FREE(c); + VIR_FREE(dw->path); + return -1; + } + VIR_FREE(c); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1272,6 +1370,10 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->emulator); VIR_FREE(def->description); + virBlkioDeviceWeightArrayClear(def->blkio.devices, + def->blkio.ndevices); + VIR_FREE(def->blkio.devices); + virDomainWatchdogDefFree(def->watchdog); virDomainMemballoonDefFree(def->memballoon); @@ -6711,6 +6813,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight)< 0) def->blkio.weight = 0; + n = virXPathNodeSet("./blkiotune/device", ctxt,&nodes); + if (n> 0) { + if (VIR_ALLOC_N(def->blkio.devices, n)< 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i< n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], +&def->blkio.devices[i])< 0) { + virBlkioDeviceWeightArrayClear(def->blkio.devices, i); + goto error; + } + } + def->blkio.ndevices = n; + VIR_FREE(nodes); + } + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit)< 0) @@ -10849,10 +10969,22 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon); /* add blkiotune only if there are any */ - if (def->blkio.weight) { + if (def->blkio.weight || def->blkio.devices) { virBufferAddLit(buf, "<blkiotune>\n"); - virBufferAsprintf(buf, "<weight>%u</weight>\n", - def->blkio.weight); + + if (def->blkio.weight) + virBufferAsprintf(buf, "<weight>%u</weight>\n", + def->blkio.weight); + + for (n = 0; n< def->blkio.ndevices; n++) { + virBufferAddLit(buf, "<device>\n"); + virBufferEscapeString(buf, "<path>%s</path>\n", + def->blkio.devices[n].path); + virBufferAsprintf(buf, "<weight>%u</weight>\n", + def->blkio.devices[n].weight); + virBufferAddLit(buf, "</device>\n"); + } + virBufferAddLit(buf, "</blkiotune>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5ebb441..7db2a11 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1361,6 +1361,20 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ }; +typedef struct _virBlkioDeviceWeight virBlkioDeviceWeight; +typedef virBlkioDeviceWeight *virBlkioDeviceWeightPtr; +struct _virBlkioDeviceWeight { + char *path; + unsigned int weight; +}; + +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices); +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1378,6 +1392,9 @@ struct _virDomainDef { struct { unsigned int weight; + + size_t ndevices; + virBlkioDeviceWeightPtr devices; } blkio; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2185294..f7d0fb2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,8 @@ virDomainAuditVcpu; # domain_conf.h +virBlkioDeviceWeightArrayClear; +virBlkioDeviceWeightToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/tools/virsh.c b/tools/virsh.c index 5544a41..fe87e9b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4648,6 +4648,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]")}, + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weights, in the form of /path/to/device,weight;...")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -4658,6 +4660,7 @@ static bool cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; + const char *device_weight = NULL; int weight = 0; int nparams = 0; int rv = 0; @@ -4702,6 +4705,16 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } } + rv = vshCommandOptString(cmd, "device-weight",&device_weight); + 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 */ if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) { @@ -4749,6 +4762,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"); } @@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight; + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - else - ret = true; + ret = false; + } } cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 775d302..a117e84 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1032,12 +1032,16 @@ value are kilobytes (i.e. blocks of 1024 bytes). Specifying -1 as a value for these limits is interpreted as unlimited. -=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] [[I<--config>] +=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] +[I<--device-weight> B<device-weights>] [[I<--config>] [I<--live>] | [I<--current>]] Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000]. +B<device-weights> is a single string listing one or more device/weight +pairs, in the format of /path/to/device,weight;/path/to/device,weight. + If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state.

On 11/08/2011 06:00 AM, Hu Tao wrote: > Implement setting/getting per-device blkio weights in qemu, > using the cgroups blkio.weight_device tunable. > > --- > src/libvirt_private.syms | 1 + > src/qemu/qemu_cgroup.c | 22 +++++ > src/qemu/qemu_driver.c | 216 > ++++++++++++++++++++++++++++++++++++++++++++- > src/util/cgroup.c | 20 ++++ > src/util/cgroup.h | 3 + > 5 files changed, 257 insertions(+), 5 deletions(-)

> --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver, > } > } > > + if (qemuCgroupControllerActive(driver, > VIR_CGROUP_CONTROLLER_BLKIO)) { > + char *tmp; > + if (virBlkioDeviceWeightToStr(vm->def->blkio.devices, > + vm->def->blkio.ndevices, > +&tmp)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to set io device weight for > domain %s"), > + vm->def->name); The ToStr function doesn't report an OOM error. So this is ok. I wonder whether that function should report an OOM error, though? Eric?

> + goto cleanup; > + } > + if (tmp) { I think you can remove the if branch.

> @@ -5978,6 +6058,53 @@ static int > qemuDomainSetBlkioParameters(virDomainPtr dom, > _("unable to set blkio > weight tunable")); > ret = -1; > } > + } else if (STREQ(param->field, > VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { > + int ndevices; > + virBlkioDeviceWeightPtr devices = NULL; > + char *tmp; > + if (param->type != VIR_TYPED_PARAM_STRING) { > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("invalid type for device_weight > tunable, expected a 'char *'")); > + ret = -1; > + continue; > + } > + > + if (parseBlkioWeightDeviceStr(params[i].value.s, > +&devices, > +&ndevices)< 0) { > + ret = -1; > + continue; > + } > + if (virBlkioDeviceWeightToStr(devices, > + ndevices, > +&tmp)< 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to set blkio > weight_device tunable")); > + virBlkioDeviceWeightArrayClear(devices, ndevices); > + VIR_FREE(devices); > + ret = -1; > + continue; > + } > + if (tmp) {

> ACK with those nits fixed.