On 11/08/2011 06:05 AM, Stefan Berger wrote:
> 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(-)
In addition to Stefan's comments, which I fixed as recommended, I had
some additional comments. In particular, qemu's set blkio parameters
had a pre-existing bug, where using 'virsh blkiotune --live
--persistent' failed to update persistent state, which I noticed in
testing this (a simple matter of removing a bogus 'else').
>> --- 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?
I fixed that in my review of 4/5 to have ToStr always report error, so
no need to report error here.
>> + goto cleanup;
>> + }
>> + if (tmp) {
> I think you can remove the if branch.
Not entirely true. virBlkioDeviceWeightToStr sets tmp to NULL and
returns 0 if vm->def->blkio.ndevices is 0. But in that case, why even
bother going into this entire block of code?
>> @@ -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) {
Useless conditional; here we know tmp is non-NULL because
virBlkioDeviceWeightToStr succeeded.
>>
> ACK with those nits fixed.
In my testing, I ran into a weird issue. From the shell:
printf '8:0 500\n8:16 500\n' >
/cgroup/blkio/libvirt/qemu/dom/blkio.weight_device
succeeded at altering the cgroup contents. But:
/usr/bin/printf '8:0 500\n8:16 500\n' >
/cgroup/blkio/libvirt/qemu/dom/blkio.weight_device
fails, as did the C code, with EINVAL. It took me longer than I care to
admit to realize the issue: bash's printf uses line-buffering mode on
ALL files, and results in two separate write() calls of one line each,
while coreutils' printf and our C code was trying to do two lines at
once and getting rejected by the kernel.
Therefore, we need to split up the code to use cgroups.c for only one
device at a time. Also, when thinking about it more, domain_conf.c is
the wrong place to worry about major() and minor(); util/cgroup.c
already worries about it. So I'm going to propose a v8 of the last two
patches, moving a hunk out of 4/5 into 5/5 so that we only use major()
and minor() in cgroup-specific code, rather than globally in domain_conf.c.
Here's where I got prior to hitting the EINVAL issue with multiple
blocks; I'll be posting further edits in the form of v8 soon.
diff --git i/src/qemu/qemu_cgroup.c w/src/qemu/qemu_cgroup.c
index d397578..99b8105 100644
--- i/src/qemu/qemu_cgroup.c
+++ w/src/qemu/qemu_cgroup.c
@@ -312,7 +312,8 @@ int qemuSetupCgroup(struct qemud_driver *driver,
}
}
- if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
+ if (vm->def->blkio.ndevices &&
+ qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
char *tmp;
if (virBlkioDeviceWeightToStr(vm->def->blkio.devices,
vm->def->blkio.ndevices,
@@ -322,15 +323,13 @@ int qemuSetupCgroup(struct qemud_driver *driver,
vm->def->name);
goto cleanup;
}
- if (tmp) {
- rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp);
- VIR_FREE(tmp);
- if (rc != 0) {
- virReportSystemError(-rc,
- _("Unable to set io device weight
for domain %s"),
- vm->def->name);
- goto cleanup;
- }
+ rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp);
+ VIR_FREE(tmp);
+ if (rc != 0) {
+ virReportSystemError(-rc,
+ _("Unable to set io device weight for
domain %s"),
+ vm->def->name);
+ goto cleanup;
}
}
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 01a3fd8..eb5b561 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -5907,7 +5907,8 @@ parseBlkioWeightDeviceStr(char *deviceWeightStr,
}
}
- /* a valid string must have even number of fields */
+ /* A valid string must have even number of fields, hence an odd
+ * number of commas. */
if (!(nsep & 1))
goto error;
@@ -6061,7 +6062,8 @@ static int
qemuDomainSetBlkioParameters(virDomainPtr dom,
char *tmp;
if (param->type != VIR_TYPED_PARAM_STRING) {
qemuReportError(VIR_ERR_INVALID_ARG, "%s",
- _("invalid type for device_weight
tunable, expected a 'char *'"));
+ _("invalid type for device_weight
tunable, "
+ "expected a 'char *'"));
ret = -1;
continue;
}
@@ -6075,40 +6077,36 @@ static int
qemuDomainSetBlkioParameters(virDomainPtr dom,
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) {
- rc = virCgroupSetBlkioDeviceWeight(group, tmp);
- VIR_FREE(tmp);
- if (rc != 0) {
- virReportSystemError(-rc, "%s",
- _("unable to set blkio
weight_device tunable"));
- ret = -1;
- virBlkioDeviceWeightArrayClear(devices,
ndevices);
- VIR_FREE(devices);
- 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 {
+ rc = virCgroupSetBlkioDeviceWeight(group, tmp);
+ VIR_FREE(tmp);
+ if (rc != 0) {
+ virReportSystemError(-rc, "%s",
+ _("unable to set blkio
weight_device tunable"));
+ ret = -1;
virBlkioDeviceWeightArrayClear(devices, ndevices);
VIR_FREE(devices);
+ 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);
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);
@@ -6185,8 +6183,9 @@ static int
qemuDomainGetBlkioParameters(virDomainPtr dom,
VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
- /* We blindly return a string, and let libvirt.c do the filtering
- * on behalf of older clients that can't parse it. */
+ /* 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);
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list