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

[libvirt] [PATCH v4 5/7] blockjob: improve handling of bandwidth in virsh



Treating -1 as the maximum bandwidth of block jobs is not very
practical, given the restrictions on overflow between 32-bit
vs. 64-bit long, as well as conversions between MiB/s and bytes/s.
We already document that 0 means unlimited, so the only reason to
keep negative support is for back-compat.

Meanwhile, it is a good idea to allow users to input in scales
other than MiB/s.  This patch just rounds back up to MiB/s, but by
using a common helper, a later patch can then determine when a
particular input scale will be better for using flags with the
corresponding API call.

* tools/virsh-domain.c (blockJobBandwidth): New function.
(blockJobImpl, cmdBlockCopy): Use it.
* tools/virsh.pod (blockjob, blockcopy, blockpull, blockcommit):
Document this.

Signed-off-by: Eric Blake <eblake redhat com>
---
 tools/virsh-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++----------
 tools/virsh.pod      | 42 ++++++++++++++++++------------
 2 files changed, 84 insertions(+), 31 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc1e554..594647a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1460,6 +1460,9 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
     goto cleanup;
 }

+
+/* Code common between blockpull, blockcommit, blockcopy, blockjob */
+
 typedef enum {
     VSH_CMD_BLOCK_JOB_ABORT,
     VSH_CMD_BLOCK_JOB_SPEED,
@@ -1467,6 +1470,56 @@ typedef enum {
     VSH_CMD_BLOCK_JOB_COMMIT,
 } vshCmdBlockJobMode;

+
+/* Parse a block job bandwidth parameter.  Returns -1 on error with
+ * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is
+ * set to non-zero.  */
+static int
+blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
+                  unsigned long *bandwidth)
+{
+    const char *value = NULL;
+    int ret = vshCommandOptString(cmd, "bandwidth", &value);
+    unsigned long long speed;
+
+    *bandwidth = 0;
+    if (ret <= 0)
+        return ret;
+
+    /* For back-compat, accept -1 as meaning unlimited, by converting
+     * negative values to 0 (and forbid wraparound back to positive).
+     * Alas, we have to parse the raw string ourselves, instead of
+     * using vshCommandOptUL.  If only we had never allowed wraparound
+     * on bandwidth.  */
+    if (strchr(value, '-')) {
+        unsigned long tmp;
+
+        if (vshCommandOptULWrap(cmd, "bandwidth", &tmp) < 0 ||
+            (long) tmp >= 0) {
+            vshError(ctl, _("could not parse bandwidth '%s'"), value);
+            return -1;
+        }
+        return 0;
+    }
+
+    /* Read a number in bytes/s, but with default unit of MiB/s if no
+     * unit was specified. */
+    if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024,
+                               (sizeof(*bandwidth) < sizeof(speed) ?
+                                ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) {
+        vshError(ctl, _("could not parse bandwidth '%s'"), value);
+        return -1;
+    }
+
+    /* Now scale it to MiB/s for the caller.  What a pain.  */
+    speed = VIR_DIV_UP(speed, 1024 * 1024);
+
+    /* We already checked that narrowing the type will fit.  */
+    *bandwidth = speed;
+    return !!speed;
+}
+
+
 static bool
 blockJobImpl(vshControl *ctl, const vshCmd *cmd,
              vshCmdBlockJobMode mode, virDomainPtr *pdom)
@@ -1485,10 +1538,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
     if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
         goto cleanup;

-    if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
-        vshError(ctl, "%s", _("bandwidth must be a number"));
+    if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
         goto cleanup;
-    }

     switch (mode) {
     case VSH_CMD_BLOCK_JOB_ABORT:
@@ -1610,7 +1661,7 @@ static const vshCmdOptDef opts_block_commit[] = {
     },
     {.name = "bandwidth",
      .type = VSH_OT_DATA,
-     .help = N_("bandwidth limit in MiB/s")
+     .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
     },
     {.name = "base",
      .type = VSH_OT_DATA,
@@ -1826,7 +1877,7 @@ static const vshCmdOptDef opts_block_copy[] = {
     },
     {.name = "bandwidth",
      .type = VSH_OT_DATA,
-     .help = N_("bandwidth limit in MiB/s")
+     .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
     },
     {.name = "shallow",
      .type = VSH_OT_BOOL,
@@ -1959,14 +2010,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         goto cleanup;

-    /* XXX: Parse bandwidth as scaled input, rather than forcing
-     * MiB/s, and either reject negative input or treat it as 0 rather
-     * than trying to guess which value will work well across both
-     * APIs with their different sizes and scales.  */
-    if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
-        vshError(ctl, "%s", _("bandwidth must be a number"));
+    if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
         goto cleanup;
-    }
     if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) {
         vshError(ctl, "%s", _("granularity must be a number"));
         goto cleanup;
@@ -2182,7 +2227,7 @@ static const vshCmdOptDef opts_block_job[] = {
     },
     {.name = "bandwidth",
      .type = VSH_OT_DATA,
-     .help = N_("set the Bandwidth limit in MiB/s")
+     .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
     },
     {.name = NULL}
 };
@@ -2341,7 +2386,7 @@ static const vshCmdOptDef opts_block_pull[] = {
     },
     {.name = "bandwidth",
      .type = VSH_OT_DATA,
-     .help = N_("bandwidth limit in MiB/s")
+     .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
     },
     {.name = "base",
      .type = VSH_OT_DATA,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 60ee515..7aba571 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -892,11 +892,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds
 to 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).
-I<bandwidth> specifies copying bandwidth limit in MiB/s, although for
-qemu, it may be non-zero only for an online domain. Specifying a negative
-value is interpreted as an unsigned long long value or essentially
-unlimited. The hypervisor can choose whether to reject the value or
-convert it to the maximum value allowed.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix.  It can be used to set a bandwidth limit for
+the commit job.  For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.

 =item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>]
 | I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>]
@@ -943,10 +944,13 @@ as fast as possible, otherwise the command may continue to block a little
 while longer until the job has actually cancelled.

 I<path> specifies fully-qualified path of the disk.
-I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative
-value is interpreted as an unsigned long long value that might be essentially
-unlimited, but more likely would overflow; it is safer to use 0 for that
-purpose.  Specifying I<granularity> allows fine-tuning of the granularity that
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix.  It can be used to set a bandwidth limit for
+the copy job.  For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.
+Specifying I<granularity> allows fine-tuning of the granularity that
 will be copied when a dirty region is detected; larger values trigger less
 I/O overhead but may end up copying more data overall (the default value is
 usually correct); this value must be a power of two.  Specifying I<buf-size>
@@ -983,10 +987,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds
 to 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).
-I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative
-value is interpreted as an unsigned long long value or essentially
-unlimited. The hypervisor can choose whether to reject the value or
-convert it to the maximum value allowed.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix.  It can be used to set a bandwidth limit for
+the pull job.  For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.

 =item B<blkdeviotune> I<domain> I<device>
 [[I<--config>] [I<--live>] | [I<--current>]]
@@ -1050,10 +1056,12 @@ not supply bytes/s resolution; when omitting the flag, raw output is
 listed in MiB/s and human-readable output automatically selects the
 best resolution supported by the server.

-I<bandwidth> can be used to set bandwidth limit for the active job.
-Specifying a negative value is interpreted as an unsigned long long
-value or essentially unlimited. The hypervisor can choose whether to
-reject the value or convert it to the maximum value allowed.
+I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to
+MiB/s if there is no suffix.  It can be used to set bandwidth limit for
+the active job.  For backward compatibility, a negative value is treated
+the same as 0 (unlimited); the actual upper limit accepted by the command
+may depend on the word size of both client and server, or by further limits
+of the hypervisor.

 =item B<blockresize> I<domain> I<path> I<size>

-- 
1.9.3


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