On 09/12/14 05:55, Eric Blake wrote: IIRC you added "VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE" so that some of the members of the <disk> structure can be omitted, so I presume you want to pass it here. ACK if you pass VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE or explain why not appropriately. Peter

Upstream qemu 1.4 added some drive-mirror tunables not present when it was first introduced in 1.3. Management apps may want to set these in some cases (for example, without tuning granularity down to sector size, a copy may end up occupying more bytes than the original because an entire cluster is copied even when only a sector within the cluster is dirty, although tuning it down results in more CPU time to do the copy). I haven't personally needed to use the parameters, but since they exist, and since the new API supports virTypedParams, we might as well expose them. Since the tuning parameters aren't often used, and omitted from the QMP command when unspecified, I think it is safe to rely on qemu 1.3 to issue an error about them being unsupported, rather than trying to create a new capability bit in libvirt. Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where a bad granularity (such as non-power-of-2) gives a poor message: error: internal error: unable to execute QEMU command 'drive-mirror': Invalid parameter 'drive-virtio-disk0' because of abuse of QERR_INVALID_PARAMETER (which is supposed to name the parameter that was given a bad value, rather than the value passed to some other parameter). I don't see that a capability check will help, so we'll just live with it (and it has since been improved in upstream qemu). * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add parameters. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise. (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers. * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise. * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 18 +++++------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 4 ++++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 2 +- 7 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6895b23..1673c7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15287,6 +15287,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, const char *path, virStorageSourcePtr mirror, unsigned long long bandwidth, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; @@ -15432,7 +15434,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15528,7 +15530,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT); ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest, - bandwidth, flags); + bandwidth, 0, 0, flags); vm = NULL; dest = NULL; @@ -15596,23 +15598,13 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, buf_size = param->value.ul; } } - if (granularity) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("granularity tuning not supported yet")); - goto cleanup; - } - if (buf_size) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("buffer size tuning not supported yet")); - goto cleanup; - } if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); vm = NULL; cleanup: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4819c04..ce1a5cd 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1461,7 +1461,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto error; mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, - NULL, speed, mirror_flags); + NULL, speed, 0, 0, mirror_flags); qemuDomainObjExitMonitor(driver, vm); if (mon_ret < 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 702404a..6059133 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3189,17 +3189,19 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long long bandwidth, + unsigned int granularity, unsigned long long buf_size, unsigned int flags) { int ret = -1; VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%lld, " - "flags=%x", - mon, device, file, NULLSTR(format), bandwidth, flags); + "granularity=%#x, buf_size=%lld, flags=%x", + mon, device, file, NULLSTR(format), bandwidth, granularity, + buf_size, flags); if (mon->json) ret = qemuMonitorJSONDriveMirror(mon, device, file, format, bandwidth, - flags); + granularity, buf_size, flags); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("drive-mirror requires JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ced198e..c32001d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -650,6 +650,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long long bandwidth, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 30f9ffb..0c4832a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3422,6 +3422,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) { int ret = -1; @@ -3434,6 +3436,8 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, "s:device", device, "s:target", file, "Y:speed", speed, + "z:granularity", granularity, + "P:buf-size", buf_size, "s:sync", shallow ? "top" : "full", "s:mode", reuse ? "existing" : "absolute-paths", "S:format", format, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a6d05b5..b5c61ca 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -249,6 +249,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e3fb4f7..afbf13a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1176,7 +1176,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") -GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, +GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) -- 1.9.3

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 (commit db33cc24), 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 94b942c..729bc82 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 941c518..f7e5a37 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19733,11 +19733,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 @@ -19785,8 +19794,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 @@ -19803,12 +19812,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. * @@ -19853,7 +19870,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 @@ -19932,12 +19949,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, @@ -20119,7 +20144,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 @@ -20197,12 +20222,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 1673c7b..92869c0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15052,14 +15052,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, @@ -15263,7 +15268,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; @@ -15482,7 +15487,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; @@ -15506,14 +15512,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 @@ -15619,7 +15627,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; @@ -15664,7 +15672,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; @@ -15690,14 +15699,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

Treating -1 as the maximum bandwidth of block jobs is not very practical, given the restrictions on overflow between 32-bit vs. 64-bit long, as well as conversions between MiB/s and bytes/s. We already document that 0 means unlimited, so the only reason to keep negative support is for back-compat. Meanwhile, it is a good idea to allow users to input in scales other than MiB/s. This patch just rounds back up to MiB/s, but by using a common helper, a later patch can then determine when a particular input scale will be better for using flags with the corresponding API call. * tools/virsh-domain.c (blockJobBandwidth): New function. (blockJobImpl, cmdBlockCopy): Use it. * tools/virsh.pod (blockjob, blockcopy, blockpull, blockcommit): Document this. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++---------- tools/virsh.pod | 42 ++++++++++++++++++------------ 2 files changed, 84 insertions(+), 31 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc1e554..594647a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1460,6 +1460,9 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto cleanup; } + +/* Code common between blockpull, blockcommit, blockcopy, blockjob */ + typedef enum { VSH_CMD_BLOCK_JOB_ABORT, VSH_CMD_BLOCK_JOB_SPEED, @@ -1467,6 +1470,56 @@ typedef enum { VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; + +/* Parse a block job bandwidth parameter. Returns -1 on error with + * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is + * set to non-zero. */ +static int +blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, + unsigned long *bandwidth) +{ + const char *value = NULL; + int ret = vshCommandOptString(cmd, "bandwidth", &value); + unsigned long long speed; + + *bandwidth = 0; + if (ret <= 0) + return ret; + + /* For back-compat, accept -1 as meaning unlimited, by converting + * negative values to 0 (and forbid wraparound back to positive). + * Alas, we have to parse the raw string ourselves, instead of + * using vshCommandOptUL. If only we had never allowed wraparound + * on bandwidth. */ + if (strchr(value, '-')) { + unsigned long tmp; + + if (vshCommandOptULWrap(cmd, "bandwidth", &tmp) < 0 || + (long) tmp >= 0) { + vshError(ctl, _("could not parse bandwidth '%s'"), value); + return -1; + } + return 0; + } + + /* Read a number in bytes/s, but with default unit of MiB/s if no + * unit was specified. */ + if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024, + (sizeof(*bandwidth) < sizeof(speed) ? + ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) { + vshError(ctl, _("could not parse bandwidth '%s'"), value); + return -1; + } + + /* Now scale it to MiB/s for the caller. What a pain. */ + speed = VIR_DIV_UP(speed, 1024 * 1024); + + /* We already checked that narrowing the type will fit. */ + *bandwidth = speed; + return !!speed; +} + + static bool blockJobImpl(vshControl *ctl, const vshCmd *cmd, vshCmdBlockJobMode mode, virDomainPtr *pdom) @@ -1485,10 +1538,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("bandwidth must be a number")); + if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) goto cleanup; - } switch (mode) { case VSH_CMD_BLOCK_JOB_ABORT: @@ -1610,7 +1661,7 @@ static const vshCmdOptDef opts_block_commit[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "base", .type = VSH_OT_DATA, @@ -1826,7 +1877,7 @@ static const vshCmdOptDef opts_block_copy[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "shallow", .type = VSH_OT_BOOL, @@ -1959,14 +2010,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; - /* XXX: Parse bandwidth as scaled input, rather than forcing - * MiB/s, and either reject negative input or treat it as 0 rather - * than trying to guess which value will work well across both - * APIs with their different sizes and scales. */ - if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("bandwidth must be a number")); + if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) goto cleanup; - } if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) { vshError(ctl, "%s", _("granularity must be a number")); goto cleanup; @@ -2182,7 +2227,7 @@ static const vshCmdOptDef opts_block_job[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("set the Bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = NULL} }; @@ -2341,7 +2386,7 @@ static const vshCmdOptDef opts_block_pull[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "base", .type = VSH_OT_DATA, diff --git a/tools/virsh.pod b/tools/virsh.pod index 60ee515..7aba571 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -892,11 +892,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). -I<bandwidth> specifies copying bandwidth limit in MiB/s, although for -qemu, it may be non-zero only for an online domain. Specifying a negative -value is interpreted as an unsigned long long value or essentially -unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the commit job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>] | I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>] @@ -943,10 +944,13 @@ as fast as possible, otherwise the command may continue to block a little while longer until the job has actually cancelled. I<path> specifies fully-qualified path of the disk. -I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative -value is interpreted as an unsigned long long value that might be essentially -unlimited, but more likely would overflow; it is safer to use 0 for that -purpose. Specifying I<granularity> allows fine-tuning of the granularity that +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the copy job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. +Specifying I<granularity> allows fine-tuning of the granularity that will be copied when a dirty region is detected; larger values trigger less I/O overhead but may end up copying more data overall (the default value is usually correct); this value must be a power of two. Specifying I<buf-size> @@ -983,10 +987,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). -I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative -value is interpreted as an unsigned long long value or essentially -unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the pull job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blkdeviotune> I<domain> I<device> [[I<--config>] [I<--live>] | [I<--current>]] @@ -1050,10 +1056,12 @@ not supply bytes/s resolution; when omitting the flag, raw output is listed in MiB/s and human-readable output automatically selects the best resolution supported by the server. -I<bandwidth> can be used to set bandwidth limit for the active job. -Specifying a negative value is interpreted as an unsigned long long -value or essentially unlimited. The hypervisor can choose whether to -reject the value or convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set bandwidth limit for +the active job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blockresize> I<domain> I<path> I<size> -- 1.9.3

Trickier than I had hoped; but now that we can parse numbers with arbitrary scale, it's time to use those numbers to call into the new API if the old API would have rounded the input, while still gracefully falling back to the old API if the new one fails. * tools/virsh-domain.c (blockJobBandwidth): Add parameter, adjust return value. (blockJobImpl, cmdBlockCopy): Adjust callers. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 154 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 113 insertions(+), 41 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 594647a..14d6921 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1471,18 +1471,29 @@ typedef enum { } vshCmdBlockJobMode; -/* Parse a block job bandwidth parameter. Returns -1 on error with - * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is - * set to non-zero. */ +/* Parse a block job bandwidth parameter. Caller must provide exactly + * one of @bandwidth or @bandwidthBytes. Returns -1 on error with + * message printed, 0 if no bandwidth needed, 1 if *bandwidth is set + * to non-zero MiB/s, and 2 if *bandwidth or *bandwidthBytes is + * non-zero bytes/s. */ static int blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, - unsigned long *bandwidth) + unsigned long *bandwidth, unsigned long long *bandwidthBytes) { const char *value = NULL; int ret = vshCommandOptString(cmd, "bandwidth", &value); unsigned long long speed; + unsigned long long limit = ULLONG_MAX; + + if (bandwidth) { + *bandwidth = 0; + bandwidthBytes = &speed; + if (sizeof(*bandwidth) < sizeof(speed)) + limit = ULONG_MAX << 20ULL; + } else { + *bandwidthBytes = 0; + } - *bandwidth = 0; if (ret <= 0) return ret; @@ -1504,19 +1515,29 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, /* Read a number in bytes/s, but with default unit of MiB/s if no * unit was specified. */ - if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024, - (sizeof(*bandwidth) < sizeof(speed) ? - ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) { + if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, 1024 * 1024, + limit) < 0) { vshError(ctl, _("could not parse bandwidth '%s'"), value); return -1; } + if (!bandwidth) /* bandwidthBytes is already correct */ + return 2; - /* Now scale it to MiB/s for the caller. What a pain. */ - speed = VIR_DIV_UP(speed, 1024 * 1024); + /* If the number fits in unsigned long, is not a multiple of + * MiB/s, and we haven't proven the new API flag will fail, then + * let the caller try it directly. Otherwise, scale it to MiB/s + * for the caller. What a pain. */ + if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) && + !ctl->blockJobNoBytes) { + ret = 2; + } else { + speed = VIR_DIV_UP(speed, 1024 * 1024); + ret = 1; + } /* We already checked that narrowing the type will fit. */ *bandwidth = speed; - return !!speed; + return ret; } @@ -1527,10 +1548,12 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, virDomainPtr dom = NULL; const char *path; unsigned long bandwidth = 0; + int bandwidthScale; bool ret = false; const char *base = NULL; const char *top = NULL; unsigned int flags = 0; + int rc; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -1538,7 +1561,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) + if ((bandwidthScale = blockJobBandwidth(ctl, cmd, &bandwidth, NULL)) < 0) goto cleanup; switch (mode) { @@ -1551,7 +1574,19 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; break; case VSH_CMD_BLOCK_JOB_SPEED: - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; + rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); + } + if (rc < 0) goto cleanup; break; case VSH_CMD_BLOCK_JOB_PULL: @@ -1559,15 +1594,28 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; + /* This flag intentionally has the same value for both APIs */ + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + if (base || flags) + rc = virDomainBlockRebase(dom, path, base, bandwidth, flags); + else + rc = virDomainBlockPull(dom, path, bandwidth, flags); - if (base || flags) { - if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) - goto cleanup; - } else { - if (virDomainBlockPull(dom, path, bandwidth, 0) < 0) - goto cleanup; + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + if (base || flags) + rc = virDomainBlockRebase(dom, path, base, bandwidth, flags); + else + rc = virDomainBlockPull(dom, path, bandwidth, flags); } - + if (rc < 0) + goto cleanup; break; case VSH_CMD_BLOCK_JOB_COMMIT: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || @@ -1583,7 +1631,19 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; - if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES; + rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); + } + if (rc < 0) goto cleanup; break; } @@ -1944,7 +2004,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *dest = NULL; const char *format = NULL; - unsigned long bandwidth = 0; unsigned int granularity = 0; unsigned long long buf_size = 0; unsigned int flags = 0; @@ -2010,8 +2069,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; - if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) - goto cleanup; if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) { vshError(ctl, "%s", _("granularity must be a number")); goto cleanup; @@ -2040,22 +2097,18 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) { /* New API */ - if (bandwidth || granularity || buf_size) { + unsigned long long bandwidthBytes = 0; + if (blockJobBandwidth(ctl, cmd, NULL, &bandwidthBytes) < 0) + goto cleanup; + + if (bandwidthBytes || granularity || buf_size) { params = vshCalloc(ctl, 3, sizeof(*params)); - if (bandwidth) { - /* bandwidth is ulong MiB/s, but the typed parameter is - * ullong bytes/s; make sure we don't overflow */ - if (bandwidth + 0ULL > ULLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - ULLONG_MAX >> 20); - } - if (virTypedParameterAssign(&params[nparams++], - VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, - VIR_TYPED_PARAM_ULLONG, - bandwidth << 20ULL) < 0) - goto cleanup; - } + if (bandwidthBytes && + virTypedParameterAssign(&params[nparams++], + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + bandwidthBytes) < 0) + goto cleanup; if (granularity && virTypedParameterAssign(&params[nparams++], VIR_DOMAIN_BLOCK_COPY_GRANULARITY, @@ -2088,14 +2141,33 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (virDomainBlockCopy(dom, path, xmlstr, params, nparams, flags) < 0) goto cleanup; } else { - /* Old API */ + /* Old API, parse scaled bandwidth */ + int rc; + unsigned long bandwidth = 0; + int bandwidthScale; + + if ((bandwidthScale = blockJobBandwidth(ctl, cmd, &bandwidth, + NULL)) < 0) + goto cleanup; flags |= VIR_DOMAIN_BLOCK_REBASE_COPY; if (vshCommandOptBool(cmd, "blockdev")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; if (STREQ_NULLABLE(format, "raw")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; - if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) + rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); + } + if (rc < 0) goto cleanup; } -- 1.9.3

Take the previous patch one step further. In addition to automatically selecting byte mode with fallback if the user's result would be rounded, we also want to give the user a way to guarantee byte mode even if rounding is not required, with no fallback if the remote side doesn't support byte mode. This way, virsh can be used to more fully test the impact of setting the bytes flag. * tools/virsh-domain.c (blockJobBandwidth): Adjust return type and check for --bytes flag. (cmdBlockCommit, cmdBlockPull, cmdBlockJob): Add --bytes flag. (cmdBlockCopy): Likewise, and adjust caller. (blockJobImpl): Adjust caller. * tools/virsh.pod (blockcommit, blockcopy, blockpull, blockjob): Document this. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 57 ++++++++++++++++++++++++++++++++++------------------ tools/virsh.pod | 48 +++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 14d6921..c6ed90e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1474,14 +1474,16 @@ typedef enum { /* Parse a block job bandwidth parameter. Caller must provide exactly * one of @bandwidth or @bandwidthBytes. Returns -1 on error with * message printed, 0 if no bandwidth needed, 1 if *bandwidth is set - * to non-zero MiB/s, and 2 if *bandwidth or *bandwidthBytes is - * non-zero bytes/s. */ + * to non-zero MiB/s, 2 if *bandwidth or *bandwidthBytes is non-zero + * bytes/s but fallback to MiB/s is okay, and 3 if *bandwidth or + * *bandwidthBytes is non-zero bytes/s and fallback is not okay. */ static int blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, unsigned long *bandwidth, unsigned long long *bandwidthBytes) { const char *value = NULL; int ret = vshCommandOptString(cmd, "bandwidth", &value); + bool bytes = vshCommandOptBool(cmd, "bytes"); unsigned long long speed; unsigned long long limit = ULLONG_MAX; @@ -1489,7 +1491,7 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, *bandwidth = 0; bandwidthBytes = &speed; if (sizeof(*bandwidth) < sizeof(speed)) - limit = ULONG_MAX << 20ULL; + limit = ULONG_MAX << (bytes ? 0 : 20ULL); } else { *bandwidthBytes = 0; } @@ -1515,20 +1517,23 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, /* Read a number in bytes/s, but with default unit of MiB/s if no * unit was specified. */ - if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, 1024 * 1024, - limit) < 0) { + if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, + bytes ? 1 : 1024 * 1024, limit) < 0) { vshError(ctl, _("could not parse bandwidth '%s'"), value); return -1; } if (!bandwidth) /* bandwidthBytes is already correct */ - return 2; + return 3; - /* If the number fits in unsigned long, is not a multiple of - * MiB/s, and we haven't proven the new API flag will fail, then - * let the caller try it directly. Otherwise, scale it to MiB/s - * for the caller. What a pain. */ - if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) && - !ctl->blockJobNoBytes) { + /* If the caller did not force bytes, the number fits in unsigned + * long, the number is not a multiple of MiB/s, and we haven't + * proven the new API flag will fail, then let the caller try it + * directly. Otherwise, scale it to MiB/s for the caller. What a + * pain. */ + if (bytes) { + ret = 3; + } else if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) && + !ctl->blockJobNoBytes) { ret = 2; } else { speed = VIR_DIV_UP(speed, 1024 * 1024); @@ -1577,7 +1582,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (bandwidthScale > 1) flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1602,7 +1607,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, else rc = virDomainBlockPull(dom, path, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1634,7 +1639,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (bandwidthScale > 1) flags |= VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES; rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1723,6 +1728,10 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "base", .type = VSH_OT_DATA, .help = N_("path of base file to commit into (default bottom of chain)") @@ -1939,6 +1948,10 @@ static const vshCmdOptDef opts_block_copy[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "shallow", .type = VSH_OT_BOOL, .help = N_("make the copy share a backing chain") @@ -2158,7 +2171,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -2291,7 +2304,8 @@ static const vshCmdOptDef opts_block_job[] = { }, {.name = "bytes", .type = VSH_OT_BOOL, - .help = N_("with --info, get bandwidth in bytes rather than MiB/s") + .help = N_("with --info or --bandwidth, treat bandwidth in bytes " + "rather than MiB/s") }, {.name = "raw", .type = VSH_OT_BOOL, @@ -2343,9 +2357,8 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) _("conflict between abort, info, and bandwidth modes")); return false; } - /* XXX also support --bytes with bandwidth mode */ - if (bytes && (abortMode || bandwidth)) { - vshError(ctl, "%s", _("--bytes requires info mode")); + if (bytes && abortMode) { + vshError(ctl, "%s", _("--bytes not compatible with abort mode")); return false; } @@ -2460,6 +2473,10 @@ static const vshCmdOptDef opts_block_pull[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "base", .type = VSH_OT_DATA, .help = N_("path of backing file in chain for a partial pull") diff --git a/tools/virsh.pod b/tools/virsh.pod index 7aba571..c9b4c31 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -847,7 +847,7 @@ currently in use by a running domain. Other contexts that require a MAC address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. -=item B<blockcommit> I<domain> I<path> [I<bandwidth>] +=item B<blockcommit> I<domain> I<path> [I<bandwidth> [I<--bytes>]] [I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>] [I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>] [I<--active>] [{I<--pivot> | I<--keep-overlay>}] @@ -893,14 +893,14 @@ to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the commit job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the commit job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>] -| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>] +| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth> [I<--bytes>]] [I<--wait> [I<--async>] [I<--verbose>]] [{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] [I<granularity>] [I<buf-size>] @@ -945,11 +945,11 @@ while longer until the job has actually cancelled. I<path> specifies fully-qualified path of the disk. I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the copy job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the copy job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. Specifying I<granularity> allows fine-tuning of the granularity that will be copied when a dirty region is detected; larger values trigger less I/O overhead but may end up copying more data overall (the default value is @@ -958,7 +958,7 @@ will control how much data can be simultaneously in-flight during the copy; larger values use more memory but may allow faster completion (the default value is usually correct). -=item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] +=item B<blockpull> I<domain> I<path> [I<bandwidth> [I<--bytes>]] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] [I<--keep-relative>] @@ -988,11 +988,11 @@ to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the pull job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the pull job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blkdeviotune> I<domain> I<device> [[I<--config>] [I<--live>] | [I<--current>]] @@ -1029,7 +1029,7 @@ exclusive. If no flag is specified, behavior is different depending on hypervisor. =item B<blockjob> I<domain> I<path> { [I<--abort>] [I<--async>] [I<--pivot>] | -[I<--info>] [I<--raw>] [I<--bytes>] | [I<bandwidth>] } +[I<--info>] [I<--raw>] | [I<bandwidth>] } [I<--bytes>] Manage active block operations. There are three mutually-exclusive modes: I<--info>, I<bandwidth>, and I<--abort>. I<--async> and I<--pivot> imply @@ -1057,11 +1057,11 @@ listed in MiB/s and human-readable output automatically selects the best resolution supported by the server. I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set bandwidth limit for -the active job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set bandwidth limit for the active job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blockresize> I<domain> I<path> I<size> -- 1.9.3