[libvirt] [PATCHv6 4/5] blkiotune: add interface for blkiotune.device_weight
Stefan Berger
stefanb at linux.vnet.ibm.com
Fri Nov 4 00:33:35 UTC 2011
On 11/03/2011 02:06 PM, Eric Blake wrote:
> 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".
> ---
>
> v5: did not include this patch
> v6: split v4 2/2 into two parts; this part covers just the libvirt.h
> and virsh changes. Rename public-facing terminology to be
> "device_weight", not "weight_device", since we are talking about
> per-device weights and may later add other per-device attributes
> (no need to propagate cgroups' stupid naming to the user). Rebase
> to fit in with VIR_TYPED_PARAM_STRING tweaks earlier in series.
>
> Should we use just commas for field separation, rather than commas
> between device and weight but semicolon between device-weight pairs?
> It would make it much easier to do:
> virsh blkiotune dom --device-weight /dev/sda,500,/dev/sdb,500
> instead of requiring shell quoting:
> virsh blkiotune dom --device-weight '/dev/sda,500;/dev/sdb,500'
Either looks fine to me, but the first is likely better for non-shell
savvy users...
> docs/formatdomain.html.in | 29 ++++++++-
> docs/schemas/domaincommon.rng | 12 ++++
> include/libvirt/libvirt.h.in | 10 +++
> src/conf/domain_conf.c | 130 ++++++++++++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 17 +++++
> src/libvirt_private.syms | 2 +
> tools/virsh.c | 32 +++++++++-
> tools/virsh.pod | 6 ++-
> 8 files changed, 228 insertions(+), 10 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index cbad196..e848b7d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -505,6 +505,14 @@
> ...
> <blkiotune>
> <weight>800</weight>
> +<device>
> +<path>/dev/sda</path>
> +<weight>1000</weight>
> +</device>
> +<device>
> +<path>/dev/sdb</path>
> +<weight>500</weight>
> +</device>
> </blkiotune>
> ...
> </domain>
> @@ -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.7</span></dd>
> </dl>
>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index b6f858e..6f96fe2 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">
> +<data type="int"/>
'int' or 'unsigned int' ? If there's a definite valid range beyond
'should be [100,1000]', have a data type like
<data type="int">
<param name="minInclusive">100</param>
<param name="maxInclusive">1000</param>
</data>
> +</element>
> +</interleave>
> +</element>
> +</zeroOrMore>
> </element>
> </optional>
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index e10a72b..5ae7d10 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 238edfd..e5b5f69 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -603,6 +603,99 @@ 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_i(c, NULL, 10,&dw->weight)< 0)
can the weight be negative? if not use virStrToLong_ui() ?
VIR_FREE(c);
maybe also VIR_FREE(dw->path) if the entry is completely assumed
completely wrong
> + return -1;
> + VIR_FREE(c);
> + }
> + }
> + node = node->next;
> + }
> +
> + return 0;
> +}
> +
> +
> +
> static void
> virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
> {
> @@ -1272,6 +1365,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);
> @@ -6710,6 +6807,21 @@ 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++) {
> + virDomainBlkioDeviceWeightParseXML(nodes[i],
> +&def->blkio.devices[i]);
Check return code here and report error upon parser error ?
> + }
> + def->blkio.ndevices = n;
> + VIR_FREE(nodes);
> + }
> +
> /* Extract other memory tunables */
> if (virXPathULong("string(./memtune/hard_limit)", ctxt,
> &def->mem.hard_limit)< 0)
> @@ -10848,10 +10960,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);
this weight type is an unsigned int
> +
> + 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>%d</weight>\n",
> + def->blkio.devices[n].weight);
whereas this weight is signed? (thought this was only possible in space :-))
> + virBufferAddLit(buf, "</device>\n");
> + }
> +
> virBufferAddLit(buf, "</blkiotune>\n");
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5ebb441..577d10d 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;
> + int weight;
unsigned int ?
> +};
> +
> +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..ca8db70 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");
> }
> @@ -4769,11 +4786,20 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> sizeof(temp->field));
> weight = 0;
> }
> +
> + if (device_weight) {
> + /* Safe to cast away const here. */
> + temp->value.s = (char *)device_weight;
> + temp->type = VIR_TYPED_PARAM_STRING;
> + strncpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT,
> + sizeof(temp->field));
Use virStrcpy()
> + }
> }
> - 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.
More information about the libvir-list
mailing list