On 10/25/2016 01:30 PM, Erik Skultety wrote:
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>
> ---
> 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.
D'oh... The name was so long I missed the double set!
> 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)
Yes, quite painful - especially when I realized it after the initial
series... The difference is I went with a function.
I create a macro for the setting of the defaults
> +#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.
Ironically - there is another field, but it was added before the -length
values were supported. That's the *next* series I'll be posting...
The tricky part of the OR'd flags will be how the command line logic is
put together. It's a rather unique hack that ultimately is in
qemuMonitorJSONSetBlockIoThrottle regarding how the command line is put
together with a bit of "knowledge" regarding "when" the boolean was
added.
Coming to the theater of the absurd soon is "max" (added in qemu 1.7),
"group" (added in qemu 2.4), and "max_length" (added in qemu 2.6).
Tks -
John
ACK with the macro adjustment above.
Erik