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

Re: [libvirt] [PATCH 4/8] Support block I/O throtte in XML



subject line: s/throtte/throttle/

On 11/15/2011 02:02 AM, Lei Li wrote:
> Enable block I/O throttle for per-disk in XML.
> 
> Signed-off-by: Lei Li <lilei linux vnet ibm com>
> Signed-off-by: Zhi Yong Wu <wuzhy linux vnet ibm com>
> ---
>  src/conf/domain_conf.c  |  101 +++++++++++++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h  |   12 ++++++
>  src/qemu/qemu_command.c |   33 +++++++++++++++
>  src/util/xml.h          |    2 +
>  4 files changed, 145 insertions(+), 3 deletions(-)

I'm now applying this before 3/8.

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 58f4d0f..a157b80 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2318,7 +2318,8 @@ static virDomainDiskDefPtr
>  virDomainDiskDefParseXML(virCapsPtr caps,
>                           xmlNodePtr node,
>                           virBitmapPtr bootMap,
> -                         unsigned int flags)
> +                         unsigned int flags,
> +                         xmlXPathContextPtr ctxt)

The ctxt parameter is usually passed right after node.  I adjusted here
and all callers.

>  {
>      virDomainDiskDefPtr def;
>      xmlNodePtr cur, child;
> @@ -2517,6 +2518,62 @@ 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) {

Indentation.

> +                    def->blkdeviotune.total_bytes_sec = 0;
> +                } else {
> +                    def->blkdeviotune.mark = 1;

I'm not sure the mark field buys us anything.  If 0 means unlimited,
then either we have a limit (at least one field is non-zero) or we don't
(all 6 fields are 0).

> @@ -6003,7 +6060,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,
> -                                                        NULL, flags)))
> +                                                        NULL, flags, NULL)))

Why are you passing a NULL ctxt?  That's a guaranteed NULL deref the
xmlStrEqual(cur->name, BAD_CAST "iotune")) line hits, during any attempt
to hot-plug or hot-unplug a disk with iotuning set.

> @@ -9511,6 +9569,43 @@ virDomainDiskDefFormat(virBufferPtr buf,
>      virBufferAsprintf(buf, "      <target dev='%s' bus='%s'/>\n",
>                        def->dst, bus);
>  
> +    /*disk I/O throttling*/
> +    if (def->blkdeviotune.mark) {

Again, I'm not sure if a mark buys us anything.

> +++ b/src/qemu/qemu_command.c
> @@ -1690,6 +1690,39 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
>          }
>      }
>  
> +    /*block I/O throttling*/
> +    if (disk->blkdeviotune.mark) {

This hunk belongs with 3/8 (qemu implementation), not here.

> diff --git a/src/util/xml.h b/src/util/xml.h
> index c492063..5742f19 100644
> --- a/src/util/xml.h
> +++ b/src/util/xml.h
> @@ -50,6 +50,8 @@ xmlNodePtr          virXPathNode(const char *xpath,
>  int              virXPathNodeSet(const char *xpath,
>                                   xmlXPathContextPtr ctxt,
>                                   xmlNodePtr **list);
> +int     virXMLStringToULongLong (const char *stringval,
> +                                 unsigned long long *value);

This looks like leftovers from a failed experiment.

Here's what I'm squashing in, so far:

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index fb86c18..a2702c5 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -2394,9 +2394,9 @@ cleanup:
 static virDomainDiskDefPtr
 virDomainDiskDefParseXML(virCapsPtr caps,
                          xmlNodePtr node,
+                         xmlXPathContextPtr ctxt,
                          virBitmapPtr bootMap,
-                         unsigned int flags,
-                         xmlXPathContextPtr ctxt)
+                         unsigned int flags)
 {
     virDomainDiskDefPtr def;
     xmlNodePtr cur, child;
@@ -2597,45 +2597,33 @@ virDomainDiskDefParseXML(virCapsPtr caps,
                 }
             } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) {
                 if
(virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)",
-                                       ctxt,
&def->blkdeviotune.total_bytes_sec) < 0) {
+                                      ctxt,
&def->blkdeviotune.total_bytes_sec) < 0) {
                     def->blkdeviotune.total_bytes_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

                 if
(virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)",
-                                       ctxt,
&def->blkdeviotune.read_bytes_sec) < 0) {
+                                      ctxt,
&def->blkdeviotune.read_bytes_sec) < 0) {
                     def->blkdeviotune.read_bytes_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

                 if
(virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)",
-                                       ctxt,
&def->blkdeviotune.write_bytes_sec) < 0) {
+                                      ctxt,
&def->blkdeviotune.write_bytes_sec) < 0) {
                     def->blkdeviotune.write_bytes_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

-               if
(virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)",
-                                       ctxt,
&def->blkdeviotune.total_iops_sec) < 0) {
+                if
(virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)",
+                                      ctxt,
&def->blkdeviotune.total_iops_sec) < 0) {
                     def->blkdeviotune.total_iops_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

                 if
(virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)",
-                                       ctxt,
&def->blkdeviotune.read_iops_sec) < 0) {
+                                      ctxt,
&def->blkdeviotune.read_iops_sec) < 0) {
                     def->blkdeviotune.read_iops_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

                 if
(virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)",
-                                       ctxt,
&def->blkdeviotune.write_iops_sec) < 0) {
+                                      ctxt,
&def->blkdeviotune.write_iops_sec) < 0) {
                     def->blkdeviotune.write_iops_sec = 0;
-                } else {
-                    def->blkdeviotune.mark = 1;
                 }

                 if ((def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.read_bytes_sec)
@@ -6135,8 +6123,8 @@ virDomainDeviceDefPtr
virDomainDeviceDefParse(virCapsPtr caps,

     if (xmlStrEqual(node->name, BAD_CAST "disk")) {
         dev->type = VIR_DOMAIN_DEVICE_DISK;
-        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node,
-                                                        NULL, flags,
NULL)))
+        if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt,
+                                                        NULL, flags)))
             goto error;
     } else if (xmlStrEqual(node->name, BAD_CAST "lease")) {
         dev->type = VIR_DOMAIN_DEVICE_LEASE;
@@ -7205,9 +7193,9 @@ static virDomainDefPtr
virDomainDefParseXML(virCapsPtr caps,
     for (i = 0 ; i < n ; i++) {
         virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps,
                                                             nodes[i],
+                                                            ctxt,
                                                             bootMap,
-                                                            flags,
-                                                            ctxt);
+                                                            flags);
         if (!disk)
             goto error;

@@ -9648,7 +9636,12 @@ virDomainDiskDefFormat(virBufferPtr buf,
                       def->dst, bus);

     /*disk I/O throttling*/
-    if (def->blkdeviotune.mark) {
+    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",
diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h
index 04cd7d8..9180769 100644
--- i/src/conf/domain_conf.h
+++ w/src/conf/domain_conf.h
@@ -344,7 +344,6 @@ struct _virDomainDiskDef {
         unsigned long long total_iops_sec;
         unsigned long long read_iops_sec;
         unsigned long long write_iops_sec;
-        unsigned int mark;
     } blkdeviotune;

     char *serial;
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 449b685..85b34bb 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -1866,39 +1866,6 @@ qemuBuildDriveStr(virConnectPtr conn
ATTRIBUTE_UNUSED,
         }
     }

-    /*block I/O throttling*/
-    if (disk->blkdeviotune.mark) {
-        if (disk->blkdeviotune.total_bytes_sec) {
-            virBufferAsprintf(&opt, ",bps=%llu",
-                              disk->blkdeviotune.total_bytes_sec);
-        }
-
-        if (disk->blkdeviotune.read_bytes_sec) {
-            virBufferAsprintf(&opt, ",bps_rd=%llu",
-                              disk->blkdeviotune.read_bytes_sec);
-        }
-
-        if (disk->blkdeviotune.write_bytes_sec) {
-            virBufferAsprintf(&opt, ",bps_wr=%llu",
-                              disk->blkdeviotune.write_bytes_sec);
-        }
-
-        if (disk->blkdeviotune.total_iops_sec) {
-            virBufferAsprintf(&opt, ",iops=%llu",
-                              disk->blkdeviotune.total_iops_sec);
-        }
-
-        if (disk->blkdeviotune.read_iops_sec) {
-            virBufferAsprintf(&opt, ",iops_rd=%llu",
-                              disk->blkdeviotune.read_iops_sec);
-        }
-
-        if (disk->blkdeviotune.write_iops_sec) {
-            virBufferAsprintf(&opt, ",iops_wr=%llu",
-                              disk->blkdeviotune.write_iops_sec);
-        }
-    }
-
     if (virBufferError(&opt)) {
         virReportOOMError();
         goto error;
diff --git i/src/util/xml.h w/src/util/xml.h
index 5742f19..c492063 100644
--- i/src/util/xml.h
+++ w/src/util/xml.h
@@ -50,8 +50,6 @@ xmlNodePtr          virXPathNode(const char *xpath,
 int              virXPathNodeSet(const char *xpath,
                                  xmlXPathContextPtr ctxt,
                                  xmlNodePtr **list);
-int     virXMLStringToULongLong (const char *stringval,
-                                 unsigned long long *value);
 char *          virXMLPropString(xmlNodePtr node,
                                  const char *name);



-- 
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]