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

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



On 11/24/2011 12:54 AM, Daniel Veillard wrote:
> On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
>> From: Lei Li <lilei 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 linux vnet ibm com>
>> Signed-off-by: Zhi Yong Wu <wuzhy linux vnet ibm com>
>> Signed-off-by: Eric Blake <eblake redhat com>
> [...]

>   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 ...)

We've done this elsewhere, so it's pretty easy to copy.

> 
>   ACK, once fixed.

Here's what I'm squashing in.  I still have to actually test response on
qemu, though, before I'm comfortable pushing (it may be that the code
as-is, while passing its self-test on xml handling, fails to gracefully
handle old qemu that lacks support for this feature).

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index a2702c5..caf4cf5 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -2400,6 +2400,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
 {
     virDomainDiskDefPtr def;
     xmlNodePtr cur, child;
+    xmlNodePtr save_ctxt = ctxt->node;
     char *type = NULL;
     char *device = NULL;
     char *snapshot = NULL;
@@ -2431,6 +2432,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
         return NULL;
     }

+    ctxt->node = node;
+
     type = virXMLPropString(node, "type");
     if (type) {
         if ((def->type = virDomainDiskTypeFromString(type)) < 0) {
@@ -2596,47 +2599,59 @@ 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) {
+                if (virXPathULongLong("string(./iotune/total_bytes_sec)",
+                                      ctxt,
+
&def->blkdeviotune.total_bytes_sec) < 0) {
                     def->blkdeviotune.total_bytes_sec = 0;
                 }

-                if
(virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)",
-                                      ctxt,
&def->blkdeviotune.read_bytes_sec) < 0) {
+                if (virXPathULongLong("string(./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) {
+                if (virXPathULongLong("string(./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) {
+                if (virXPathULongLong("string(./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) {
+                if (virXPathULongLong("string(./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) {
+                if (virXPathULongLong("string(./iotune/write_iops_sec)",
+                                      ctxt,
+
&def->blkdeviotune.write_iops_sec) < 0) {
                     def->blkdeviotune.write_iops_sec = 0;
                 }

-                if ((def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.read_bytes_sec)
-                    || (def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.write_bytes_sec)) {
+                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"));
+                                         _("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)) {
+                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"));
+                                         _("total and read/write iops_sec "
+                                           "cannot be set at the same
time"));
                     goto error;
                 }
             } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
@@ -2933,6 +2948,7 @@ cleanup:
     virStorageEncryptionFree(encryption);
     VIR_FREE(startupPolicy);

+    ctxt->node = save_ctxt;
     return def;

 no_memory:


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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