blockjob: hoist bandwidth scaling out of monitor code

qemu treats blockjob bandwidth as a 64-bit number, in the units
of bytes/second.  But we stupidly modeled block job bandwidth
after migration bandwidth, which in turn was an 'unsigned long'
and therefore subject to 32-bit vs. 64-bit interpretations, and
with a scale of MiB/s.  Our code already has to convert between
the two scales, and report overflow as appropriate; although
this conversion currently lives in the monitor code.  In fact,
our conversion code limited things to 63 bits, because we
checked against LLONG_MAX and reject what would be negative
bandwidth if treated as signed.

On the bright side, our use of MiB/s means that even with a
32-bit unsigned long, we still have no problem representing a
bandwidth of 2GiB/s, which is starting to be more feasible as
10-gigabit or even faster interfaces are used.  And once you
get past the physical speeds of existing interfaces, any larger
bandwidth number behaves the same - effectively unlimited.
But on the low side, the granularity of 1MiB/s tuning is rather
coarse.  So the new virDomainBlockJob API decided to go with
a direct 64-bit bytes/sec number instead of the scaled number
that prior blockjob APIs had used.  But there is no point in
rounding this number to MiB/s just to scale it back to bytes/s
for handing to qemu.

In order to make future code sharing possible between the old
virDomainBlockRebase and the new virDomainBlockCopy, this patch
moves the scaling and overflow detection into the driver code.
Several of the block job calls that can set speed are fed
through a common interface, so it was easier to adjust all block
jobs at once, for consistency.  This patch is just code motion;
there should be no user-visible change in behavior.

* src/qemu/qemu_monitor.h (qemuMonitorBlockJob)
(qemuMonitorBlockCommit, qemuMonitorDriveMirror): Change
parameter type and scale.
* src/qemu/qemu_monitor.c (qemuMonitorBlockJob)
(qemuMonitorBlockCommit, qemuMonitorDriveMirror): Move scaling
and overflow detection...
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl)
(qemuDomainBlockRebase, qemuDomainBlockCommit): ...here.
(qemuDomainBlockCopy): Use bytes/sec.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b893e38d59bca37415495de010e8ff93b0b48cf2..d69115a576b08ab80aa841e7c74ed59308af744a 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14989,6 +14989,8 @@ qemuDomainBlockPivot(virConnectPtr conn,
     return ret;
 }
 
+
+/* bandwidth in MiB/s per public API */
 static int
 qemuDomainBlockJobImpl(virDomainObjPtr vm,
                        virConnectPtr conn,
@@ -15011,6 +15013,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
     char *backingPath = NULL;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     bool save = false;
+    unsigned long long speed = bandwidth;
 
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -15122,9 +15125,18 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
         }
     }
 
+    /* Convert bandwidth MiB to bytes */
+    if (speed > LLONG_MAX >> 20) {
+        virReportError(VIR_ERR_OVERFLOW,
+                       _("bandwidth must be less than %llu"),
+                       LLONG_MAX >> 20);
+        goto endjob;
+    }
+    speed <<= 20;
+
     qemuDomainObjEnterMonitor(driver, vm);
     ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
-                              bandwidth, mode, async);
+                              speed, mode, async);
     qemuDomainObjExitMonitor(driver, vm);
     if (ret < 0) {
         if (mode == BLOCK_JOB_ABORT && disk->mirror)
@@ -15326,12 +15338,14 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
                                   BLOCK_JOB_SPEED, flags);
 }
 
+
+/* bandwidth in bytes/s */
 static int
 qemuDomainBlockCopy(virDomainObjPtr vm,
                     virConnectPtr conn,
                     const char *path,
                     const char *dest, const char *format,
-                    unsigned long bandwidth, unsigned int flags)
+                    unsigned long long bandwidth, unsigned int flags)
 {
     virQEMUDriverPtr driver = conn->privateData;
     qemuDomainObjPrivatePtr priv;
@@ -15514,6 +15528,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
     virDomainObjPtr vm;
     const char *format = NULL;
     int ret = -1;
+    unsigned long long speed = bandwidth;
 
     virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
                   VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
@@ -15537,6 +15552,15 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
     if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)
         format = "raw";
 
+    /* Convert bandwidth MiB to bytes */
+    if (speed > LLONG_MAX >> 20) {
+        virReportError(VIR_ERR_OVERFLOW,
+                       _("bandwidth must be less than %llu"),
+                       LLONG_MAX >> 20);
+        goto cleanup;
+    }
+    speed <<= 20;
+
     /* XXX: If we are doing a shallow copy but not reusing an external
      * file, we should attempt to pre-create the destination with a
      * relative backing chain instead of qemu's default of absolute */
@@ -15602,6 +15626,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
     char *basePath = NULL;
     char *backingPath = NULL;
     virStorageSourcePtr mirror = NULL;
+    unsigned long long speed = bandwidth;
 
     /* XXX Add support for COMMIT_DELETE */
     virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
@@ -15632,6 +15657,15 @@ qemuDomainBlockCommit(virDomainPtr dom,
         goto endjob;
     }
 
+    /* Convert bandwidth MiB to bytes */
+    if (speed > LLONG_MAX >> 20) {
+        virReportError(VIR_ERR_OVERFLOW,
+                       _("bandwidth must be less than %llu"),
+                       LLONG_MAX >> 20);
+        goto endjob;
+    }
+    speed <<= 20;
+
     device = qemuDiskPathToAlias(vm, path, &idx);
     if (!device)
         goto endjob;
@@ -15766,7 +15800,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
     qemuDomainObjEnterMonitor(driver, vm);
     ret = qemuMonitorBlockCommit(priv->mon, device,
                                  topPath, basePath, backingPath,
-                                 bandwidth);
+                                 speed);
     qemuDomainObjExitMonitor(driver, vm);
 
     if (mirror) {

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 44930512c766428fd51d2fa62b3adc70fa811cb8..ef35e6abb217c46956a9107875c71d735d9d946a 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3178,33 +3178,21 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
     return ret;
 }
 
-/* Start a drive-mirror block job.  bandwidth is in MiB/sec.  */
+/* Start a drive-mirror block job.  bandwidth is in bytes/sec.  */
 int
 qemuMonitorDriveMirror(qemuMonitorPtr mon,
                        const char *device, const char *file,
-                       const char *format, unsigned long bandwidth,
+                       const char *format, unsigned long long bandwidth,
                        unsigned int flags)
 {
     int ret = -1;
-    unsigned long long speed;
 
-    VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%ld, "
+    VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%lld, "
               "flags=%x",
               mon, device, file, NULLSTR(format), bandwidth, flags);
 
-    /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is
-     * limited to LLONG_MAX also for unsigned values */
-    speed = bandwidth;
-    if (speed > LLONG_MAX >> 20) {
-        virReportError(VIR_ERR_OVERFLOW,
-                       _("bandwidth must be less than %llu"),
-                       LLONG_MAX >> 20);
-        return -1;
-    }
-    speed <<= 20;
-
     if (mon->json)
-        ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed,
+        ret = qemuMonitorJSONDriveMirror(mon, device, file, format, bandwidth,
                                          flags);
     else
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -3228,33 +3216,22 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
     return ret;
 }
 
-/* Start a block-commit block job.  bandwidth is in MiB/sec.  */
+/* Start a block-commit block job.  bandwidth is in bytes/sec.  */
 int
 qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device,
                        const char *top, const char *base,
                        const char *backingName,
-                       unsigned long bandwidth)
+                       unsigned long long bandwidth)
 {
     int ret = -1;
-    unsigned long long speed;
 
-    VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, bandwidth=%lu",
+    VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, "
+              "bandwidth=%llu",
               mon, device, top, base, NULLSTR(backingName), bandwidth);
 
-    /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is
-     * limited to LLONG_MAX also for unsigned values */
-    speed = bandwidth;
-    if (speed > LLONG_MAX >> 20) {
-        virReportError(VIR_ERR_OVERFLOW,
-                       _("bandwidth must be less than %llu"),
-                       LLONG_MAX >> 20);
-        return -1;
-    }
-    speed <<= 20;
-
     if (mon->json)
         ret = qemuMonitorJSONBlockCommit(mon, device, top, base,
-                                         backingName, speed);
+                                         backingName, bandwidth);
     else
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("block-commit requires JSON monitor"));
@@ -3359,38 +3336,26 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
     return ret;
 }
 
-/* bandwidth is in MiB/sec */
+/* bandwidth is in bytes/sec */
 int
 qemuMonitorBlockJob(qemuMonitorPtr mon,
                     const char *device,
                     const char *base,
                     const char *backingName,
-                    unsigned long bandwidth,
+                    unsigned long long bandwidth,
                     qemuMonitorBlockJobCmd mode,
                     bool modern)
 {
     int ret = -1;
-    unsigned long long speed;
 
-    VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%luM, "
+    VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%lluB, "
               "mode=%o, modern=%d",
               mon, device, NULLSTR(base), NULLSTR(backingName),
               bandwidth, mode, modern);
 
-    /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is
-     * limited to LLONG_MAX also for unsigned values */
-    speed = bandwidth;
-    if (speed > LLONG_MAX >> 20) {
-        virReportError(VIR_ERR_OVERFLOW,
-                       _("bandwidth must be less than %llu"),
-                       LLONG_MAX >> 20);
-        return -1;
-    }
-    speed <<= 20;
-
     if (mon->json)
         ret = qemuMonitorJSONBlockJob(mon, device, base, backingName,
-                                      speed, mode, modern);
+                                      bandwidth, mode, modern);
     else
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("block jobs require JSON monitor"));

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index c66b77e7fe638f3379ddac1607729c6eb46de5cd..6000cbdab686995efc4bf999c164bd518d8a31b6 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -649,7 +649,7 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon,
                            const char *device,
                            const char *file,
                            const char *format,
-                           unsigned long bandwidth,
+                           unsigned long long bandwidth,
                            unsigned int flags)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 int qemuMonitorDrivePivot(qemuMonitorPtr mon,
@@ -663,7 +663,7 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon,
                            const char *top,
                            const char *base,
                            const char *backingName,
-                           unsigned long bandwidth)
+                           unsigned long long bandwidth)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
     ATTRIBUTE_NONNULL(4);
 bool qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon);
@@ -693,7 +693,7 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
                         const char *device,
                         const char *base,
                         const char *backingName,
-                        unsigned long bandwidth,
+                        unsigned long long bandwidth,
                         qemuMonitorBlockJobCmd mode,
                         bool modern)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);