On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
> From: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
>
> Enable block I/O throttle for per-disk in XML, as the first
> per-disk IO tuning parameter.
>
> Signed-off-by: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
[...]
> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list