[libvirt] [PATCH] filter blkio 0-device-weight at two oter places(was Re: [PATCHv4 4/4] blkiotune: add qemu support for blkiotune.device_weight)

[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 at 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 at 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