After the previous patch, there are now some redundant checks. * src/qemu/qemu_driver.c (qemudDomainGetVcpuPinInfo) (qemuGetSchedulerParametersFlags): Drop checks now guaranteed by libvirt.c. * src/lxc/lxc_driver.c (lxcGetSchedulerParametersFlags): Likewise. ---

...

Drivers were inconsistent when presented both --live and --config at once.

...

* src/libvirt.c (virDomainGetMemoryParameters) (virDomainGetBlkioParameters) (virDomainGetSchedulerParametersFlags, virDomainGetVcpuPinInfo): Borrow sanity checking from virDomainGetVcpusFlags.

ACK

I'm now pushing 1/4, but in the process of my final pre-push review, I noticed that this would be a worthwhile followup patch. src/lxc/lxc_driver.c | 7 ------- src/qemu/qemu_driver.c | 14 -------------- 2 files changed, 0 insertions(+), 21 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c02fe70..8bd1501 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2959,13 +2959,6 @@ lxcGetSchedulerParametersFlags(virDomainPtr dom, lxcDriverLock(driver); - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == - (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { - lxcError(VIR_ERR_INVALID_ARG, "%s", - _("cannot query live and config together")); - goto cleanup; - } - if (*nparams > 1) { rc = lxcGetCpuBWStatus(driver->cgroup); if (rc < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3be97f3..307cc37 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3574,13 +3574,6 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == - (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("cannot get live and persistent info concurrently")); - goto cleanup; - } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -6902,13 +6895,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, /* We don't return strings, and thus trivially support this flag. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == - (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("cannot query live and config together")); - goto cleanup; - } - if (*nparams > 1) { rc = qemuGetCpuBWStatus(driver->cgroup); if (rc < 0) -- 1.7.7.3

On 11/29/2011 09:26 AM, Daniel P. Berrange wrote:

On Mon, Nov 14, 2011 at 09:30:00PM -0700, Eric Blake wrote:

...

Without this, 'virsh blkiotune --live --config --weight=n' only affected live.

* src/qemu/qemu_driver.c (qemuDomainSetBlkioParameters): Allow setting both configurations at once. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f4a18d..b0ce115 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5981,7 +5981,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = -1; } } - } else if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + } + if (ret < 0) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Clang can't see that if we get here, persistentDef was set. */ sa_assert(persistentDef);

ACK

Thanks for the reviews. I've now pushed 1, 1.5, and 2. It looks like Hu's suggestions to fix 3 and 4 to store 0 values in the DefPtr, but omit it from the xml output, work without much further effort on my part, and that both Hu and my contributions have all been acked by at least one other reviewer, so I will finish my testing that I didn't break anything when rebasing to merge Hu's touchups into my v4 patches, then push those as well. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Nov 14, 2011 at 09:30:01PM -0700, Eric Blake wrote:

From: Hu Tao <hutao@cn.fujitsu.com>

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 <devices>/<disk> entries:

<domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ..

This patch also adds a parameter --device-weights 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 <hutao@cn.fujitsu.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 29 +++++- docs/schemas/domaincommon.rng | 26 ++++- include/libvirt/libvirt.h.in | 10 ++ src/conf/domain_conf.c | 99 +++++++++++++++++++- src/conf/domain_conf.h | 14 +++ src/libvirt_private.syms | 1 + .../qemuxml2argv-blkiotune-device.xml | 36 +++++++ tools/virsh.c | 43 +++++++-- tools/virsh.pod | 8 ++- 9 files changed, 245 insertions(+), 21 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.xml

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 @@ ... <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.8</span></dd> </dl>

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..f776a51 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -322,12 +322,26 @@ <!-- The Blkio cgroup related tunables would go in the blkiotune --> <optional> <element name="blkiotune"> - <!-- I/O weight the VM can use --> - <optional> - <element name="weight"> - <ref name="weight"/> - </element> - </optional> + <interleave> + <!-- I/O weight the VM can use --> + <optional> + <element name="weight"> + <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> + </interleave> </element> </optional>

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2ab89f5..ff4f51b 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 58f4d0f..b35c83c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -603,6 +603,67 @@ 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); +} + +/** + * 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; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path") && !dw->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) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse weight %s"), + c); + VIR_FREE(c); + VIR_FREE(dw->path); + return -1; + } + VIR_FREE(c); + } + } + node = node->next; + } + if (!dw->path) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing per-device path")); + return -1; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1272,6 +1333,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 +6776,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight) < 0) def->blkio.weight = 0;

+ if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract blkiotune nodes")); + goto error; + } + if (n && VIR_ALLOC_N(def->blkio.devices, n) < 0) + goto no_memory; + + for (i = 0; i < n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], + &def->blkio.devices[i]) < 0) + goto error; + def->blkio.ndevices++; + } + VIR_FREE(nodes); + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit) < 0) @@ -10849,10 +10930,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"); }

We can filter out 0-weight here: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b35c83c..5160003 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10893,6 +10893,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; int n, allones = 1; + int blkio = 0; virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | @@ -10930,7 +10931,15 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon); /* add blkiotune only if there are any */ - if (def->blkio.weight || def->blkio.devices) { + + if (def->blkio.weight) + blkio = 1; + for (n = 0; n < def->blkio.ndevices; n++) { + if (def->blkio.devices[n].weight) + blkio = 1; + } + + if (blkio) { virBufferAddLit(buf, " <blkiotune>\n"); if (def->blkio.weight) @@ -10938,6 +10947,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->blkio.weight); for (n = 0; n < def->blkio.ndevices; n++) { + if (def->blkio.devices[n].weight == 0) + continue; virBufferAddLit(buf, " <device>\n"); virBufferEscapeString(buf, " <path>%s</path>\n", def->blkio.devices[n].path);

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a3cb834..d3c6381 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1361,6 +1361,17 @@ 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); + + /* * Guest VM main configuration * @@ -1378,6 +1389,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 b15a8db..d78fd53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,7 @@ virDomainAuditVcpu;

# domain_conf.h +virBlkioDeviceWeightArrayClear; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.xml new file mode 100644 index 0000000..3412753 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <currentMemory>219136</currentMemory> + <blkiotune> + <weight>800</weight> + <device> + <path>/dev/sda</path> + <weight>400</weight> + </device> + <device> + <path>/dev/sdb</path> + <weight>900</weight> + </device> + </blkiotune> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tools/virsh.c b/tools/virsh.c index 83dc3c7..a1d2afd 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-weights", 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-weights", &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,12 +4762,14 @@ 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"); } } - - ret = true; } else { /* set the blkio parameters */ params = vshCalloc(ctl, nparams, sizeof(*params)); @@ -4765,18 +4780,30 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)

if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); - weight = 0; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) + goto cleanup; + } + + if (device_weight) { + temp->value.s = vshStrdup(ctl, device_weight); + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) + goto cleanup; } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + + if (virDomainSetBlkioParameters(dom, params, nparams, flags) < 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - else - ret = true; + goto cleanup; + } }

+ ret = true; + cleanup: + virTypedParameterArrayClear(params, nparams); VIR_FREE(params); virDomainFree(dom); return ret; diff --git a/tools/virsh.pod b/tools/virsh.pod index 775d302..ef024c5 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1032,12 +1032,18 @@ 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-weights> 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. +Each weight is in the range [100, 1000], or the value 0 to remove that +device from per-device listings. + 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. -- 1.7.7.1

-- Thanks, Hu Tao

On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote:

From: Hu Tao <hutao@cn.fujitsu.com>

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 | 20 ++ src/qemu/qemu_driver.c | 218 +++++++++++++++++++- src/util/cgroup.c | 51 +++++ src/util/cgroup.h | 4 + .../qemuxml2argv-blkiotune-device.args | 4 + tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 8 files changed, 295 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.args

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d78fd53..3575fe0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -89,6 +89,7 @@ virCgroupKillRecursive; virCgroupMounted; virCgroupPathOfController; virCgroupRemove; +virCgroupSetBlkioDeviceWeight; virCgroupSetBlkioWeight; virCgroupSetCpuShares; virCgroupSetCpuCfsPeriod; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..eda4c66 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -312,6 +312,26 @@ int qemuSetupCgroup(struct qemud_driver *driver, } }

+ if (vm->def->blkio.ndevices) { + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + for (i = 0; i < vm->def->blkio.ndevices; i++) { + virBlkioDeviceWeightPtr dw = &vm->def->blkio.devices[i]; + rc = virCgroupSetBlkioDeviceWeight(cgroup, dw->path, + dw->weight); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to set io device weight " + "for domain %s"), + vm->def->name); + goto cleanup; + } + } + } else { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Block I/O tuning is not available on this host")); + } + } + if (vm->def->mem.hard_limit != 0 || vm->def->mem.soft_limit != 0 || vm->def->mem.swap_hard_limit != 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0ce115..43f4041 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,7 +112,7 @@ # define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ #endif

-#define QEMU_NB_BLKIO_PARAM 1 +#define QEMU_NB_BLKIO_PARAM 2

static void processWatchdogEvent(void *data, void *opaque);

@@ -5885,6 +5885,105 @@ cleanup: return ret; }

+/* deviceWeightStr 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(char *deviceWeightStr, + virBlkioDeviceWeightPtr *dw, int *size, + virCgroupPtr cgroup) +{ + char *temp; + int ndevices = 0; + int nsep = 0; + int i; + virBlkioDeviceWeightPtr result = NULL; + + temp = deviceWeightStr; + while (temp) { + temp = strchr(temp, ','); + if (temp) { + temp++; + nsep++; + } + } + + /* A valid string must have even number of fields, hence an odd + * number of commas. */ + if (!(nsep & 1)) + goto error; + + ndevices = (nsep + 1) / 2; + + if (VIR_ALLOC_N(result, ndevices) < 0) { + virReportOOMError(); + return -1; + } + + i = 0; + temp = deviceWeightStr; + while (temp) { + char *p = temp; + + /* device path */ + p = strchr(p, ','); + if (!p) + goto error; + + result[i].path = strndup(temp, p - temp); + if (!result[i].path) { + virReportOOMError(); + goto cleanup; + } + + /* weight */ + temp = p + 1; + + if (virStrToLong_ui(temp, &p, 10, &result[i].weight) < 0) + goto error; + + if (cgroup) { + int rc = virCgroupSetBlkioDeviceWeight(cgroup, + result[i].path, + result[i].weight);

Does more than the function name says. Would it be better to just do the parsing here, and set the cgroup values after parsing(see my comment to patch 3 for filtering out 0-weight when writing to xml): diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 43f4041..275d5c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5890,8 +5890,7 @@ cleanup: */ static int parseBlkioWeightDeviceStr(char *deviceWeightStr, - virBlkioDeviceWeightPtr *dw, int *size, - virCgroupPtr cgroup) + virBlkioDeviceWeightPtr *dw, int *size) { char *temp; int ndevices = 0; @@ -5942,24 +5941,8 @@ parseBlkioWeightDeviceStr(char *deviceWeightStr, if (virStrToLong_ui(temp, &p, 10, &result[i].weight) < 0) goto error; - if (cgroup) { - int rc = virCgroupSetBlkioDeviceWeight(cgroup, - result[i].path, - result[i].weight); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to set io device weight " - "for path %s"), - result[i].path); - goto cleanup; - } - } + i++; - /* 0-weight entries only affect cgroups, they don't stick in xml */ - if (result[i].weight) - i++; - else - VIR_FREE(result[i].path); if (*p == '\0') break; else if (*p != ',') @@ -6087,7 +6070,23 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, if (parseBlkioWeightDeviceStr(params[i].value.s, &devices, - &ndevices, group) < 0) { + &ndevices) < 0) { + ret = -1; + continue; + } + for (i = 0; i < ndevices; i++) { + rc = virCgroupSetBlkioDeviceWeight(group, + devices[i].path, + devices[i].weight); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to set io device weight " + "for path %s"), + devices[i].path); + break; + } + } + if (i != ndevices) { ret = -1; continue; } @@ -6140,7 +6139,7 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } if (parseBlkioWeightDeviceStr(params[i].value.s, &devices, - &ndevices, NULL) < 0) { + &ndevices) < 0) { ret = -1; continue; } -- Thanks, Hu Tao

On Tue, Nov 29, 2011 at 11:50:56AM -0700, Eric Blake wrote:

On 11/15/2011 12:22 AM, Hu Tao wrote:

...

On Mon, Nov 14, 2011 at 09:30:02PM -0700, Eric Blake wrote:

...

From: Hu Tao <hutao@cn.fujitsu.com>

Implement setting/getting per-device blkio weights in qemu, using the cgroups blkio.weight_device tunable.

Does more than the function name says. Would it be better to just do the parsing here, and set the cgroup values after parsing(see my comment to patch 3 for filtering out 0-weight when writing to xml):

ACK to your improvements for setting, but you forgot to do 0-weight filtering from the getting side:

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a8fea6c..105bdde 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -6270,9 +6270,14 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, case 1: /* blkiotune.device_weight */ if (vm->def->blkio.ndevices > 0) { virBuffer buf = VIR_BUFFER_INITIALIZER; + bool comma = false; for (j = 0; j < vm->def->blkio.ndevices; j++) { - if (j) + if (!vm->def->blkio.devices[j].weight) + continue; + if (comma) virBufferAddChar(&buf, ','); + else + comma = true; virBufferAsprintf(&buf, "%s,%u", vm->def->blkio.devices[j].path,

vm->def->blkio.devices[j].weight); @@ -6282,7 +6287,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } param->value.s = virBufferContentAndReset(&buf); - } else { + } + if (!param->value.s) { param->value.s = strdup(""); if (!param->value.s) { virReportOOMError();

Thanks. I also found we need to do filtering at other places. Since this is already pushed, here is another patch:

From b2009f582641b61c450744061ce34faa7a5f8eb7 Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Wed, 30 Nov 2011 10:06:19 +0800 Subject: [PATCH] filter blkio 0-device-weight at two oter places

filter 0-device-weight when: - getting blkio parameters with --config - starting up a domain When testing with blkio, I found these issues: (dom is down) virsh blkiotune dom --device-weights /dev/sda,300,/dev/sdb,500 virsh blkiotune dom --device-weights /dev/sda,300,/dev/sdb,0 virsh blkiotune dom weight : 800 device_weight : /dev/sda,200,/dev/sdb,0 # issue 1: shows 0 device weight of /dev/sdb that may confuse user (continued) virsh start dom # issue 2: If /dev/sdb doesn't exist, libvirt refuses to bring the # dom up because it wants to set the device weight to 0 of a # non-existing device. Since 0 means no weight-limit, we really don't # have to set it. --- src/qemu/qemu_cgroup.c | 2 ++ src/qemu/qemu_driver.c | 5 ++++- 2 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index e5ca8f3..d663798 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -317,6 +317,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { for (i = 0; i < vm->def->blkio.ndevices; i++) { virBlkioDeviceWeightPtr dw = &vm->def->blkio.devices[i]; + if (!dw->weight) + continue; rc = virCgroupSetBlkioDeviceWeight(cgroup, dw->path, dw->weight); if (rc != 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 81d11c2..6818449 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6370,6 +6370,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if (persistentDef->blkio.ndevices > 0) { virBuffer buf = VIR_BUFFER_INITIALIZER; for (j = 0; j < persistentDef->blkio.ndevices; j++) { + if (!persistentDef->blkio.devices[j].weight) + continue; if (j) virBufferAddChar(&buf, ','); virBufferAsprintf(&buf, "%s,%u", @@ -6381,7 +6383,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } param->value.s = virBufferContentAndReset(&buf); - } else { + } + if (!param->value.s) { param->value.s = strdup(""); if (!param->value.s) { virReportOOMError(); -- 1.7.3.1 -- Thanks, Hu Tao