[libvirt] [PATCHv6 3/7] Support block I/O throttle in XML

[Daniel Veillard]

On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
> From: Lei Li <lilei at linux.vnet.ibm.com>
> 
> Enable block I/O throttle for per-disk in XML, as the first
> per-disk IO tuning parameter.
> 
> Signed-off-by: Lei Li <lilei at linux.vnet.ibm.com>
> Signed-off-by: Zhi Yong Wu <wuzhy at linux.vnet.ibm.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
[...]
>  static virDomainDiskDefPtr
>  virDomainDiskDefParseXML(virCapsPtr caps,
>                           xmlNodePtr node,
> +                         xmlXPathContextPtr ctxt,
>                           virBitmapPtr bootMap,
>                           unsigned int flags)
>  {
> @@ -2594,6 +2595,50 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>                      }
>                      child = child->next;
>                  }
> +            } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
> +                if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)",
> +                                      ctxt, &def->blkdeviotune.total_bytes_sec) < 0) {
> +                    def->blkdeviotune.total_bytes_sec = 0;
> +                }

  This code is buggy I'm afraid. It's supposed to apply to
  virDomainDiskDefParseXML() which will go down in the tree searching
for the informations on a given disk. By using an XPath query on the
passed context without updating it with the current node, you go back
to the domain root search and scan the fulls set of devices for iotune
parameter, then if there is more than one disk with such param
you will *concatenate* the string and get an erroneous value.
  2 ways to fix this:
    - reset the node from ctxt to the current node of the disk parsed
      and use "iotune/total_bytes_sec" expressions (may also fit in the
      80 char line ...)
    - just get the value directly not using XPath, but then get rid of
      the extra ctxt

 first is probably simpler w.r.t. current patch,

> +                if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)",
> +                                      ctxt, &def->blkdeviotune.read_bytes_sec) < 0) {
> +                    def->blkdeviotune.read_bytes_sec = 0;
> +                }
> +
> +                if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)",
> +                                      ctxt, &def->blkdeviotune.write_bytes_sec) < 0) {
> +                    def->blkdeviotune.write_bytes_sec = 0;
> +                }
> +
> +                if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)",
> +                                      ctxt, &def->blkdeviotune.total_iops_sec) < 0) {
> +                    def->blkdeviotune.total_iops_sec = 0;
> +                }
> +
> +                if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)",
> +                                      ctxt, &def->blkdeviotune.read_iops_sec) < 0) {
> +                    def->blkdeviotune.read_iops_sec = 0;
> +                }
> +
> +                if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)",
> +                                      ctxt, &def->blkdeviotune.write_iops_sec) < 0) {
> +                    def->blkdeviotune.write_iops_sec = 0;
> +                }

  same applies to all those XPath queries.

> +                if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec)
> +                    || (def->blkdeviotune.total_bytes_sec && def->blkdeviotune.write_bytes_sec)) {
> +                    virDomainReportError(VIR_ERR_XML_ERROR,
> +                                         _("total and read/write bytes_sec cannot be set at the same time"));
> +                    goto error;
> +                }
> +
> +                if ((def->blkdeviotune.total_iops_sec && def->blkdeviotune.read_iops_sec)
> +                    || (def->blkdeviotune.total_iops_sec && def->blkdeviotune.write_iops_sec)) {
> +                    virDomainReportError(VIR_ERR_XML_ERROR,
> +                                         _("total and read/write iops_sec cannot be set at the same time"));
> +                    goto error;
> +                }
>              } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
>                  def->readonly = 1;
>              } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
> @@ -6078,7 +6123,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
> 
>      if (xmlStrEqual(node->name, BAD_CAST "disk")) {
>          dev->type = VIR_DOMAIN_DEVICE_DISK;
> -        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node,
> +        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt,
>                                                          NULL, flags)))
>              goto error;
>      } else if (xmlStrEqual(node->name, BAD_CAST "lease")) {
> @@ -7148,6 +7193,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>      for (i = 0 ; i < n ; i++) {
>          virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps,
>                                                              nodes[i],
> +                                                            ctxt,
>                                                              bootMap,
>                                                              flags);
>          if (!disk)
> @@ -9589,6 +9635,48 @@ virDomainDiskDefFormat(virBufferPtr buf,
>      virBufferAsprintf(buf, "      <target dev='%s' bus='%s'/>\n",
>                        def->dst, bus);
> 
> +    /*disk I/O throttling*/
> +    if (def->blkdeviotune.total_bytes_sec ||
> +        def->blkdeviotune.read_bytes_sec ||
> +        def->blkdeviotune.write_bytes_sec ||
> +        def->blkdeviotune.total_iops_sec ||
> +        def->blkdeviotune.read_iops_sec ||
> +        def->blkdeviotune.write_iops_sec) {
> +        virBufferAddLit(buf, "      <iotune>\n");
> +        if (def->blkdeviotune.total_bytes_sec) {
> +            virBufferAsprintf(buf, "        <total_bytes_sec>%llu</total_bytes_sec>\n",
> +                              def->blkdeviotune.total_bytes_sec);
> +        }
> +
> +        if (def->blkdeviotune.read_bytes_sec) {
> +            virBufferAsprintf(buf, "        <read_bytes_sec>%llu</read_bytes_sec>\n",
> +                              def->blkdeviotune.read_bytes_sec);
> +
> +        }
> +
> +        if (def->blkdeviotune.write_bytes_sec) {
> +            virBufferAsprintf(buf, "        <write_bytes_sec>%llu</write_bytes_sec>\n",
> +                              def->blkdeviotune.write_bytes_sec);
> +        }
> +
> +        if (def->blkdeviotune.total_iops_sec) {
> +            virBufferAsprintf(buf, "        <total_iops_sec>%llu</total_iops_sec>\n",
> +                              def->blkdeviotune.total_iops_sec);
> +        }
> +
> +        if (def->blkdeviotune.read_iops_sec) {
> +            virBufferAsprintf(buf, "        <read_iops_sec>%llu</read_iops_sec>",
> +                              def->blkdeviotune.read_iops_sec);
> +        }
> +
> +        if (def->blkdeviotune.write_iops_sec) {
> +            virBufferAsprintf(buf, "        <write_iops_sec>%llu</write_iops_sec>",
> +                              def->blkdeviotune.write_iops_sec);
> +        }
> +
> +        virBufferAddLit(buf, "      </iotune>\n");
> +    }
> +

  Don't forget to reset back the ctxt->node on exit from the parsing
  routine if picking option 1

>      if (def->bootIndex)
>          virBufferAsprintf(buf, "      <boot order='%d'/>\n", def->bootIndex);
>      if (def->readonly)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h

  ACK, once fixed.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/