[libvirt] [PATCH 0/3] leave cgroup value checking to kernel

From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Since 2.6.39, kernel changed the minimum weight of device blkio while libvirt hard-coded the value range checking. So we should leave the job of checking to kernel. Chen Hanxiao (3): [libvirt]docs: change the minimum weight description for blkio [libvirt]cgroup: show error when EINVAL caught [libvirt]cgroup: leave value checking to kernel docs/formatdomain.html.in | 6 ++++-- src/util/vircgroup.c | 26 +++++++++++--------------- tools/virsh.pod | 11 ++++++----- 3 files changed, 21 insertions(+), 22 deletions(-) -- 1.8.2.1

From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Since 2.6.39, kernel changed the minimum weight of device blkio. Update related docs. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- docs/formatdomain.html.in | 6 ++++-- tools/virsh.pod | 11 ++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3689399..6458567 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -772,7 +772,8 @@ <dt><code>weight</code></dt> <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> + 1000]. After kernel 2.6.39, the value could be in the + range [10, 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 @@ -789,7 +790,8 @@ 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> + 1000]. After kernel 2.6.39, the value could be in the + range [10, 1000].<span class="since">Since 0.9.8</span></dd> </dl> diff --git a/tools/virsh.pod b/tools/virsh.pod index e12a800..4114ab6 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1623,14 +1623,15 @@ Specifying -1 as a value for these limits is interpreted as unlimited. [I<--live>] | [I<--current>]] Display or set the blkio parameters. QEMU/KVM supports I<--weight>. -I<--weight> is in range [100, 1000]. +I<--weight> is in range [100, 1000]. After kernel 2.6.39, the value +could be in the range [10, 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. Only the devices listed in the string -are modified; any existing per-device weights for other devices remain -unchanged. +Each weight is in the range [100, 1000], [10, 1000] after kernel 2.6.39, +or the value 0 to remove that device from per-device listings. +Only the devices listed in the string are modified; +any existing per-device weights for other devices remain unchanged. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -- 1.8.2.1

On Fri, Oct 11, 2013 at 09:41:22PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Since 2.6.39, kernel changed the minimum weight of device blkio. Update related docs.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- docs/formatdomain.html.in | 6 ++++-- tools/virsh.pod | 11 ++++++----- 2 files changed, 10 insertions(+), 7 deletions(-)
ACK and pushed. 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: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> When EINVAL caught, tell user that what values are invalid for what field. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e99caf5..a98bd63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -664,12 +664,21 @@ virCgroupSetValueStr(virCgroupPtr group, { int ret = -1; char *keypath = NULL; + char *tmp = NULL; if (virCgroupPathOfController(group, controller, key, &keypath) < 0) return -1; VIR_DEBUG("Set value '%s' to '%s'", keypath, value); if (virFileWriteStr(keypath, value, 0) < 0) { + if (errno == EINVAL) { + tmp = strrchr(keypath, '/'); + if (!tmp) + goto cleanup; + virReportSystemError(errno, + _("Invalid value '%s' for '%s'"), value, ++tmp); + goto cleanup; + } virReportSystemError(errno, _("Unable to write to '%s'"), keypath); goto cleanup; @@ -1829,7 +1838,8 @@ virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) * * @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 + * @weight: The new device weight (100-1000), + * (10-1000) after kernel 2.6.39, or 0 to clear * * device_weight is treated as a write-only parameter, so * there isn't a getter counterpart. -- 1.8.2.1

On Fri, Oct 11, 2013 at 09:41:23PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
When EINVAL caught, tell user that what values are invalid for what field.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index e99caf5..a98bd63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -664,12 +664,21 @@ virCgroupSetValueStr(virCgroupPtr group, { int ret = -1; char *keypath = NULL; + char *tmp = NULL;
if (virCgroupPathOfController(group, controller, key, &keypath) < 0) return -1;
VIR_DEBUG("Set value '%s' to '%s'", keypath, value); if (virFileWriteStr(keypath, value, 0) < 0) { + if (errno == EINVAL) { + tmp = strrchr(keypath, '/'); + if (!tmp) + goto cleanup;
This would fail to raise an error if '!tmp', so I re-arranged the code todo if (errno == EINVAL && (tmp = strrchr(keypath, '/'))) { ... so it falls back to the existing error report
+ virReportSystemError(errno, + _("Invalid value '%s' for '%s'"), value, ++tmp); + goto cleanup; + } virReportSystemError(errno, _("Unable to write to '%s'"), keypath); goto cleanup;
ACK & pushed with the above change 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 :|

-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, October 15, 2013 7:24 PM To: Chen Hanxiao Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 2/3]cgroup: show error when EINVAL caught
On Fri, Oct 11, 2013 at 09:41:23PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
This would fail to raise an error if '!tmp', so I re-arranged the code todo
if (errno == EINVAL && (tmp = strrchr(keypath, '/'))) { ...
so it falls back to the existing error report
Thanks for your help. I'll be more careful next time.
+ virReportSystemError(errno, + _("Invalid value '%s' for '%s'"), value, ++tmp); + goto cleanup;

From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> Values for cgroup may change in the future kernels. We should not hard-coded it. Leave this job to kernel, who will take care of them. Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a98bd63..b164dca 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1797,13 +1797,6 @@ virCgroupPathOfController(virCgroupPtr group, int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight) { - if (weight > 1000 || weight < 100) { - virReportError(VIR_ERR_INVALID_ARG, - _("weight '%u' must be in range (100, 1000)"), - weight); - return -1; - } - return virCgroupSetValueU64(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.weight", @@ -1855,13 +1848,6 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group, struct stat sb; int ret; - if (weight && (weight > 1000 || weight < 100)) { - virReportError(VIR_ERR_INVALID_ARG, - _("weight '%u' must be in range (100, 1000)"), - weight); - return -1; - } - if (stat(path, &sb) < 0) { virReportSystemError(errno, _("Path '%s' is not accessible"), -- 1.8.2.1

On Fri, Oct 11, 2013 at 09:41:24PM +0800, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
Values for cgroup may change in the future kernels. We should not hard-coded it. Leave this job to kernel, who will take care of them.
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- src/util/vircgroup.c | 14 -------------- 1 file changed, 14 deletions(-)
ACK & pushed 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 :|
participants (2)
-
Chen Hanxiao
-
Daniel P. Berrange