[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 05/18] blockjob: hoist bandwidth scaling out of monitor code



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]