[libvirt] [PATCH v3 18/18] blockjob: allow finer bandwidth tuning for set speed

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. To work around the fact that 10-gigabit interfaces are possible but don't fit within 32 bits, the original interface took the number scaled as MiB/sec. But this scaling is rather coarse, and it might be nice to tune bandwidth finer than in megabyte chunks. Several of the block job calls that can set speed are fed through a common interface, so it was easier to adjust them all at once. Note that there is intentionally no flag for the new virDomainBlockCopy; there, since the API already uses a 64-bit type always, instead of a possible 32-bit type, and is brand new, it was easier to just avoid scaling issues. As with the previous patch that adjusted the query side, omitting the new flag preserves old behavior, and the documentation now mentions limits of what happens when a 32-bit machine is on either client or server side. * include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags) (virDomainBlockPullFlags) (VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES) (VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums. * src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull) (virDomainBlockRebase, virDomainBlockCommit): Document them. * src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed) (qemuDomainBlockPull, qemuDomainBlockRebase) (qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- include/libvirt/libvirt.h.in | 31 +++++++++++++---- src/libvirt.c | 83 +++++++++++++++++++++++++++++++------------- src/qemu/qemu_driver.c | 61 +++++++++++++++++++------------- 3 files changed, 118 insertions(+), 57 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aced31c..bffba06 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2617,11 +2617,24 @@ 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); +/* Flags for use with virDomainBlockJobSetSpeed */ +typedef enum { + VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES = 1 << 0, /* bandwidth in bytes/s + instead of MiB/s */ +} virDomainBlockJobSetSpeedFlags; -int virDomainBlockPull(virDomainPtr dom, const char *disk, - unsigned long bandwidth, unsigned int flags); +int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, + unsigned long bandwidth, unsigned int flags); + +/* Flags for use with virDomainBlockPull (values chosen to be a subset + * of the flags for virDomainBlockRebase) */ +typedef enum { + VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s + instead of MiB/s */ +} virDomainBlockPullFlags; + +int virDomainBlockPull(virDomainPtr dom, const char *disk, + unsigned long bandwidth, unsigned int flags); /** * virDomainBlockRebaseFlags: @@ -2640,11 +2653,13 @@ typedef enum { names */ VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = 1 << 5, /* Treat destination as block device instead of file */ + VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s + instead of MiB/s */ } virDomainBlockRebaseFlags; -int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); +int virDomainBlockRebase(virDomainPtr dom, const char *disk, + const char *base, unsigned long bandwidth, + unsigned int flags); /** * virDomainBlockCopyFlags: @@ -2719,6 +2734,8 @@ typedef enum { VIR_DOMAIN_BLOCK_COMMIT_RELATIVE = 1 << 3, /* keep the backing chain referenced using relative names */ + VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES = 1 << 4, /* bandwidth in bytes/s + instead of MiB/s */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, diff --git a/src/libvirt.c b/src/libvirt.c index 4806535..38bc1cb 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19724,11 +19724,20 @@ virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk, * virDomainBlockJobSetSpeed: * @dom: pointer to domain object * @disk: path to the block device, or device shorthand - * @bandwidth: specify bandwidth limit in MiB/s - * @flags: extra flags; not used yet, so callers should always pass 0 + * @bandwidth: specify bandwidth limit; flags determine the unit + * @flags: bitwise-OR of virDomainBlockJobSetSpeedFlags * * Set the maximimum allowable bandwidth that a block job may consume. If - * bandwidth is 0, the limit will revert to the hypervisor default. + * bandwidth is 0, the limit will revert to the hypervisor default of + * unlimited. + * + * If @flags contains VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES, @bandwidth + * is in bytes/second; otherwise, it is in MiB/second. Values larger than + * 2^52 bytes/sec may be rejected due to overflow considerations based on + * the word size of both client and server, and values larger than 2^31 + * bytes/sec may cause overflow problems if later queried by + * virDomainGetBlockJobInfo() without scaling. Hypervisors may further + * restrict the range of valid bandwidth values. * * The @disk parameter is either an unambiguous source name of the * block device (the <source file='...'/> sub-element, such as @@ -19776,8 +19785,8 @@ virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, * virDomainBlockPull: * @dom: pointer to domain object * @disk: path to the block device, or device shorthand - * @bandwidth: (optional) specify copy bandwidth limit in MiB/s - * @flags: extra flags; not used yet, so callers should always pass 0 + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit + * @flags: bitwise-OR of virDomainBlockPullFlags * * Populate a disk image with data from its backing image. Once all data from * its backing image has been pulled, the disk no longer depends on a backing @@ -19794,12 +19803,20 @@ virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * - * The maximum bandwidth (in MiB/s) that will be used to do the copy can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the copy can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * This is shorthand for virDomainBlockRebase() with a NULL base. * @@ -19844,7 +19861,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file - * @bandwidth: (optional) specify copy bandwidth limit in MiB/s + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * * Populate a disk image with data from its backing image chain, and @@ -19923,12 +19940,20 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * example, "vda[3]" refers to the backing store with index equal to "3" * in the chain of disk "vda". * - * The maximum bandwidth (in MiB/s) that will be used to do the copy can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the copy can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * When @base is NULL and @flags is 0, this is identical to * virDomainBlockPull(). When @flags contains VIR_DOMAIN_BLOCK_REBASE_COPY, @@ -20110,7 +20135,7 @@ virDomainBlockCopy(virDomainPtr dom, const char *disk, * or NULL for default * @top: path to file within backing chain that contains data to be merged, * or device shorthand, or NULL to merge all possible data - * @bandwidth: (optional) specify commit bandwidth limit in MiB/s + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockCommitFlags * * Commit changes that were made to temporary top-level files within a disk @@ -20188,12 +20213,20 @@ virDomainBlockCopy(virDomainPtr dom, const char *disk, * example, "vda[3]" refers to the backing store with index equal to "3" * in the chain of disk "vda". * - * The maximum bandwidth (in MiB/s) that will be used to do the commit can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the commit can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * Returns 0 if the operation has started, -1 on failure. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e597d68..2efcbcb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15125,14 +15125,19 @@ 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; + /* Convert bandwidth MiB to bytes, if needed */ + if ((mode == BLOCK_JOB_SPEED && + !(flags & VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES)) || + (mode == BLOCK_JOB_PULL && + !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES))) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto endjob; + } + speed <<= 20; } - speed <<= 20; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, @@ -15334,7 +15339,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15555,7 +15560,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, VIR_DOMAIN_BLOCK_REBASE_COPY | VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | VIR_DOMAIN_BLOCK_REBASE_RELATIVE | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV | + VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15579,14 +15585,16 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) dest->format = VIR_STORAGE_FILE_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; + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES)) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto cleanup; + } + speed <<= 20; } - 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 @@ -15692,7 +15700,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15737,7 +15745,8 @@ qemuDomainBlockCommit(virDomainPtr dom, /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | - VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1); + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | + VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15763,14 +15772,16 @@ 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; + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto endjob; + } + speed <<= 20; } - speed <<= 20; device = qemuDiskPathToAlias(vm, path, &idx); if (!device) -- 1.9.3