11:02 p.m.
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(a)redhat.com>
---
include/libvirt/libvirt.h.in | 38 +++++++++++++++++++++++++-------------
src/libvirt.c | 8 ++++++--
src/qemu/qemu_driver.c | 20 ++++++++++++++++----
src/qemu/qemu_migration.c | 3 ++-
src/qemu/qemu_monitor.c | 8 +++++---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c | 19 ++++++++++++-------
src/qemu/qemu_monitor_json.h | 3 ++-
8 files changed, 70 insertions(+), 32 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index a64f597..6d0c042 100644
--- 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 5d8f01c..b00ee16 100644
--- 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 e86ebf5..1ad682f 100644
--- 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 9885a16..6bdee4e 100644
--- 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 ef35e6a..d96b1b8 100644
--- 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 35d9546..9012ad4 100644
--- 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);
int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 68ba084..8130f92 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3684,15 +3684,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",
@@ -3717,12 +3720,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",
@@ -3746,7 +3749,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;
@@ -3788,7 +3792,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 d3f6f37..4595eae 100644
--- 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);
int qemuMonitorJSONSetLink(qemuMonitorPtr mon,
--
1.9.3