blockjob: allow finer bandwidth tuning for query

While reviewing the new virDomainBlockCopy API, Peter Krempa
pointed out that our existing design of using MiB/s for block
job bandwidth is rather coarse, especially since qemu tracks
it in bytes/s; so virDomainBlockCopy only accepts bytes/s.
But once the new API is implemented for qemu, we will be in
the situation where it is possible to set a value that cannot
be accurately reflected back to the user, because the existing
virDomainGetBlockJobInfo defaults to the coarser units.

Fortunately, we have an escape hatch; and one that has already
served us well in the past: we can use the flags argument to
specify which scale to use (see virDomainBlockResize for prior
art).  This patch fixes the query side of the API; made easier
by previous patches that split the query side out from the
modification code.  Later patches will address the virsh
interface, as well retrofitting all other blockjob APIs to
also accept a flag for toggling bandwidth units.

* include/libvirt/libvirt.h.in (_virDomainBlockJobInfo)
(VIR_DOMAIN_BLOCK_COPY_BANDWIDTH): Document sizing issues.
(virDomainBlockJobInfoFlags): New enum.
* src/libvirt.c (virDomainGetBlockJobInfo): Document new flag.
* src/qemu/qemu_monitor.h (qemuMonitorBlockJobInfo): Add parameter.
* src/qemu/qemu_monitor.c (qemuMonitorBlockJobInfo): Likewise.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockJobInfo):
Likewise.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJobInfo)
(qemuMonitorJSONGetBlockJobInfoOne): Likewise. Don't scale here.
* src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Update
callers.
* src/qemu/qemu_driver.c (qemuDomainBlockPivot)
(qemuDomainBlockJobImpl): Likewise.
(qemuDomainGetBlockJobInfo): Likewise, and support new flag.

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

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index a64f597d39b544c6d9e319453cd3624b134ba41b..6d0c042dfde0e1bd385975e5655419480d919472 100644 (file)
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2585,13 +2585,23 @@ typedef enum {
     VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT = 1 << 1,
 } virDomainBlockJobAbortFlags;
 
+int virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
+                           unsigned int flags);
+
+/* Flags for use with virDomainGetBlockJobInfo */
+typedef enum {
+    VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES = 1 << 0, /* bandwidth in bytes/s
+                                                           instead of MiB/s */
+} virDomainBlockJobInfoFlags;
+
 /* An iterator for monitoring block job operations */
 typedef unsigned long long virDomainBlockJobCursor;
 
 typedef struct _virDomainBlockJobInfo virDomainBlockJobInfo;
 struct _virDomainBlockJobInfo {
     int type; /* virDomainBlockJobType */
-    unsigned long bandwidth;
+    unsigned long bandwidth; /* either bytes/s or MiB/s, according to flags */
+
     /*
      * The following fields provide an indication of block job progress.  @cur
      * indicates the current position and will be between 0 and @end.  @end is
@@ -2603,11 +2613,10 @@ struct _virDomainBlockJobInfo {
 };
 typedef virDomainBlockJobInfo *virDomainBlockJobInfoPtr;
 
-int       virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
-                                 unsigned int flags);
-int     virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk,
-                                 virDomainBlockJobInfoPtr info,
-                                 unsigned int flags);
+int virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk,
+                             virDomainBlockJobInfoPtr info,
+                             unsigned int flags);
+
 int    virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk,
                                  unsigned long bandwidth, unsigned int flags);
 
@@ -2653,13 +2662,16 @@ typedef enum {
  * the maximum bandwidth in bytes/s, and is used while getting the
  * copy operation into the mirrored phase, with a type of ullong.  For
  * compatibility with virDomainBlockJobSetSpeed(), values larger than
- * 2^52 bytes/sec (a 32-bit MiB/s value) may be rejected due to
- * overflow considerations, and hypervisors may further restrict the
- * set of valid values. Specifying 0 is the same as omitting this
- * parameter, to request no bandwidth limiting.  Some hypervisors may
- * lack support for this parameter, while still allowing a subsequent
- * change of bandwidth via virDomainBlockJobSetSpeed().  The actual
- * speed can be determined with virDomainGetBlockJobInfo().
+ * 2^52 bytes/sec (a 32-bit MiB/s value) may be rejected on input due
+ * to overflow considerations (but do you really have an interface
+ * with that much bandwidth?), and values larger than 2^31 bytes/sec
+ * may cause overflow problems if queried in bytes/sec.  Hypervisors
+ * may further restrict the set of valid values. Specifying 0 is the
+ * same as omitting this parameter, to request no bandwidth limiting.
+ * Some hypervisors may lack support for this parameter, while still
+ * allowing a subsequent change of bandwidth via
+ * virDomainBlockJobSetSpeed().  The actual speed can be determined
+ * with virDomainGetBlockJobInfo().
  */
 #define VIR_DOMAIN_BLOCK_COPY_BANDWIDTH "bandwidth"
 

diff --git a/src/libvirt.c b/src/libvirt.c
index 5d8f01c26ef52918429c5870b61834fc7e4d6a82..b00ee16760b431e2e8fe2e8c3b11f6ce64162a32 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -19667,10 +19667,14 @@ virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
  * @dom: pointer to domain object
  * @disk: path to the block device, or device shorthand
  * @info: pointer to a virDomainBlockJobInfo structure
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: bitwise-OR of virDomainBlockJobInfoFlags
  *
  * Request block job information for the given disk.  If an operation is active
- * @info will be updated with the current progress.
+ * @info will be updated with the current progress.  The units used for the
+ * bandwidth field of @info depends on @flags.  If @flags includes
+ * VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES, bandwidth is in bytes/second
+ * (although this mode can risk failure due to overflow, depending on both
+ * client and server word size); otherwise, the value is rounded up to MiB/s.
  *
  * The @disk parameter is either an unambiguous source name of the
  * block device (the <source file='...'/> sub-element, such as

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d69115a576b08ab80aa841e7c74ed59308af744a..16a89c85f70a62a415fa349e8ef9ae42c6a23345 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14857,7 +14857,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
     /* Probe the status, if needed.  */
     if (!disk->mirrorState) {
         qemuDomainObjEnterMonitor(driver, vm);
-        rc = qemuMonitorBlockJobInfo(priv->mon, device, &info);
+        rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL);
         qemuDomainObjExitMonitor(driver, vm);
         if (rc < 0)
             goto cleanup;
@@ -15180,7 +15180,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
                 virDomainBlockJobInfo dummy;
 
                 qemuDomainObjEnterMonitor(driver, vm);
-                ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy);
+                ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL);
                 qemuDomainObjExitMonitor(driver, vm);
 
                 if (ret <= 0)
@@ -15253,8 +15253,9 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
     int idx;
     virDomainDiskDefPtr disk;
     int ret = -1;
+    unsigned long long bandwidth;
 
-    virCheckFlags(0, -1);
+    virCheckFlags(VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES, -1);
 
     if (!(vm = qemuDomObjFromDomain(dom)))
         return -1;
@@ -15287,7 +15288,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
     disk = vm->def->disks[idx];
 
     qemuDomainObjEnterMonitor(driver, vm);
-    ret = qemuMonitorBlockJobInfo(priv->mon, device, info);
+    ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth);
     qemuDomainObjExitMonitor(driver, vm);
     if (ret < 0)
         goto endjob;
@@ -15295,6 +15296,17 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
     if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
         disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
         info->type = disk->mirrorJob;
+    if (bandwidth) {
+        if (!(flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES))
+            bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024);
+        info->bandwidth = bandwidth;
+        if (info->bandwidth != bandwidth) {
+            virReportError(VIR_ERR_OVERFLOW,
+                           _("bandwidth %llu cannot be represented in result"),
+                           bandwidth);
+            goto endjob;
+        }
+    }
 
     /* Snoop block copy operations, so future cancel operations can
      * avoid checking if pivot is safe.  Save the change to XML, but

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9885a166eebbf990a17154b166339229eaf67c0d..6bdee4ebe0f7470385dc6b9f4a179f0466d9b5b5 100644 (file)
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1307,7 +1307,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
                                _("canceled by client"));
                 goto error;
             }
-            mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info);
+            mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
+                                              NULL);
             qemuDomainObjExitMonitor(driver, vm);
 
             if (mon_ret < 0)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ef35e6abb217c46956a9107875c71d735d9d946a..d96b1b80cf7a2d2813524c58c7fc02e05478807e 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3366,14 +3366,16 @@ qemuMonitorBlockJob(qemuMonitorPtr mon,
 int
 qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
                         const char *device,
-                        virDomainBlockJobInfoPtr info)
+                        virDomainBlockJobInfoPtr info,
+                        unsigned long long *bandwidth)
 {
     int ret = -1;
 
-    VIR_DEBUG("mon=%p, device=%s, info=%p", mon, device, info);
+    VIR_DEBUG("mon=%p, device=%s, info=%p, bandwidth=%p",
+              mon, device, info, bandwidth);
 
     if (mon->json)
-        ret = qemuMonitorJSONBlockJobInfo(mon, device, info);
+        ret = qemuMonitorJSONBlockJobInfo(mon, device, info, bandwidth);
     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 6000cbdab686995efc4bf999c164bd518d8a31b6..ced198e36841c418420e861366caa8f7f76f54c6 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -700,7 +700,8 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
 
 int qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
                             const char *device,
-                            virDomainBlockJobInfoPtr info)
+                            virDomainBlockJobInfoPtr info,
+                            unsigned long long *bandwidth)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 int qemuMonitorOpenGraphics(qemuMonitorPtr mon,

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 85980f9bbbd1410603132ac9ae94625ec23aa550..4373ba2aafafb9103148e53c6dbe0475a11591e1 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3706,15 +3706,18 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon,
     return ret;
 }
 
-/* Returns -1 on error, 0 if not the right device, 1 if info was populated.  */
+/* Returns -1 on error, 0 if not the right device, 1 if info was
+ * populated.  However, rather than populate info->bandwidth (which
+ * might overflow on 32-bit machines), bandwidth is tracked optionally
+ * on the side.  */
 static int
 qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry,
                                   const char *device,
-                                  virDomainBlockJobInfoPtr info)
+                                  virDomainBlockJobInfoPtr info,
+                                  unsigned long long *bandwidth)
 {
     const char *this_dev;
     const char *type;
-    unsigned long long speed_bytes;
 
     if ((this_dev = virJSONValueObjectGetString(entry, "device")) == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3739,12 +3742,12 @@ qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry,
     else
         info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
 
-    if (virJSONValueObjectGetNumberUlong(entry, "speed", &speed_bytes) < 0) {
+    if (bandwidth &&
+        virJSONValueObjectGetNumberUlong(entry, "speed", bandwidth) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("entry was missing 'speed'"));
         return -1;
     }
-    info->bandwidth = speed_bytes / 1024ULL / 1024ULL;
 
     if (virJSONValueObjectGetNumberUlong(entry, "offset", &info->cur) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3768,7 +3771,8 @@ qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry,
 int
 qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
                             const char *device,
-                            virDomainBlockJobInfoPtr info)
+                            virDomainBlockJobInfoPtr info,
+                            unsigned long long *bandwidth)
 {
     virJSONValuePtr cmd = NULL;
     virJSONValuePtr reply = NULL;
@@ -3809,7 +3813,8 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
             ret = -1;
             goto cleanup;
         }
-        ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info);
+        ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info,
+                                                bandwidth);
     }
 
  cleanup:

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index ebcf8ae64561dfa8e05ed061c09bae69ab2d987c..a6d05b5d0822af65f11f5179e8e2055319eab073 100644 (file)
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -291,7 +291,8 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
 
 int qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
                                 const char *device,
-                                virDomainBlockJobInfoPtr info)
+                                virDomainBlockJobInfoPtr info,
+                                unsigned long long *bandwidth)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
 
 int qemuMonitorJSONSetLink(qemuMonitorPtr mon,