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

Re: [libvirt] [PATCH 5/8] Enable the blkdeviotune command in virsh



On 11/15/2011 02:02 AM, Lei Li wrote:
> Support virsh command blkdeviotune. Can set or query a block disk
> I/O throttle setting.
> 
> Signed-off-by: Lei Li <lilei linux vnet ibm com>
> Signed-off-by: Zhi Yong Wu <wuzhy linux vnet ibm com>
> ---
>  tools/virsh.c   |  240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod |   23 +++++
>  2 files changed, 263 insertions(+), 0 deletions(-)
> 

>  
>  /*
> + * "blkdeviotune" command
> + */
> +static const vshCmdInfo info_blkdeviotune[] = {
> +    {"help", N_("Set or query a block disk I/O throttle setting.")},
> +    {"desc", N_("Set or query a block disk I/O throttle setting.\n" \
> +                "    To query the block disk I/O throttle setting use the following" \
> +                " command: \n\n" \
> +                "    virsh # blkdeviotune <domain> <device>")},

That's a bit long; most of our help text is one line, with the long
version left for the man page (virsh.pod).

> +    {NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_blkdeviotune[] = {
> +    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> +    {"device", VSH_OT_DATA, VSH_OFLAG_REQ, N_("block device")},
> +    {"total_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("total throughput limit in bytes per second")},

I line wrapped these.

> +
> +        for (i = 0; i < nparams; i++) {
> +            switch(params[i].type) {
> +                case VIR_TYPED_PARAM_INT:
> +                    vshPrint(ctl, "%-15s: %d\n", params[i].field,
> +                             params[i].value.i);
> +                    break;

This feels like code duplication that we should factor out in a future
patch, but it doesn't matter for this one.

> +
> +        virDomainFree(dom);
> +        return true;

Memory leak - you need to free params.

> +    } else {
> +        /* Set the block I/O throttle, match by opt since parameters can be 0 */
> +        params = vshCalloc(ctl, nparams, sizeof(*params));
> +        i = 0;
> +

> +        if ((virDomainSetBlockIoTune(dom, disk, params, nparams, flags)) != 0) {
> +            vshError(ctl, "%s",
> +                     _("Unable to change block I/O throttle"));
> +            goto out;
> +        } else {
> +            virDomainFree(dom);
> +            return true;

Same leak.

> @@ -14023,6 +14262,7 @@ static const vshCmdDef domManagementCmds[] = {
>      {"blkiotune", cmdBlkiotune, opts_blkiotune, info_blkiotune, 0},
>      {"blockpull", cmdBlockPull, opts_block_pull, info_block_pull, 0},
>      {"blockjob", cmdBlockJob, opts_block_job, info_block_job, 0},
> +    {"blkdeviotune", cmdBlkdeviotune, opts_blkdeviotune, info_blkdeviotune, 0},

Sorting.

> +++ b/tools/virsh.pod
> @@ -572,6 +572,29 @@ operation can be checked with B<blockjob>.
>  I<path> specifies fully-qualified path of the disk.
>  I<bandwidth> specifies copying bandwidth limit in Mbps.
>  
> +=item B<blkdeviotune> I<domain> I<device> [[I<--total_bytes_sec> B<total_bytes_sec>]
> +| [[I<--read_bytes_sec> B<read_bytes_sec>] [I<--write_bytes_sec> B<write_bytes_sec>]]
> +[[I<--total_iops_sec> B<total_iops_sec>] | [[I<--read_iops_sec> B<read_iops_sec>]
> +[I<--write_iops_sec> B<write_iops_sec>]] [[I<--config>] [I<--live>] | [I<--current>]]

This reads long; I shortened it to I<op> instead of I<--op> B<op>.

> +
> +Set or query the block disk io limits settting.

s/settting/setting/

> +I<path> specifies block disk name.

I copied text from <domblkstat> here.

> +I<--total_bytes_sec> specifies total throughput limit in bytes per second.
> +I<--read_bytes_sec> specifies read throughput limit in bytes per second.
> +I<--write_bytes_sec> specifies write throughput limit in bytes per second.
> +I<--total_iops_sec> specifies total I/O operations limit per second.
> +I<--read_iops_sec> specifies read I/O operations limit per second.
> +I<--write_iops_sec> specifies write I/O operations limit per second.

Needs to mention the restriction on total vs. read/write, also on the
handling of 0: it is necessary to specify at least one flag as explicit
0 to clear limits, otherwise you are just querying limits; and per my
comments in 4/8, the current behavior clears the remaining 5 limits when
one limit is set, although we may find it desirable to instead leave
limits unchanged if they are not specified.

> +
> +If I<--live> is specified, affect a running guest.
> +If I<--config> is specified, affect the next boot of a persistent guest.
> +If I<--current> is specified, affect the current guest state.
> +Both I<--live> and I<--current> flags may be given, but I<--current> is
> +exclusive. If no flag is specified, behavior is different depending
> +on hypervisor.
> +
> +If no limit is specified, it will query current I/O limits setting.

This line belongs earlier.

Here's what I'm squashing:

diff --git i/tools/virsh.c w/tools/virsh.c
index eb8c0b6..ea5a267 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -6079,23 +6079,26 @@ cmdNetworkAutostart(vshControl *ctl, const
vshCmd *cmd)
  * "blkdeviotune" command
  */
 static const vshCmdInfo info_blkdeviotune[] = {
-    {"help", N_("Set or query a block disk I/O throttle setting.")},
-    {"desc", N_("Set or query a block disk I/O throttle setting.\n" \
-                "    To query the block disk I/O throttle setting use
the following" \
-                " command: \n\n" \
-                "    virsh # blkdeviotune <domain> <device>")},
+    {"help", N_("Set or query a block device I/O tuning parameters.")},
+    {"desc", N_("Set or query disk I/O parameters such as block
throttling.")},
     {NULL, NULL}
 };

 static const vshCmdOptDef opts_blkdeviotune[] = {
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
     {"device", VSH_OT_DATA, VSH_OFLAG_REQ, N_("block device")},
-    {"total_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("total
throughput limit in bytes per second")},
-    {"read_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("read throughput
limit in bytes per second")},
-    {"write_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("write
throughput limit in bytes per second")},
-    {"total_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("total I/O
operations limit per second")},
-    {"read_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("read I/O
operations limit per second")},
-    {"write_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, N_("write I/O
operations limit per second")},
+    {"total_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("total throughput limit in bytes per second")},
+    {"read_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("read throughput limit in bytes per second")},
+    {"write_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("write throughput limit in bytes per second")},
+    {"total_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("total I/O operations limit per second")},
+    {"read_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("read I/O operations limit per second")},
+    {"write_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE,
+     N_("write I/O operations limit per second")},
     {"config", VSH_OT_BOOL, 0, N_("affect next boot")},
     {"live", VSH_OT_BOOL, 0, N_("affect running domain")},
     {"current", VSH_OT_BOOL, 0, N_("affect current domain")},
@@ -6116,6 +6119,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     int current = vshCommandOptBool(cmd, "current");
     int config = vshCommandOptBool(cmd, "config");
     int live = vshCommandOptBool(cmd, "live");
+    bool ret = false;

     if (current) {
         if (live || config) {
@@ -6131,18 +6135,18 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     }

     if (!vshConnectionUsability(ctl, ctl->conn))
-        goto out;
+        goto cleanup;

     if (!(dom = vshCommandOptDomain(ctl, cmd, &name)))
-        goto out;
+        goto cleanup;

     if (vshCommandOptString(cmd, "device", &disk) < 0)
-        goto out;
+        goto cleanup;

     if ((rv = vshCommandOptULongLong(cmd, "total_bytes_sec",
&total_bytes_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }
@@ -6150,7 +6154,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     if ((rv = vshCommandOptULongLong(cmd, "read_bytes_sec",
&read_bytes_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }
@@ -6158,7 +6162,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     if ((rv = vshCommandOptULongLong(cmd, "write_bytes_sec",
&write_bytes_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }
@@ -6166,7 +6170,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     if ((rv = vshCommandOptULongLong(cmd, "total_iops_sec",
&total_iops_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }
@@ -6174,7 +6178,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     if ((rv = vshCommandOptULongLong(cmd, "read_iops_sec",
&read_iops_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }
@@ -6182,22 +6186,22 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
     if ((rv = vshCommandOptULongLong(cmd, "write_iops_sec",
&write_iops_sec)) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
-        goto out;
+        goto cleanup;
     } else if (rv > 0) {
         nparams++;
     }

     if (nparams == 0) {

-        if ((virDomainGetBlockIoTune(dom, disk, NULL, &nparams, flags))
!= 0) {
+        if ((virDomainGetBlockIoTune(dom, NULL, NULL, &nparams, flags))
!= 0) {
             vshError(ctl, "%s",
                      _("Unable to get number of block I/O throttle
parameters"));
-            goto out;
+            goto cleanup;
         }

         if (nparams == 0) {
-            virDomainFree(dom);
-            return true;
+            ret = true;
+            goto cleanup;
         }

         params = vshCalloc(ctl, nparams, sizeof(*params));
@@ -6205,7 +6209,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
         if ((virDomainGetBlockIoTune(dom, disk, params, &nparams,
flags)) != 0) {
             vshError(ctl, "%s",
                      _("Unable to get block I/O throttle parameters"));
-            goto out;
+            goto cleanup;
         }

         for (i = 0; i < nparams; i++) {
@@ -6299,19 +6303,19 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
             temp->value.ul = write_iops_sec;
         }

-        if ((virDomainSetBlockIoTune(dom, disk, params, nparams,
flags)) != 0) {
+        if ((virDomainSetBlockIoTune(dom, disk, params, nparams,
flags)) < 0) {
             vshError(ctl, "%s",
                      _("Unable to change block I/O throttle"));
-            goto out;
-        } else {
-            virDomainFree(dom);
-            return true;
+            goto cleanup;
         }
     }

-out:
+    ret = true;
+
+cleanup:
+    VIR_FREE(params);
     virDomainFree(dom);
-    return false;
+    return ret;
 }

 /*
@@ -14928,10 +14932,10 @@ static const vshCmdDef domManagementCmds[] = {
     {"attach-interface", cmdAttachInterface, opts_attach_interface,
      info_attach_interface, 0},
     {"autostart", cmdAutostart, opts_autostart, info_autostart, 0},
+    {"blkdeviotune", cmdBlkdeviotune, opts_blkdeviotune,
info_blkdeviotune, 0},
     {"blkiotune", cmdBlkiotune, opts_blkiotune, info_blkiotune, 0},
     {"blockpull", cmdBlockPull, opts_block_pull, info_block_pull, 0},
     {"blockjob", cmdBlockJob, opts_block_job, info_block_job, 0},
-    {"blkdeviotune", cmdBlkdeviotune, opts_blkdeviotune,
info_blkdeviotune, 0},
 #ifndef WIN32
     {"console", cmdConsole, opts_console, info_console, 0},
 #endif
diff --git i/tools/virsh.pod w/tools/virsh.pod
index bea4288..8737cac 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -572,13 +572,18 @@ operation can be checked with B<blockjob>.
 I<path> specifies fully-qualified path of the disk.
 I<bandwidth> specifies copying bandwidth limit in Mbps.

-=item B<blkdeviotune> I<domain> I<device> [[I<--total_bytes_sec>
B<total_bytes_sec>]
-| [[I<--read_bytes_sec> B<read_bytes_sec>] [I<--write_bytes_sec>
B<write_bytes_sec>]]
-[[I<--total_iops_sec> B<total_iops_sec>] | [[I<--read_iops_sec>
B<read_iops_sec>]
-[I<--write_iops_sec> B<write_iops_sec>]] [[I<--config>] [I<--live>] |
[I<--current>]]
+=item B<blkdeviotune> I<domain> I<device>
+[[I<--config>] [I<--live>] | [I<--current>]]
+[[I<total_bytes_sec>] | [I<read_bytes_sec>] [I<write_bytes_sec>]]
+[[I<total_iops_sec>] | [I<read_iops_sec>] [I<write_iops_sec>]]

-Set or query the block disk io limits settting.
-I<path> specifies block disk name.
+Set or query the block disk io parameters for a block device of I<domain>.
+I<device> specifies a unique target name (<target dev='name'/>) or source
+file (<source file='name'/>) for one of the disk devices attached to
+I<domain> (see also B<domblklist> for listing these names).
+
+If no limit is specified, it will query current I/O limits setting.
+Otherwise, alter the limits with these flags:
 I<--total_bytes_sec> specifies total throughput limit in bytes per second.
 I<--read_bytes_sec> specifies read throughput limit in bytes per second.
 I<--write_bytes_sec> specifies write throughput limit in bytes per second.
@@ -586,6 +591,10 @@ I<--total_iops_sec> specifies total I/O operations
limit per second.
 I<--read_iops_sec> specifies read I/O operations limit per second.
 I<--write_iops_sec> specifies write I/O operations limit per second.

+When setting any value, all remaining values are reset to unlimited,
+an explicit 0 also clears any limit.  A non-zero value for a given total
+cannot be mixed with non-zero values for read or write.
+
 If I<--live> is specified, affect a running guest.
 If I<--config> is specified, affect the next boot of a persistent guest.
 If I<--current> is specified, affect the current guest state.
@@ -593,8 +602,6 @@ Both I<--live> and I<--current> flags may be given,
but I<--current> is
 exclusive. If no flag is specified, behavior is different depending
 on hypervisor.

-If no limit is specified, it will query current I/O limits setting.
-
 =item B<blockjob> I<domain> I<path> [I<--abort>] [I<--info>] [I<bandwidth>]

 Manage active block operations.

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