[libvirt] [PATCH v2 08/12] qemu: Add length for bps/iops throttling parameters to driver

Erik Skultety eskultet at redhat.com
Tue Oct 25 17:30:56 UTC 2016


On Thu, Oct 06, 2016 at 06:38:56PM -0400, John Ferlan wrote:
> Add support for a duration/length for the bps/iops and friends.
> 
> Modify the API in order to add the "blkdeviotune." specific definitions
> for the iotune throttling duration/length options
> 
>     total_bytes_sec_max_length
>     write_bytes_sec_max_length
>     read_bytes_sec_max_length
>     total_iops_sec_max_length
>     write_iops_sec_max_length
>     read_iops_sec_max_length
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  include/libvirt/libvirt-domain.h |  54 +++++++++++++++++++++
>  src/conf/domain_conf.h           |   6 +++
>  src/qemu/qemu_driver.c           | 100 +++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_monitor.c          |   7 ++-
>  src/qemu/qemu_monitor.h          |   3 +-
>  src/qemu/qemu_monitor_json.c     |  25 +++++++++-
>  src/qemu/qemu_monitor_json.h     |   3 +-
>  tests/qemumonitorjsontest.c      |  17 ++++++-
>  8 files changed, 202 insertions(+), 13 deletions(-)
> 

[...]

> @@ -17296,7 +17297,9 @@ qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,

just a nitpick, are both the 'Set's necessary in the name, unless one of them
is a verb and the other a noun, I think the first one could be dropped.

>                                      bool set_iops,
>                                      bool set_bytes_max,
>                                      bool set_iops_max,
> -                                    bool set_size_iops)
> +                                    bool set_size_iops,
> +                                    bool set_bytes_max_length,
> +                                    bool set_iops_max_length)
>  {
>      if (!set_bytes) {
>          newinfo->total_bytes_sec = oldinfo->total_bytes_sec;
> @@ -17320,6 +17323,36 @@ qemuDomainSetBlockIoTuneSetDefaults(virDomainBlockIoTuneInfoPtr newinfo,
>      }
>      if (!set_size_iops)
>          newinfo->size_iops_sec = oldinfo->size_iops_sec;
> +
> +    /* The length field is handled a bit differently. If not defined/set,
> +     * QEMU will default these to 0 or 1 depending on whether something in
> +     * the same family is set or not.
> +     *
> +     * Similar to other values, if nothing in the family is defined/set,
> +     * then take whatever is in the oldinfo.
> +     *
> +     * To clear an existing limit, a 0 is provided; however, passing that
> +     * 0 onto QEMU if there's a family value defined/set (or defaulted)
> +     * will cause an error. So, to mimic that, if our oldinfo was set and
> +     * our newinfo is clearing, then set max_length based on whether we
> +     * have a value in the family set/defined. */

Hmm. You can disregard my comment in 4/12 about replacing the whole function
with a macro, instead I suggest keeping the function and making the macro below
also work for all the settings above, otherwise it just doesn't look right, one
part of the function being expanded while the other covered by a macro because
the behaviour is slightly different (I have to admit though, it was kinda
painful to comprehend what's going on on the qemu side)

> +#define SET_MAX_LENGTH(BOOL, FIELD)                                            \
> +    if (!BOOL)                                                                 \
> +        newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length;             \
> +    else if (BOOL && oldinfo->FIELD##_max_length &&                            \
> +             !newinfo->FIELD##_max_length)                                     \
> +        newinfo->FIELD##_max_length = (newinfo->FIELD ||                       \
> +                                       newinfo->FIELD##_max) ? 1 : 0;
> +
> +        SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec);
> +        SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec);
> +        SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec);
> +        SET_MAX_LENGTH(set_iops_max_length, total_iops_sec);
> +        SET_MAX_LENGTH(set_iops_max_length, read_iops_sec);
> +        SET_MAX_LENGTH(set_iops_max_length, write_iops_sec);
> +
> +#undef SET_MAX_LENGTH
> +
>  }
>  
>  
> @@ -17346,7 +17379,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>      bool set_bytes_max = false;
>      bool set_iops_max = false;
>      bool set_size_iops = false;
> +    bool set_bytes_max_length = false;
> +    bool set_iops_max_length = false;
>      bool supportMaxOptions = true;
> +    bool supportMaxLengthOptions = true;
>      virQEMUDriverConfigPtr cfg = NULL;
>      virObjectEventPtr event = NULL;
>      virTypedParameterPtr eventParams = NULL;
> @@ -17382,6 +17418,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>                                 VIR_TYPED_PARAM_ULLONG,
>                                 VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
>                                 VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
> +                               VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,
> +                               VIR_TYPED_PARAM_ULLONG,
>                                 NULL) < 0)
>          return -1;
>  
> @@ -17449,6 +17497,19 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>          SET_IOTUNE_FIELD(write_iops_sec_max, set_iops_max,
>                           WRITE_IOPS_SEC_MAX);
>          SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC);
> +
> +        SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length,
> +                         TOTAL_BYTES_SEC_MAX_LENGTH);
> +        SET_IOTUNE_FIELD(read_bytes_sec_max_length, set_bytes_max_length,
> +                         READ_BYTES_SEC_MAX_LENGTH);
> +        SET_IOTUNE_FIELD(write_bytes_sec_max_length, set_bytes_max_length,
> +                         WRITE_BYTES_SEC_MAX_LENGTH);
> +        SET_IOTUNE_FIELD(total_iops_sec_max_length, set_iops_max_length,
> +                         TOTAL_IOPS_SEC_MAX_LENGTH);
> +        SET_IOTUNE_FIELD(read_iops_sec_max_length, set_iops_max_length,
> +                         READ_IOPS_SEC_MAX_LENGTH);
> +        SET_IOTUNE_FIELD(write_iops_sec_max_length, set_iops_max_length,
> +                         WRITE_IOPS_SEC_MAX_LENGTH);
>      }
>  
>  #undef SET_IOTUNE_FIELD
> @@ -17488,6 +17549,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>      if (def) {
>          supportMaxOptions = virQEMUCapsGet(priv->qemuCaps,
>                                             QEMU_CAPS_DRIVE_IOTUNE_MAX);
> +        supportMaxLengthOptions =
> +            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
> +

Basically I have nothing against this^^, just a note that should the
capabilities covering iotune be extended in the future I can image this to be
handled as OR'd flags rather than individual variables, but it's okay now.

ACK with the macro adjustment above.

Erik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161025/ff3f1f38/attachment-0001.sig>


More information about the libvir-list mailing list