On 08/31/14 06:02, Eric Blake wrote:
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@redhat.com> --- 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_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;
I started from bottom, so see at the end for a common comment ...
- } - 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);
See below ...
- 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);
I'd keep the check for if (speed > LLONG_MAX) here to be sure that we don't pass something between LLONG_MAX and ULLONG_MAX to qemu as it would be converted to signed by the monitor code.
- return -1; - } - speed <<= 20;
Of course this has to be dropped.
- 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"));
Possibly we could add a new conversion option to qemuMonitorJSONMakeCommand that would check and reject numbers between LLONG_MAX and ULLONG_MAX rather than converting them to signed silently... If you decide against the option above I ACK this with the bandwidth check added in the monitor APIs. Peter