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

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(a)redhat.com&gt; > --- > 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