On Sat, Aug 30, 2014 at 22:02:19 -0600, Eric Blake wrote: ACK, we need this in 1.2.8. Jirka

The existing virDomainBlockRebase code rejected the combination of _RELATIVE and _COPY flags, but only by accident. It makes sense, at least for the case of _SHALLOW and not _REUSE_EXT, but to implement it, libvirt would have to pre-create the file with a relative backing name. Meanwhile, the code to forward on to the block copy code is getting longer, and reorganising the function to have the block pull done early makes it easier to add even more block copy prep code. This patch should have no semantic difference other than the quality of the error message on the unsupported flag combination. * src/qemu/qemu_driver.c (qemuDomainBlockRebase): Reorder code, and improve error message of relative copy. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 47 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..177d3e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15467,6 +15467,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; + const char *format = NULL; + int ret = -1; virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | @@ -15477,22 +15479,37 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, if (!(vm = qemuDomObjFromDomain(dom))) return -1; - if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0) { + if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + /* For normal rebase (enhanced blockpull), the common code handles + * everything, including vm cleanup. */ + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) + return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, + NULL, BLOCK_JOB_PULL, flags); + + /* If we got here, we are doing a block copy rebase. */ + if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) + format = "raw"; + + /* XXX: If we are doing a shallow copy but not reusing an external + * file, we should attempt to pre-create the destination with a + * relative backing chain instead of qemu's default of absolute */ + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Relative backing during copy not supported yet")); + goto cleanup; + } + + flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT); + ret = qemuDomainBlockCopy(vm, dom->conn, path, base, format, + bandwidth, flags); + vm = NULL; + cleanup: + if (vm) virObjectUnlock(vm); - return -1; - } - - if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY) { - const char *format = NULL; - if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) - format = "raw"; - flags &= ~(VIR_DOMAIN_BLOCK_REBASE_COPY | - VIR_DOMAIN_BLOCK_REBASE_COPY_RAW); - return qemuDomainBlockCopy(vm, dom->conn, path, base, format, bandwidth, flags); - } - - return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, NULL, - BLOCK_JOB_PULL, flags); + return ret; } static int -- 1.9.3

On 09/04/14 16:40, Peter Krempa wrote: Okay. Now I see that you are adding other stuff with "goto cleanup" at this place so this makes sense. This is then okay as-is without any change.

The qemu implementation for virDomainGetBlockJobInfo() has a minor bug: it grabs the qemu job with intent to QEMU_JOB_MODIFY, which means it cannot be run in parallel with any other domain-modifying command. Among others, virDomainBlockJobAbort() is such a modifying command, and it defaults to being synchronous, and can wait as long as several seconds to ensure that the job has actually finished. Due to the job rules, this means a user cannot obtain status about the job during that timeframe, even though we know management code is using a polling loop on status to see when a job finishes. This bug has been present ever since blockpull support was first introduced (commit b976165, v0.9.4 in Jul 2011), all because we stupidly tried to cram too much multiplexing through a single helper routine. It's time to disentangle some of that mess, and in the process relax block job query to use QEMU_JOB_QUERY, since it can safely be used in parallel with any long running modify command. Technically, there is one case where getting block job info can modify domain XML - we do snooping to see if a 2-phase job has transitioned into the second phase, for an optimization in the case of old qemu that lacked an event for the transition. But I think that optimization is safe; and if it proves to be a problem, we could use the difference between QEMU_CAPS_BLOCKJOB_{ASYNC,SYNC} to determine whether we even need snooping, and request a modifying job in that case. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Move info handling... (qemuDomainGetBlockJobInfo): ...here, and relax job type. (qemuDomainBlockJobAbort, qemuDomainBlockJobSetSpeed) (qemuDomainBlockRebase, qemuDomainBlockPull): Adjust callers. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 177d3e4..bedccc6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14994,7 +14994,7 @@ static int qemuDomainBlockJobImpl(virDomainObjPtr vm, virConnectPtr conn, const char *path, const char *base, - unsigned long bandwidth, virDomainBlockJobInfoPtr info, + unsigned long bandwidth, int mode, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; @@ -15125,25 +15125,14 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, - bandwidth, info, mode, async); + bandwidth, NULL, mode, async); qemuDomainObjExitMonitor(driver, vm); - if (info && info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && - disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) - info->type = disk->mirrorJob; if (ret < 0) { if (mode == BLOCK_JOB_ABORT && disk->mirror) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; goto endjob; } - /* Snoop block copy operations, so future cancel operations can - * avoid checking if pivot is safe. */ - if (mode == BLOCK_JOB_INFO && ret == 1 && disk->mirror && - info->cur == info->end && !disk->mirrorState) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; - save = true; - } - waitjob: /* If we have made changes to XML due to a copy job, make a best * effort to save it now. But we can ignore failure, since there @@ -15239,15 +15228,22 @@ qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags) return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, 0, NULL, BLOCK_JOB_ABORT, - flags); + return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, 0, + BLOCK_JOB_ABORT, flags); } static int qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, virDomainBlockJobInfoPtr info, unsigned int flags) { + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; + char *device = NULL; + int idx; + virDomainDiskDefPtr disk; + int ret = -1; + virCheckFlags(0, -1); if (!(vm = qemuDomObjFromDomain(dom))) @@ -15258,8 +15254,58 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, 0, info, BLOCK_JOB_INFO, - flags); + priv = vm->privateData; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("block jobs not supported with this QEMU binary")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + device = qemuDiskPathToAlias(vm, path, &idx); + if (!device) + goto endjob; + disk = vm->def->disks[idx]; + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, + info, BLOCK_JOB_INFO, true); + qemuDomainObjExitMonitor(driver, vm); + if (ret < 0) + goto endjob; + + if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + info->type = disk->mirrorJob; + + /* Snoop block copy operations, so future cancel operations can + * avoid checking if pivot is safe. Save the change to XML, but + * we can ignore failure because it is only an optimization. */ + if (ret == 1 && disk->mirror && + info->cur == info->end && !disk->mirrorState) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; + ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm)); + virObjectUnref(cfg); + } + endjob: + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; } static int @@ -15277,7 +15323,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, NULL, + return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, BLOCK_JOB_SPEED, flags); } @@ -15486,7 +15532,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, - NULL, BLOCK_JOB_PULL, flags); + BLOCK_JOB_PULL, flags); /* If we got here, we are doing a block copy rebase. */ if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) @@ -15527,7 +15573,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, NULL, + return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, BLOCK_JOB_PULL, flags); } -- 1.9.3

On 09/04/2014 09:11 AM, Peter Krempa wrote: Here's what I squashed in. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index e7bd504..fb13169 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15289,7 +15289,9 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, /* Snoop block copy operations, so future cancel operations can * avoid checking if pivot is safe. Save the change to XML, but - * we can ignore failure because it is only an optimization. */ + * we can ignore failure because it is only an optimization. We + * hold the vm lock, so modifying the in-memory representation + * even though we are a query rather than a modify job, is safe. */ if (ret == 1 && disk->mirror && info->cur == info->end && !disk->mirrorState) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/31/14 06:02, Eric Blake wrote: ... Implementation of this function doesn't check for presence of the @info structure and dereferences it always. You should mark argument 3 nonnull too. ... This doesn't break the loop, thus if the device info isn't last in the structure you'd overwrite the return code. I presume the clients don't check that far but you've documented that semantics. ... Again, @info is required, thus should be marked ATTRIBUTE_NONNULL. Peter

qemu treats blockjob bandwidth as a 64-bit number, in the units of bytes/second. But 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, and with a scale of MiB/s. Our code already has to convert between the two scales, and report overflow as appropriate; although this conversion currently lives in the monitor code. On the bright side, our use of MiB/s means that even with a 32-bit unsigned long, we still have no problem representing a bandwidth of 2GiB/s, which is starting to be more feasible as 10-gigabit or even faster interfaces are used. And once you get past the physical speeds of existing interfaces, any larger bandwidth number behaves the same - effectively unlimited. But on the low side, the granularity of 1MiB/s tuning is rather coarse. So the new virDomainBlockJob API decided to go with a direct 64-bit bytes/sec number instead of the scaled number that prior blockjob APIs had used. But there is no point in rounding this number to MiB/s just to scale it back to bytes/s for handing to qemu. In order to make future code sharing possible between the old virDomainBlockRebase and the new virDomainBlockCopy, this patch moves the scaling and overflow detection into the driver code. Several of the block job calls that can set speed are fed through a common interface, so it was easier to adjust all block jobs at once, for consistency. * src/qemu/qemu_monitor.h (qemuMonitorBlockJob) (qemuMonitorBlockCommit, qemuMonitorDriveMirror): Change parameter type and scale. * src/qemu/qemu_monitor.c (qemuMonitorBlockJob) (qemuMonitorBlockCommit, qemuMonitorDriveMirror): Move scaling and overflow detection... * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl) (qemuDomainBlockRebase, qemuDomainBlockCommit): ...here. (qemuDomainBlockCopy): Use bytes/sec. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 61 +++++++++++-------------------------------------- src/qemu/qemu_monitor.h | 6 ++--- 3 files changed, 53 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f208ba0..e86ebf5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14989,6 +14989,8 @@ qemuDomainBlockPivot(virConnectPtr conn, return ret; } + +/* bandwidth in MiB/s per public API */ static int qemuDomainBlockJobImpl(virDomainObjPtr vm, virConnectPtr conn, @@ -15011,6 +15013,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, char *backingPath = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; + unsigned long long speed = bandwidth; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -15122,9 +15125,18 @@ 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; + } + speed <<= 20; + qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, - bandwidth, mode, async); + speed, mode, async); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { if (mode == BLOCK_JOB_ABORT && disk->mirror) @@ -15324,12 +15336,14 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, BLOCK_JOB_SPEED, flags); } + +/* bandwidth in bytes/s */ static int qemuDomainBlockCopy(virDomainObjPtr vm, virConnectPtr conn, const char *path, const char *dest, const char *format, - unsigned long bandwidth, unsigned int flags) + unsigned long long bandwidth, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; qemuDomainObjPrivatePtr priv; @@ -15512,6 +15526,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, virDomainObjPtr vm; const char *format = NULL; int ret = -1; + unsigned long long speed = bandwidth; virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | @@ -15535,6 +15550,15 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) format = "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; + } + 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 * relative backing chain instead of qemu's default of absolute */ @@ -15600,6 +15624,7 @@ qemuDomainBlockCommit(virDomainPtr dom, char *basePath = NULL; char *backingPath = NULL; virStorageSourcePtr mirror = NULL; + unsigned long long speed = bandwidth; /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | @@ -15630,6 +15655,15 @@ 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; + } + speed <<= 20; + device = qemuDiskPathToAlias(vm, path, &idx); if (!device) goto endjob; @@ -15764,7 +15798,7 @@ qemuDomainBlockCommit(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockCommit(priv->mon, device, topPath, basePath, backingPath, - bandwidth); + speed); qemuDomainObjExitMonitor(driver, vm); if (mirror) { diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 4493051..ef35e6a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3178,33 +3178,21 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, return ret; } -/* Start a drive-mirror block job. bandwidth is in MiB/sec. */ +/* Start a drive-mirror block job. bandwidth is in bytes/sec. */ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, - const char *format, unsigned long bandwidth, + const char *format, unsigned long long bandwidth, unsigned int flags) { int ret = -1; - unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%ld, " + VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%lld, " "flags=%x", mon, device, file, NULLSTR(format), bandwidth, flags); - /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is - * limited to LLONG_MAX also for unsigned values */ - speed = bandwidth; - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - return -1; - } - speed <<= 20; - if (mon->json) - ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed, + ret = qemuMonitorJSONDriveMirror(mon, device, file, format, bandwidth, flags); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3228,33 +3216,22 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) return ret; } -/* Start a block-commit block job. bandwidth is in MiB/sec. */ +/* Start a block-commit block job. bandwidth is in bytes/sec. */ int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, const char *backingName, - unsigned long bandwidth) + unsigned long long bandwidth) { int ret = -1; - unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, bandwidth=%lu", + VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, " + "bandwidth=%llu", mon, device, top, base, NULLSTR(backingName), bandwidth); - /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is - * limited to LLONG_MAX also for unsigned values */ - speed = bandwidth; - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - return -1; - } - speed <<= 20; - if (mon->json) ret = qemuMonitorJSONBlockCommit(mon, device, top, base, - backingName, speed); + backingName, bandwidth); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block-commit requires JSON monitor")); @@ -3359,38 +3336,26 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, return ret; } -/* bandwidth is in MiB/sec */ +/* bandwidth is in bytes/sec */ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long bandwidth, + unsigned long long bandwidth, qemuMonitorBlockJobCmd mode, bool modern) { int ret = -1; - unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%luM, " + VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%lluB, " "mode=%o, modern=%d", mon, device, NULLSTR(base), NULLSTR(backingName), bandwidth, mode, modern); - /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is - * limited to LLONG_MAX also for unsigned values */ - speed = bandwidth; - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - return -1; - } - speed <<= 20; - if (mon->json) ret = qemuMonitorJSONBlockJob(mon, device, base, backingName, - speed, mode, modern); + bandwidth, mode, modern); 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 947ca34..35d9546 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -649,7 +649,7 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, - unsigned long bandwidth, + unsigned long long bandwidth, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, @@ -663,7 +663,7 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *top, const char *base, const char *backingName, - unsigned long bandwidth) + unsigned long long bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); bool qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon); @@ -693,7 +693,7 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long bandwidth, + unsigned long long bandwidth, qemuMonitorBlockJobCmd mode, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 1.9.3

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&gt; --- 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

Expose the new flag just added to virDomainGetBlockJobInfo. With --raw, the presence or absence of --bytes determines which flag to use in the single API call. Without --raw, the use of --bytes forces an error if the server doesn't support it, otherwise, the code tries to silently fall back to scaling the MiB/s value. My goal is to eventually also support --bytes in bandwidth mode; but that's a bit further down the road (and needs a new API flag added in libvirt.h first). This changes the human output, but the previous patch added raw output precisely so that we can have flexibility with the human output. For this commit, I used qemu-monitor-command to force an unusual bandwidth, but the same will be possible once qemu implements virDomainBlockCopy: Before: Block Copy: [100 %] Bandwidth limit: 2 MiB/s After: Block Copy: [100 %] Bandwidth limit: 1048577 bytes/s (1.000 MiB/s) The cache avoids having to repeatedly attempt a flag probe when talking to an older server, when multiple blockjob commands are issued during a batch session and the user is manually polling for job completion. * tools/virsh.h (_vshControl): Add a cache. * tools/virsh.c (cmdConnect, vshReconnect): Initialize the cache. * tools/virsh-domain.c (opts_block_job): Add --bytes. * tools/virsh.pod (blockjob): Document this. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.c | 2 ++ tools/virsh.h | 2 ++ tools/virsh.pod | 8 +++++-- 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b95e971..eb3ed15 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2048,6 +2048,10 @@ static const vshCmdOptDef opts_block_job[] = { .type = VSH_OT_BOOL, .help = N_("get active job information for the specified disk") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("with --info, get bandwidth in bytes rather than MiB/s") + }, {.name = "raw", .type = VSH_OT_BOOL, .help = N_("implies --info; output details rather than human summary") @@ -2080,8 +2084,9 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) { virDomainBlockJobInfo info; bool ret = false; - int rc; + int rc = -1; bool raw = vshCommandOptBool(cmd, "raw"); + bool bytes = vshCommandOptBool(cmd, "bytes"); bool abortMode = (vshCommandOptBool(cmd, "abort") || vshCommandOptBool(cmd, "async") || vshCommandOptBool(cmd, "pivot")); @@ -2090,12 +2095,18 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *path; unsigned int flags = 0; + unsigned long long speed; if (abortMode + infoMode + bandwidth > 1) { vshError(ctl, "%s", _("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")); + return false; + } if (abortMode) return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL); @@ -2110,9 +2121,45 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - rc = virDomainGetBlockJobInfo(dom, path, &info, flags); - if (rc < 0) - goto cleanup; + /* If bytes were requested, or if raw mode is not forcing MiB/s + * query and cache can't prove failure, then query bytes/sec. */ + if (bytes || !(raw || ctl->blockJobNoBytes)) { + flags |= VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES; + rc = virDomainGetBlockJobInfo(dom, path, &info, flags); + if (rc < 0) { + /* Check for particular errors, let all the rest be fatal. */ + switch (last_error->code) { + case VIR_ERR_INVALID_ARG: + ctl->blockJobNoBytes = true; + /* fallthrough */ + case VIR_ERR_OVERFLOW: + if (!bytes && !raw) { + /* try again with MiB/s, unless forcing bytes */ + vshResetLibvirtError(); + break; + } + /* fallthrough */ + default: + goto cleanup; + } + } + speed = info.bandwidth; + } + /* If we don't already have a query result, query for MiB/s */ + if (rc < 0) { + flags &= ~VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES; + if ((rc = virDomainGetBlockJobInfo(dom, path, &info, flags)) < 0) + goto cleanup; + speed = info.bandwidth; + /* Scale to bytes/s unless in raw mode */ + if (!raw) { + speed <<= 20; + if (speed >> 20 != info.bandwidth) + vshError(ctl, _("overflow in converting %ld MiB/s to bytes\n"), + info.bandwidth); + } + } + if (rc == 0) { if (!raw) vshPrint(ctl, _("No current block job for %s"), path); @@ -2127,9 +2174,12 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) } else { vshPrintJobProgress(vshDomainBlockJobToString(info.type), info.end - info.cur, info.end); - if (info.bandwidth != 0) - vshPrint(ctl, _(" Bandwidth limit: %lu MiB/s"), - info.bandwidth); + if (speed) { + const char *unit; + double val = vshPrettyCapacity(speed, &unit); + vshPrint(ctl, _(" Bandwidth limit: %llu bytes/s (%-.3lf %s/s)"), + speed, val, unit); + } vshPrint(ctl, "\n"); } ret = true; diff --git a/tools/virsh.c b/tools/virsh.c index 713c9a5..9706acc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -397,6 +397,7 @@ vshReconnect(vshControl *ctl) disconnected = 0; ctl->useGetInfo = false; ctl->useSnapshotOld = false; + ctl->blockJobNoBytes = false; } @@ -454,6 +455,7 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) ctl->useGetInfo = false; ctl->useSnapshotOld = false; + ctl->blockJobNoBytes = false; ctl->readonly = ro; ctl->conn = vshConnect(ctl, ctl->name, ctl->readonly); diff --git a/tools/virsh.h b/tools/virsh.h index b4df24b..7d5d8a2 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -238,6 +238,8 @@ struct _vshControl { virDomainGetState is not supported */ bool useSnapshotOld; /* cannot use virDomainSnapshotGetParent or virDomainSnapshotNumChildren */ + bool blockJobNoBytes; /* true if _BANDWIDTH_BYTE blockjob flags + are missing */ virThread eventLoop; virMutex lock; bool eventLoopStarted; diff --git a/tools/virsh.pod b/tools/virsh.pod index 3f1bf7e..406de5c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1013,7 +1013,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<bandwidth>] } +[I<--info>] [I<--raw>] [I<--bytes>] | [I<bandwidth>] } Manage active block operations. There are three mutually-exclusive modes: I<--info>, I<bandwidth>, and I<--abort>. I<--async> and I<--pivot> imply @@ -1034,7 +1034,11 @@ commit job be pivoted over to the new image. In I<--info> mode, the active job information on the specified disk will be printed. By default, the output is a single human-readable summary line; this format may change in future versions. Adding -I<--raw> lists each field of the struct, in a stable format. +I<--raw> lists each field of the struct, in a stable format. If the +I<--bytes> flag is set, then the command errors out if the server could +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 -- 1.9.3

On 09/05/2014 03:05 AM, Peter Krempa wrote: the new flag hinted at is patch 14/14; I'm still working on the code for a v4 patch for actually setting in byte mode. Pushed with a tweak I found during self-review: diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c index 749fa67..2bb3bbc 100644 --- i/tools/virsh-domain.c +++ w/tools/virsh-domain.c @@ -2121,7 +2121,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - /* If bytes were requested, or if raw mode is not forcing MiB/s + /* If bytes were requested, or if raw mode is not forcing a MiB/s * query and cache can't prove failure, then query bytes/sec. */ if (bytes || !(raw || ctl->blockJobNoBytes)) { flags |= VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES; @@ -2154,9 +2154,11 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) /* Scale to bytes/s unless in raw mode */ if (!raw) { speed <<= 20; - if (speed >> 20 != info.bandwidth) + if (speed >> 20 != info.bandwidth) { vshError(ctl, _("overflow in converting %ld MiB/s to bytes\n"), info.bandwidth); + goto cleanup; + } } } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

I'm about to extend the capabilities of blockcopy. Hiding a few common lines of implementation gets in the way of the new required logic, and putting the new logic in the common implementation won't benefit any of the other blockjob operations. Therefore, it is simpler to just do the work inline. There should be no semantic change in this patch. * tools/virsh-domain.c (blockJobImpl): Drop unused variable. Move block copy guts... (cmdBlockCopy): ...into their lone caller. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 813d8e0..b92b2eb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1464,7 +1464,6 @@ typedef enum { VSH_CMD_BLOCK_JOB_ABORT, VSH_CMD_BLOCK_JOB_SPEED, VSH_CMD_BLOCK_JOB_PULL, - VSH_CMD_BLOCK_JOB_COPY, VSH_CMD_BLOCK_JOB_COMMIT, } vshCmdBlockJobMode; @@ -1536,21 +1535,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) goto cleanup; break; - case VSH_CMD_BLOCK_JOB_COPY: - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY; - if (vshCommandOptBool(cmd, "shallow")) - flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; - if (vshCommandOptBool(cmd, "reuse-external")) - flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; - if (vshCommandOptBool(cmd, "raw")) - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; - if (vshCommandOptBool(cmd, "blockdev")) - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; - if (vshCommandOptStringReq(ctl, cmd, "dest", &base) < 0) - goto cleanup; - if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) - goto cleanup; - break; } ret = true; @@ -1892,6 +1876,9 @@ static bool cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; + const char *dest; + unsigned long bandwidth = 0; + unsigned int flags = VIR_DOMAIN_BLOCK_REBASE_COPY; bool ret = false; bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); @@ -1907,6 +1894,9 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + return false; + blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; if (blocking) { if (pivot && finish) { @@ -1915,8 +1905,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; - if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -1935,7 +1923,26 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) return false; } - if (!blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_COPY, &dom)) + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { + vshError(ctl, "%s", _("bandwidth must be a number")); + goto cleanup; + } + + if (vshCommandOptBool(cmd, "shallow")) + flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; + if (vshCommandOptBool(cmd, "reuse-external")) + flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; + if (vshCommandOptBool(cmd, "raw")) + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; + if (vshCommandOptBool(cmd, "blockdev")) + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; + if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) + goto cleanup; + + if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) goto cleanup; if (!blocking) { -- 1.9.3

On 09/05/14 11:29, Peter Krempa wrote: Hmmm, next patch is moving it to the destination I've suggested. It'd be better to move it to the correct position right away then ...

Expose the new power of virDomainBlockCopy through virsh (well, all but the finer-grained bandwidth, as that is its own can of worms for a later patch). Continue to use the older API where possible, for maximum compatibility. The command now requires either --dest (with optional --format and --blockdev), to directly describe the file destination, or --xml, to name a file that contains an XML description such as: <disk type='network'> <driver type='raw'/> <source protocol='gluster' name='vol1/img'> <host name='red'/> </source> </disk> Non-zero option parameters are converted into virTypedParameters, and if anything requires the new API, the command can synthesize appropriate XML even if the --dest option was used instead of --xml. The existing --raw flag remains for back-compat, but the preferred spelling is now --format=raw, since the new API now allows us to specify all formats rather than just a boolean raw to suppress probing. I hope I did justice in describing the effects of granularity and buf-size on how they get passed through to qemu. * tools/virsh-domain.c (cmdBlockCopy): Add new options --xml, --granularity, --buf-size, --format. Make --raw an alias for --format=raw. Call new API if new parameters are in use. * tools/virsh.pod (blockcopy): Document new options. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 145 +++++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 45 +++++++++------- 2 files changed, 156 insertions(+), 34 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b92b2eb..16d7f05 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1818,11 +1818,10 @@ static const vshCmdOptDef opts_block_copy[] = { {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("fully-qualified path of disk") + .help = N_("fully-qualified path of source disk") }, {.name = "dest", .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, .help = N_("path of the copy to create") }, {.name = "bandwidth", @@ -1838,8 +1837,8 @@ static const vshCmdOptDef opts_block_copy[] = { .help = N_("reuse existing destination") }, {.name = "raw", - .type = VSH_OT_BOOL, - .help = N_("use raw destination file") + .type = VSH_OT_ALIAS, + .help = "format=raw" }, {.name = "blockdev", .type = VSH_OT_BOOL, @@ -1869,6 +1868,22 @@ static const vshCmdOptDef opts_block_copy[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "xml", + .type = VSH_OT_DATA, + .help = N_("filename containing XML description of the copy destination") + }, + {.name = "format", + .type = VSH_OT_DATA, + .help = N_("format of the destination file") + }, + {.name = "granularity", + .type = VSH_OT_INT, + .help = N_("power-of-two granularity to use during the copy") + }, + {.name = "buf-size", + .type = VSH_OT_INT, + .help = N_("maximum amount of in-flight data during the copy") + }, {.name = NULL} }; @@ -1876,14 +1891,18 @@ static bool cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - const char *dest; + const char *dest = NULL; + const char *format = NULL; unsigned long bandwidth = 0; - unsigned int flags = VIR_DOMAIN_BLOCK_REBASE_COPY; + unsigned int granularity = 0; + unsigned long long buf_size = 0; + unsigned int flags = 0; bool ret = false; bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "finish"); + bool blockdev = vshCommandOptBool(cmd, "blockdev"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1893,6 +1912,23 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) const char *path = NULL; bool quit = false; int abort_flags = 0; + const char *xml = NULL; + char *xmlstr = NULL; + virTypedParameterPtr params = NULL; + int nparams = 0; + + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + return false; + if (vshCommandOptString(cmd, "dest", &dest) < 0) + return false; + if (vshCommandOptString(cmd, "xml", &xml) < 0) + return false; + if (vshCommandOptString(cmd, "format", &format) < 0) + return false; + + VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml); + VSH_EXCLUSIVE_OPTIONS_VAR(format, xml); + VSH_EXCLUSIVE_OPTIONS_VAR(blockdev, xml); if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; @@ -1926,24 +1962,101 @@ 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")); goto cleanup; } + if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) { + vshError(ctl, "%s", _("granularity must be a number")); + goto cleanup; + } + if (vshCommandOptULongLong(cmd, "buf-size", &buf_size) < 0) { + vshError(ctl, "%s", _("buf-size must be a number")); + goto cleanup; + } + if (xml) { + if (virFileReadAll(xml, 8192, &xmlstr) < 0) { + vshReportError(ctl); + goto cleanup; + } + } else if (!dest) { + vshError(ctl, "%s", _("need either --dest or --xml")); + goto cleanup; + } + + /* Exploit that VIR_DOMAIN_BLOCK_REBASE_* and + * VIR_DOMAIN_BLOCK_COPY_* flags have the same values. */ if (vshCommandOptBool(cmd, "shallow")) flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; if (vshCommandOptBool(cmd, "reuse-external")) flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; - if (vshCommandOptBool(cmd, "raw")) - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; - if (vshCommandOptBool(cmd, "blockdev")) - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; - if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) - goto cleanup; - - if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) - goto cleanup; + + if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) { + /* New API */ + if (bandwidth || 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 > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + } + if (virTypedParameterAssign(&params[nparams++], + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + bandwidth << 20ULL) < 0) + goto cleanup; + } + if (granularity && + virTypedParameterAssign(&params[nparams++], + VIR_DOMAIN_BLOCK_COPY_GRANULARITY, + VIR_TYPED_PARAM_UINT, + granularity) < 0) + goto cleanup; + if (buf_size && + virTypedParameterAssign(&params[nparams++], + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, + VIR_TYPED_PARAM_ULLONG, + buf_size) < 0) + goto cleanup; + } + + if (!xmlstr) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferEscapeString(&buf, "<disk type='%s'>\n", + blockdev ? "block" : "file"); + virBufferAdjustIndent(&buf, 2); + virBufferEscapeString(&buf, "<source %s", + blockdev ? "dev" : "file"); + virBufferEscapeString(&buf, "='%s'/>\n", dest); + virBufferEscapeString(&buf, "<driver type='%s'/>\n", format); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</disk>\n"); + if (virBufferCheckError(&buf) < 0) + goto cleanup; + xmlstr = virBufferContentAndReset(&buf); + } + + if (virDomainBlockCopy(dom, path, xmlstr, params, nparams, flags) < 0) + goto cleanup; + } else { + /* Old API */ + 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 (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) + goto cleanup; + } if (!blocking) { vshPrint(ctl, "%s", _("Block Copy started")); @@ -2014,6 +2127,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(xmlstr); + virTypedParamsFree(params, nparams); if (dom) virDomainFree(dom); if (blocking) diff --git a/tools/virsh.pod b/tools/virsh.pod index 2923fd9..c3a76dc 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -898,30 +898,31 @@ 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. -=item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>] -[I<--reuse-external>] [I<--raw>] [I<--blockdev>] -[I<--wait> [I<--async>] [I<--verbose>]] -[{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] +=item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>] +| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>] +[I<--wait> [I<--async>] [I<--verbose>]] [{I<--pivot> | I<--finish>}] +[I<--timeout> B<seconds>] [I<granularity>] [I<buf-size>] -Copy a disk backing image chain to I<dest>. By default, this command -flattens the entire chain; but if I<--shallow> is specified, the copy -shares the backing chain. +Copy a disk backing image chain to a destination. Either I<dest> as +the destination file name, or I<xml> as the name of an XML file containing +a top-level <disk> element describing the destination, must be present. +Additionally, if I<dest> is given, I<format> should be specified to declare +the format of the destination (if I<format> is omitted, then libvirt +will reuse the format of the source, or with I<--reuse-external> will +be forced to probe the destination format, which could be a potential +security hole). The command supports I<--raw> as a boolean flag synonym for +I<--format=raw>. When using I<dest>, the destination is treated as a regular +file unless I<--blockdev> is used to signal that it is a block device. By +default, this command flattens the entire chain; but if I<--shallow> is +specified, the copy shares the backing chain. -If I<--reuse-external> is specified, then I<dest> must exist and have +If I<--reuse-external> is specified, then the destination must exist and have sufficient space to hold the copy. If I<--shallow> is used in conjunction with I<--reuse-external> then the pre-created image must have guest visible contents identical to guest visible contents of the backing file of the original image. This may be used to modify the backing file names on the destination. -The format of the destination is determined by the first match in the -following list: if I<--raw> is specified, it will be raw; if -I<--reuse-external> is specified, the existing destination is probed -for a format; and in all other cases, the destination format will -match the source format. The destination is treated as a regular -file unless I<--blockdev> is used to signal that it is a block -device. - By default, the copy job runs in the background, and consists of two phases. Initially, the job must copy all data from the source, and during this phase, the job can only be canceled to revert back to the @@ -943,9 +944,15 @@ 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 or essentially -unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +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 +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> +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>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] -- 1.9.3

In order to implement the new virDomainBlockCopy, the existing block copy internal implementation needs to be adjusted. The new function will parse XML into a storage source, and parse typed parameters into integers, then call into the same common backend. For now, it's easier to keep the same implementation limits that only local file destinations are suported, but now the check needs to be explicit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename... (qemuDomainBlockCopyCommon): ...and adjust parameters. (qemuDomainBlockRebase): Adjust caller. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 108 ++++++++++++++++++++++++++----------------------- 1 file changed, 58 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 946c384..d3f1042 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15351,11 +15351,12 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, /* bandwidth in bytes/s */ static int -qemuDomainBlockCopy(virDomainObjPtr vm, - virConnectPtr conn, - const char *path, - const char *dest, const char *format, - unsigned long long bandwidth, unsigned int flags) +qemuDomainBlockCopyCommon(virDomainObjPtr vm, + virConnectPtr conn, + const char *path, + virStorageSourcePtr dest, + unsigned long long bandwidth, + unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; qemuDomainObjPrivatePtr priv; @@ -15367,11 +15368,12 @@ qemuDomainBlockCopy(virDomainObjPtr vm, bool need_unlink = false; virStorageSourcePtr mirror = NULL; virQEMUDriverConfigPtr cfg = NULL; + const char *format = NULL; + int desttype = virStorageSourceGetActualType(dest); /* Preliminaries: find the disk we are editing, sanity checks */ - virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1); priv = vm->privateData; cfg = virQEMUDriverGetConfig(driver); @@ -15416,8 +15418,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) goto endjob; - if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && - STREQ_NULLABLE(format, "raw") && + if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && + dest->format == VIR_STORAGE_FILE_RAW && disk->src->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " @@ -15427,72 +15429,68 @@ qemuDomainBlockCopy(virDomainObjPtr vm, } /* Prepare the destination file. */ - if (stat(dest, &st) < 0) { + /* XXX Allow non-file mirror destinations */ + if (!virStorageSourceIsLocalStorage(dest)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("non-file destination not supported yet")); + } + if (stat(dest->path, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), - disk->dst, dest); + disk->dst, dest->path); goto endjob; - } else if (flags & (VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) { + } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT || + desttype == VIR_STORAGE_TYPE_BLOCK) { virReportSystemError(errno, _("missing destination file for disk %s: %s"), - disk->dst, dest); + disk->dst, dest->path); goto endjob; } } else if (!S_ISBLK(st.st_mode)) { - if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external destination file for disk %s already " "exists and is not a block device: %s"), - disk->dst, dest); + disk->dst, dest->path); goto endjob; } - if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) { + if (desttype == VIR_STORAGE_TYPE_BLOCK) { virReportError(VIR_ERR_INVALID_ARG, _("blockdev flag requested for disk %s, but file " - "'%s' is not a block device"), disk->dst, dest); + "'%s' is not a block device"), + disk->dst, dest->path); goto endjob; } } - if (VIR_ALLOC(mirror) < 0) + if (!(mirror = virStorageSourceCopy(dest, false))) goto endjob; - /* XXX Allow non-file mirror destinations */ - mirror->type = flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV ? - VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; - if (format) { - if ((mirror->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"), - format); - goto endjob; - } - } else { - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + if (!dest->format) { + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { mirror->format = disk->src->format; } else { /* If the user passed the REUSE_EXT flag, then either they - * also passed the RAW flag (and format is non-NULL), or it is - * safe for us to probe the format from the file that we will - * be using. */ - mirror->format = virStorageFileProbeFormat(dest, cfg->user, + * can also pass the RAW flag or use XML to tell us the format. + * So if we get here, we assume it is safe for us to probe the + * format from the file that we will be using. */ + mirror->format = virStorageFileProbeFormat(dest->path, cfg->user, cfg->group); } } /* pre-create the image file */ - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { - int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT, + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { + int fd = qemuOpenFile(driver, vm, dest->path, + O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); if (fd < 0) goto endjob; VIR_FORCE_CLOSE(fd); } - if (!format && mirror->format > 0) + if (mirror->format > 0) format = virStorageFileFormatTypeToString(mirror->format); - if (VIR_STRDUP(mirror->path, dest) < 0) - goto endjob; if (virStorageSourceInitChainElement(mirror, disk->src, false) < 0) goto endjob; @@ -15506,8 +15504,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDriveMirror(priv->mon, device, dest, format, bandwidth, - flags); + ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format, + bandwidth, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15527,8 +15525,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, vm->def->name); endjob: - if (need_unlink && unlink(dest)) - VIR_WARN("unable to unlink just-created %s", dest); + if (need_unlink && unlink(dest->path)) + VIR_WARN("unable to unlink just-created %s", dest->path); virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; @@ -15546,9 +15544,9 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - const char *format = NULL; int ret = -1; unsigned long long speed = bandwidth; + virStorageSourcePtr dest = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | @@ -15570,8 +15568,14 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, BLOCK_JOB_PULL, flags); /* If we got here, we are doing a block copy rebase. */ + if (VIR_ALLOC(dest) < 0) + goto cleanup; + dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ? + VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; + if (VIR_STRDUP(dest->path, base) < 0) + goto cleanup; if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) - format = "raw"; + dest->format = VIR_STORAGE_FILE_RAW; /* Convert bandwidth MiB to bytes */ if (speed > LLONG_MAX >> 20) { @@ -15591,15 +15595,19 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, goto cleanup; } + /* We rely on the fact that VIR_DOMAIN_BLOCK_REBASE_SHALLOW + * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same values + * as for block copy. */ flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV); - ret = qemuDomainBlockCopy(vm, dom->conn, path, base, format, - bandwidth, flags); + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT); + ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest, + bandwidth, flags); vm = NULL; + cleanup: if (vm) virObjectUnlock(vm); + virStorageSourceFree(dest); return ret; } -- 1.9.3

On 09/05/2014 08:09 AM, Peter Krempa wrote: How about the following interdiff? diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 7026a18..26e0237 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15349,12 +15349,13 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, /* bandwidth in bytes/s. Caller must lock vm beforehand, and not - * access it afterwards. */ + * access it afterwards; likewise, caller must not access mirror + * afterwards. */ static int qemuDomainBlockCopyCommon(virDomainObjPtr vm, virConnectPtr conn, const char *path, - virStorageSourcePtr dest, + virStorageSourcePtr mirror, unsigned long long bandwidth, unsigned int flags) { @@ -15366,10 +15367,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, int idx; struct stat st; bool need_unlink = false; - virStorageSourcePtr mirror = NULL; virQEMUDriverConfigPtr cfg = NULL; const char *format = NULL; - int desttype = virStorageSourceGetActualType(dest); + int desttype = virStorageSourceGetActualType(mirror); /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -15419,7 +15419,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, goto endjob; if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && - dest->format == VIR_STORAGE_FILE_RAW && + mirror->format == VIR_STORAGE_FILE_RAW && disk->src->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " @@ -15430,20 +15430,20 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Prepare the destination file. */ /* XXX Allow non-file mirror destinations */ - if (!virStorageSourceIsLocalStorage(dest)) { + if (!virStorageSourceIsLocalStorage(mirror)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("non-file destination not supported yet")); } - if (stat(dest->path, &st) < 0) { + if (stat(mirror->path, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), - disk->dst, dest->path); + disk->dst, mirror->path); goto endjob; } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT || desttype == VIR_STORAGE_TYPE_BLOCK) { virReportSystemError(errno, _("missing destination file for disk %s: %s"), - disk->dst, dest->path); + disk->dst, mirror->path); goto endjob; } } else if (!S_ISBLK(st.st_mode)) { @@ -15451,22 +15451,19 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external destination file for disk %s already " "exists and is not a block device: %s"), - disk->dst, dest->path); + disk->dst, mirror->path); goto endjob; } if (desttype == VIR_STORAGE_TYPE_BLOCK) { virReportError(VIR_ERR_INVALID_ARG, _("blockdev flag requested for disk %s, but file " "'%s' is not a block device"), - disk->dst, dest->path); + disk->dst, mirror->path); goto endjob; } } - if (!(mirror = virStorageSourceCopy(dest, false))) - goto endjob; - - if (!dest->format) { + if (!mirror->format) { if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { mirror->format = disk->src->format; } else { @@ -15474,14 +15471,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, * can also pass the RAW flag or use XML to tell us the format. * So if we get here, we assume it is safe for us to probe the * format from the file that we will be using. */ - mirror->format = virStorageFileProbeFormat(dest->path, cfg->user, + mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, cfg->group); } } /* pre-create the image file */ if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { - int fd = qemuOpenFile(driver, vm, dest->path, + int fd = qemuOpenFile(driver, vm, mirror->path, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); if (fd < 0) @@ -15504,7 +15501,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format, + ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, bandwidth, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); @@ -15525,8 +15522,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, vm->def->name); endjob: - if (need_unlink && unlink(dest->path)) - VIR_WARN("unable to unlink just-created %s", dest->path); + if (need_unlink && unlink(mirror->path)) + VIR_WARN("unable to unlink just-created %s", mirror->path); virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; @@ -15603,6 +15600,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest, bandwidth, flags); vm = NULL; + dest = NULL; cleanup: if (vm) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 08/31/14 06:02, Eric Blake wrote: \ This actually might be useful for source of block copy and others some time in the future. ACK, thanks for dropping the refactor part. Peter

The hard part of managing the disk copy is already coded; all this had to do was convert the XML and virTypedParameters into the internal representation. With this patch, all blockcopy operations that used the old API should also work via the new API. Additional extensions, such as supporting the granularity tunable or a network rather than file destination, will be added as later patches. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3f1042..d93e184 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15611,6 +15611,90 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, return ret; } + +static int +qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, + virTypedParameterPtr params, int nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + unsigned long long bandwidth = 0; + unsigned int granularity = 0; + unsigned long long buf_size = 0; + virStorageSourcePtr dest = NULL; + size_t i; + + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_COPY_GRANULARITY, + VIR_TYPED_PARAM_UINT, + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; + + if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = &params[i]; + + /* Typed params (wisely) refused to expose unsigned long, but + * back-compat demands that we stick with a maximum of + * unsigned long bandwidth in MiB/s, while our value is + * unsigned long long in bytes/s. Hence, we have to do + * overflow detection if this is a 32-bit server handling a + * 64-bit client. */ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BANDWIDTH)) { + if (sizeof(unsigned long) < sizeof(bandwidth) && + param->value.ul > ULONG_MAX * (1ULL << 20)) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu bytes"), + ULONG_MAX * (1ULL << 20)); + goto cleanup; + } + bandwidth = param->value.ul; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) { + granularity = param->value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) { + 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); + vm = NULL; + + cleanup: + virStorageSourceFree(dest); + if (vm) + virObjectUnlock(vm); + return ret; +} + + static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) @@ -17683,6 +17767,7 @@ static virDriver qemuDriver = { .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ + .domainBlockCopy = qemuDomainBlockCopy, /* 1.2.8 */ .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */ .connectIsAlive = qemuConnectIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = qemuNodeSuspendForDuration, /* 0.9.8 */ -- 1.9.3

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 | 6 +++++- src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 2 +- 7 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d93e184..e597d68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15356,6 +15356,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, const char *path, virStorageSourcePtr dest, unsigned long long bandwidth, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; @@ -15505,7 +15507,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15601,7 +15603,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; cleanup: @@ -15668,23 +15670,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 6bdee4e..3a506f8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1279,7 +1279,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 d96b1b8..e481ec1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3183,17 +3183,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 9012ad4..cb88e6e 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 8130f92..2ccf9cb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3397,6 +3397,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; @@ -3408,7 +3410,9 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, cmd = qemuMonitorJSONMakeCommand("drive-mirror", "s:device", device, "s:target", file, - "U:speed", speed, + "P: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 4595eae..a325463 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, 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