On Tue, Nov 29, 2011 at 10:41:58AM -0700, Eric Blake wrote: ACK 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 :|

On Mon, Nov 14, 2011 at 09:30:00PM -0700, Eric Blake wrote: ACK 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 :|

From: Hu Tao <hutao(a)cn.fujitsu.com&gt; 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(a)cn.fujitsu.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- 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 @@ ... &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..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"); } 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

On 11/15/2011 12:15 AM, Hu Tao wrote: If we go this route, then this should be bool, not int. and once you set the flag, you can break to shorten the loop. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Hu Tao <hutao(a)cn.fujitsu.com&gt; 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); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to set io device weight " + "for path %s"), + result[i].path); + goto cleanup; + } + } + + /* 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 != ',') + goto error; + temp = p + 1; + } + + if (!i) + VIR_FREE(result); + + *dw = result; + *size = i; + + return 0; + +error: + qemuReportError(VIR_ERR_INVALID_ARG, + _("unable to parse %s"), deviceWeightStr); +cleanup: + virBlkioDeviceWeightArrayClear(result, ndevices); + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5951,10 +6050,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { + int rc; virTypedParameterPtr param = &params[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'")); @@ -5975,6 +6074,28 @@ 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; + 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, group) < 0) { + ret = -1; + continue; + } + virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, + vm->def->blkio.ndevices); + VIR_FREE(vm->def->blkio.devices); + vm->def->blkio.devices = devices; + vm->def->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6007,9 +6128,31 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } persistentDef->blkio.weight = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + virBlkioDeviceWeightPtr devices = NULL; + int ndevices; + 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, NULL) < 0) { + ret = -1; + continue; + } + virBlkioDeviceWeightArrayClear(persistentDef->blkio.devices, + persistentDef->blkio.ndevices); + VIR_FREE(persistentDef->blkio.devices); + persistentDef->blkio.devices = devices; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, - _("Parameter `%s' not supported"), param->field); + _("Parameter `%s' not supported"), + param->field); ret = -1; } } @@ -6032,7 +6175,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; - int i; + int i, j; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -6046,7 +6189,9 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, VIR_TYPED_PARAM_STRING_OKAY, -1); qemuDriverLock(driver); - /* We don't return strings, and thus trivially support this flag. */ + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6125,6 +6270,37 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkiotune.device_weight */ + if (vm->def->blkio.ndevices > 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j < vm->def->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + vm->def->blkio.devices[j].path, + vm->def->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break; default: break; @@ -6149,6 +6325,38 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break; + case 1: /* blkiotune.device_weight */ + if (persistentDef->blkio.ndevices > 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j < persistentDef->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + persistentDef->blkio.devices[j].path, + persistentDef->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break; + default: break; /* should not hit here */ diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c8d1f33..496f9ff 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -982,6 +982,57 @@ int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) } /** + * virCgroupSetBlkioDeviceWeight: + * + * @group: The cgroup to change io device weight device for + * @path: The device with a weight to alter + * @weight: The new device weight (100-1000), or 0 to clear + * + * device_weight is treated as a write-only parameter, so + * there isn't a getter counterpart. + * + * Returns: 0 on success, -errno on failure + */ +#if defined(major) && defined(minor) +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *path, + unsigned int weight) +{ + char *str; + struct stat sb; + int ret; + + if (weight && (weight > 1000 || weight < 100)) + return -EINVAL; + + if (stat(path, &sb) < 0) + return -errno; + + if (!S_ISBLK(sb.st_mode)) + return -EINVAL; + + if (virAsprintf(&str, "%d:%d %d", major(sb.st_rdev), minor(sb.st_rdev), + weight) < 0) + return -errno; + + ret = virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + str); + VIR_FREE(str); + return ret; +} +#else + int + virCgroupSetBlkioDeviceWeight(virCgroupPtr group ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + unsigned int weight ATTRIBUTE_UNUSED) +{ + return -ENOSYS; +} +#endif + +/** * virCgroupSetMemory: * * @group: The cgroup to change memory for diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d190bb3..70dd392 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -55,6 +55,10 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight); +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *path, + unsigned int weight); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.args b/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.args new file mode 100644 index 0000000..651793d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkiotune-device.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -monitor unix:/tmp/test-monitor,\ +server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ +none -parallel none -usb diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d9a6e8d..c75b68c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -583,6 +583,7 @@ mymain(void) DO_TEST("memtune", false, QEMU_CAPS_NAME); DO_TEST("blkiotune", false, QEMU_CAPS_NAME); + DO_TEST("blkiotune-device", false, QEMU_CAPS_NAME); DO_TEST("cputune", false, QEMU_CAPS_NAME); DO_TEST("numatune-memory", false, NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f37520..32445fd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -184,6 +184,7 @@ mymain(void) DO_TEST("encrypted-disk"); DO_TEST("memtune"); DO_TEST("blkiotune"); + DO_TEST("blkiotune-device"); DO_TEST("cputune"); DO_TEST("smp"); -- 1.7.7.1

On Tue, Nov 15, 2011 at 03:22:03PM +0800, Hu Tao wrote: Yes, IMHO it is better to do the setting of values, after parsing is fully complete. 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 :|

On 11/15/2011 12:22 AM, Hu Tao wrote: 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(); -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/29/2011 07:11 PM, Hu Tao wrote: s/oter/other/ Good catches, and your patch looks almost right. Here, we can't use 'j' as the key for whether to print a comma, but have to introduce a new variable. ACK with this squashed in, and pushed: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 0b1d6cf..ed90c66 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -6312,6 +6312,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if (vm->def->blkio.ndevices > 0) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool comma = false; + for (j = 0; j < vm->def->blkio.ndevices; j++) { if (!vm->def->blkio.devices[j].weight) continue; @@ -6372,11 +6373,15 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, case 1: /* blkiotune.device_weight */ if (persistentDef->blkio.ndevices > 0) { virBuffer buf = VIR_BUFFER_INITIALIZER; + bool comma = false; + for (j = 0; j < persistentDef->blkio.ndevices; j++) { if (!persistentDef->blkio.devices[j].weight) continue; - if (j) + if (comma) virBufferAddChar(&buf, ','); + else + comma = true; virBufferAsprintf(&buf, "%s,%u", persistentDef->blkio.devices[j].path, persistentDef->blkio.devices[j].weight); -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org