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>

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,
+int virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk,
-                                 unsigned int flags);
+                             virDomainBlockJobInfoPtr info,
-int     virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk,
+                             unsigned int flags);
-                                 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
+ * 2^52 bytes/sec (a 32-bit MiB/s value) may be rejected on input due
- * overflow considerations, and hypervisors may further restrict the
+ * to overflow considerations (but do you really have an interface
- * set of valid values. Specifying 0 is the same as omitting this
+ * with that much bandwidth?), and values larger than 2^31 bytes/sec
- * parameter, to request no bandwidth limiting.  Some hypervisors may
+ * may cause overflow problems if queried in bytes/sec.  Hypervisors
- * lack support for this parameter, while still allowing a subsequent
+ * may further restrict the set of valid values. Specifying 0 is the
- * change of bandwidth via virDomainBlockJobSetSpeed().  The actual
+ * same as omitting this parameter, to request no bandwidth limiting.
- * speed can be determined with virDomainGetBlockJobInfo().
+ * 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"
 

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

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

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)

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"));

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,

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:

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,