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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list