[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 2/2] add interface for blkio.weight_device



On 09/23/2011 12:40 AM, Hu Tao wrote:
This patch adds a parameter --weight-device to virsh command
blkiotune for setting/getting blkio.weight_device.
---
  daemon/remote.c              |    5 +
  include/libvirt/libvirt.h.in |    9 ++
  src/conf/domain_conf.c       |  114 ++++++++++++++++++++++++-
  src/conf/domain_conf.h       |   16 ++++
  src/libvirt_private.syms     |    1 +
  src/qemu/qemu_cgroup.c       |   22 +++++
  src/qemu/qemu_driver.c       |  191 +++++++++++++++++++++++++++++++++++++++++-
  src/util/cgroup.c            |   33 +++++++
  src/util/cgroup.h            |    3 +
  tools/virsh.c                |   52 ++++++++++--
  tools/virsh.pod              |    5 +-
  11 files changed, 437 insertions(+), 14 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 82ee13b..bee1b94 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1566,6 +1566,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
      int nparams = args->nparams;
      unsigned int flags;
      int rv = -1;
+    int i;
      struct daemonClientPrivate *priv =
          virNetServerClientGetPrivateData(client);

@@ -1610,6 +1611,10 @@ success:
  cleanup:
      if (rv<  0)
          virNetMessageSaveError(rerr);
+    for (i = 0; i<  nparams; i++) {
+        if (params[i].type == VIR_TYPED_PARAM_STRING)
+            VIR_FREE(params[i].value.s);
+    }
      VIR_FREE(params);

This for loop seems like something that will be done frequently enough that it might be worth factoring it into a util file for managing virTypedParameters. Something like: virTypedParameterFree(params).

      if (dom)
          virDomainFree(dom);
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 448a0e7..a1f2c98 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1139,6 +1139,15 @@ char *                  virDomainGetSchedulerType(virDomainPtr domain,

  #define VIR_DOMAIN_BLKIO_WEIGHT "weight"

+/**
+ * VIR_DOMAIN_BLKIO_WEIGHT_DEVICE:
+ *
+ * Macro for the blkio tunable weight_device: it represents the
+ * per device weight.
+ */

Also mention that this name refers to a VIR_TYPED_PARAMETER_STRING.

+/**
+ * virDomainBlkioWeightDeviceParseXML
+ *
+ * this function parses a XML node:
+ *
+ *<device>
+ *<path>/fully/qulaified/device/path</path>

s/qulaified/qualified/

+ *<weight>weight</weight>
+ *</device>
+ *
+ * and fills a virBlkioWeightDevice struct.
+ */
+static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root,
+                                              virBlkioWeightDevicePtr dw)
+{
+    char *c;
+    struct stat s;
+    xmlNodePtr node;
+
+    if (!dw)
+        return -1;
+
+    node = root->children;
+    while (node) {
+        if (node->type == XML_ELEMENT_NODE) {
+            if (xmlStrEqual(node->name, BAD_CAST "path")) {
+                c = (char *)xmlNodeGetContent(node);
+                if (!c)
+                    return -1;
+                if (stat(c,&s) == -1)
+                    return -1;

This stat() ties the parse to the existence of the node. But that seems wrong - it should be possible to define a domain with XML that refers to a node that does not yet exist, provided that the node later exists at the time we try to start the domain.

+                if ((s.st_mode&  S_IFMT) == S_IFBLK) {
+                    dw->major = major(s.st_rdev);
+                    dw->minor = minor(s.st_rdev);

Also, breaking the parse into major/minor this early makes the result unmigratable, since we can't guarantee major/minor stability across hosts. I think you have to store the name, not the major/minor, here in the domain_conf representation of the device weight, and only convert to major/minor in the hypervisor that is actually enforcing the limits.

+                } else
+                    return -1;
+                dw->path = (char *)xmlNodeGetContent(node);
+            } else if (xmlStrEqual(node->name, BAD_CAST "weight")) {
+                c = (char *)xmlNodeGetContent(node);
+                dw->weight = atoi(c);

atoi is unsafe. It cannot detect errors. You should use virStrToLong_i (or similar) instead.

@@ -10499,10 +10591,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
                        def->mem.cur_balloon);

      /* add blkiotune only if there are any */
-    if (def->blkio.weight) {
+    if (def->blkio.weight || def->blkio.weight_devices) {
          virBufferAsprintf(buf, "<blkiotune>\n");
-        virBufferAsprintf(buf, "<weight>%u</weight>\n",
-                          def->blkio.weight);
+
+        if (def->blkio.weight)
+            virBufferAsprintf(buf, "<weight>%u</weight>\n",

(Stupid Thunderbird for munging whitespace).

This will conflict with my patch series for indenting <domainsnapshot>. Shouldn't be too hard to figure out, but it boils down to who gets acked first.

+++ b/src/qemu/qemu_driver.c
@@ -44,6 +44,7 @@
  #include<sys/ioctl.h>
  #include<sys/un.h>
  #include<byteswap.h>
+#include<ctype.h>

This should use "c-ctype.h", not <ctype.h>.

+/* weightDeviceStr 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(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size)
+{
+    struct stat s;
+    const char *temp;
+    int nDevice = 0;
+    int i, len;
+    virBlkioWeightDevicePtr result = NULL;
+
+    if (!dw)
+        return -1;
+    if (*dw)
+        return -1;

This returns -1 without an error, but you later return -1 after reporting OOM error, so the caller can't tell things apart. That means you need to report an error here. On the other hand, since this function is static, you could audit that all callers pass correct parameters in the first place, and just omit this validation step.

+
+    temp = weightDeviceStr;
+    while (temp) {
+        temp = strchr(temp, ';');

What happens if the absolute path of a device contains a literal ';'? I guess we don't anticipate that happening.

+    i = 0;
+    temp = weightDeviceStr;
+    while (temp&&  i<  nDevice) {
+        const char *p = temp;
+
+        /* device path */
+
+        while (*p != ','&&  *p != '\0')
+            ++p;

strchr(p,',') would be faster.

+        if (*p == '\0')
+            goto fail;
+        len = p - temp + 1;
+        if (VIR_ALLOC_N(result[i].path, len)<  0)
+            goto fail;
+        memcpy(result[i].path, temp, len - 1);

strndup() would be better than VIR_ALLOC_N and memcpy.

+        result[i].path[len - 1] = '\0';
+
+        if (stat(result[i].path,&s) == -1) {
+            VIR_FREE(result[i].path);
+            goto fail;
+        }
+        if ((s.st_mode&  S_IFMT) != S_IFBLK) {
+            VIR_FREE(result[i].path);
+            goto fail;
+        }
+        result[i].major = major(s.st_rdev);
+        result[i].minor = minor(s.st_rdev);

major() and minor() are non-portable; this code may need to be conditionally compiled for non-Linux platforms. See how cgroup does conditional compilation before accessing major and minor numbers.

+
+        /* weight */
+
+        temp = p + 1;
+        if (!temp)
+            goto fail;
+
+        p = temp;
+        while (isdigit(*p)&&  ++p);

s/isdigit/c_isdigit/

+        if (!p || (*p != ';'&&  *p != '\0'))
+            goto fail;
+
+        result[i].weight = atoi(temp);

No atoi.  Use virStrToLong_i or similar.

  static int qemuDomainSetBlkioParameters(virDomainPtr dom,
                                           virTypedParameterPtr params,
                                           int nparams,
@@ -5860,10 +5951,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
      ret = 0;
      if (flags&  VIR_DOMAIN_AFFECT_LIVE) {

You forgot to update the virCheckFlags to allow the new VIR_DOMAIN_TYPED_STRING_OKAY flag. (Actually, I'm starting to think it might be better to name that flag VIR_TYPED_PARAM_STRING_OKAY, since it only makes sense with any API that uses virTypedParameter.

          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'"));
@@ -5884,6 +5975,44 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom,
                                           _("unable to set blkio weight tunable"));
                      ret = -1;
                  }
+            } else if(STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) {
+                int ndevices;
+                virBlkioWeightDevicePtr tmp = NULL;
+                if (param->type != VIR_TYPED_PARAM_STRING) {

Here, you should also fail if flags does not include the OKAY flag, as that indicates a client that isn't properly using the API.

+                for (i = 0; i<  ndevices; i++) {
+                    char *weight_device = NULL;
+
+                    virAsprintf(&weight_device, "%d:%d %d",
+                                tmp[i].major,
+                                tmp[i].minor,
+                                tmp[i].weight);
+                    if (weight_device) {
+                        rc = virCgroupSetBlkioWeightDevice(group, weight_device);
+                        VIR_FREE(weight_device);
+                        weight_device = NULL;
+                        if (rc != 0) {
+                            virReportSystemError(-rc, "%s",
+                                                 _("unable to set blkio weight_device tunable"));
+                            ret = -1;
+                            break;
+                        }
+                    }

You should report OOM errors if allocating weight_device failed, rather than silently ignoring them.

@@ -6012,6 +6160,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,

      if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
          for (i = 0; i<  *nparams; i++) {
+            char *weight_device;
              virTypedParameterPtr param =&params[i];
              val = 0;
              param->value.ui = 0;

If someone calls GetBlkioParameters with a size of 0 but without the _OKAY flag, to learn how large to allocate the array, then you need to return 1, not 2. I don't see that in your patch (I only see where if they allocate 2, but didn't pass _OKAY, that you don't populate the second entry).

+++ b/tools/virsh.c
@@ -4616,6 +4616,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]")},
+    {"weight-device", VSH_OT_STRING, VSH_OFLAG_NONE,
+     N_("per device IO Weight, in the form of major:minor,weight")},

s/per device/per-device/

Do not expose major:minor,weight through virsh. Rather, you should match the xml, and expose this via /path/to/device,weight (you got it right in virsh.pod).


+    flags |= VIR_DOMAIN_TYPED_STRING_OKAY;
+
      if (!vshConnectionUsability(ctl, ctl->conn))
          return false;

@@ -4670,12 +4675,28 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
          }
      }

+    rv = vshCommandOptString(cmd, "weight-device",&weight_device);
+    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 */
+        /* old libvirtd doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, we
+         * give it a second try with this flag disabled in the case of an
+         * old libvirtd. */

Won't work as written. nparams will be -1, not 0, if the old server didn't understand the _OKAY flag. Also, you should check last_error->code, and only try the fallback if the error is VIR_ERR_INVALID_ARG (any other error is fatal without having to try the fallback).

          if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) {
-            vshError(ctl, "%s",
-                     _("Unable to get number of blkio parameters"));
-            goto cleanup;
+            flags&= ~VIR_DOMAIN_TYPED_STRING_OKAY;
+            if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) {
+                vshError(ctl, "%s",
+                         _("Unable to get number of blkio parameters"));
+                goto cleanup;
+            }
          }

          if (nparams == 0) {
@@ -4717,6 +4738,10 @@ 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");
              }
@@ -4737,11 +4762,24 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
                          sizeof(temp->field));
                  weight = 0;
              }
+
+            if (weight_device) {
+                virBuffer buf = VIR_BUFFER_INITIALIZER;
+                virBufferAsprintf(&buf, "%s", weight_device);
+                temp->value.s = virBufferContentAndReset(&buf);

Why not just: temp->value.s = weight_device, instead of going through a virBuffer?

+                temp->type = VIR_TYPED_PARAM_STRING;
+                strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE,
+                        sizeof(temp->field));
+            }
+        }
+        ret = true;
+        if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) {

You need to fix the logic to pass _OKAY if weight_device was specified. Also, I think your logic is not quite right in that you pre-set flags to include _OKAY even if you are not passing weight_device, which will cause needless problems when talking to an older server if you aren't even going to be changing device weight.

  Display or set the blkio parameters. QEMU/KVM supports I<--weight>.
  I<--weight>  is in range [100, 1000].

+B<weight-device>  is in the format of /device/patch,weight;/device/patch,weight.

s/patch/path/g

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]