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 <[email protected]> > --- > 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
