Re: [libvirt] [PATCH 5/5] blkiotune: add qemu support for blkiotune.device_weight

On 11/08/2011 06:05 AM, Stefan Berger wrote: 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'). I fixed that in my review of 4/5 to have ToStr always report error, so no need to report error here. 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? Useless conditional; here we know tmp is non-NULL because virBlkioDeviceWeightToStr succeeded. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org