The upstream qemu commit 'dce13204' changed the wording just slightly to add 'in bursts' essentially. Just following that model here. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 52680cf..86126d0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3743,31 +3743,31 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX: * - * Macro represents the total throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the total throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX "blkdeviotune.total_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX: * - * Macro represents the read throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the read throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX "blkdeviotune.read_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX: * - * Macro represents the write throughput limit in maximum bytes per second, - * as VIR_TYPED_PARAM_ULLONG. + * Macro represents the write throughput limit during bursts in + * maximum bytes per second, as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX "blkdeviotune.write_bytes_sec_max" /** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX: * - * Macro represents the total maximum I/O operations per second, + * Macro represents the total maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX "blkdeviotune.total_iops_sec_max" @@ -3775,7 +3775,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX: * - * Macro represents the read maximum I/O operations per second, + * Macro represents the read maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX "blkdeviotune.read_iops_sec_max" @@ -3783,7 +3783,7 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, /** * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX: * - * Macro represents the write maximum I/O operations per second, + * Macro represents the write maximum I/O operations per second during bursts, * as VIR_TYPED_PARAM_ULLONG. */ # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX "blkdeviotune.write_iops_sec_max" -- 2.7.4

We're about to add 6 new options and it appears (from testing) one cannot utilize both the shorthand (alias) and (much) longer names for the arguments. So modify the command builder to use the longer name and of course alter the test output .args to have the similarly innocuous long name. Also utilize a macro to build that name makes it so much more visually appealing and saves a few characters or potential cut-n-paste issues. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 77 +++++----------------- .../qemuxml2argv-blkdeviotune-max.args | 10 ++- .../qemuxml2argv-blkdeviotune.args | 5 +- 3 files changed, 28 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ac0fa1..20fe6fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1766,70 +1766,29 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, goto error; } - if (disk->blkdeviotune.total_bytes_sec) { - virBufferAsprintf(&opt, ",bps=%llu", - disk->blkdeviotune.total_bytes_sec); +#define IOTUNE_ADD(_field, _label) \ + if (disk->blkdeviotune._field) { \ + virBufferAsprintf(&opt, ",throttling." _label "=%llu", \ + disk->blkdeviotune._field); \ } - if (disk->blkdeviotune.read_bytes_sec) { - virBufferAsprintf(&opt, ",bps_rd=%llu", - disk->blkdeviotune.read_bytes_sec); - } - - if (disk->blkdeviotune.write_bytes_sec) { - virBufferAsprintf(&opt, ",bps_wr=%llu", - disk->blkdeviotune.write_bytes_sec); - } - - if (disk->blkdeviotune.total_iops_sec) { - virBufferAsprintf(&opt, ",iops=%llu", - disk->blkdeviotune.total_iops_sec); - } - - if (disk->blkdeviotune.read_iops_sec) { - virBufferAsprintf(&opt, ",iops_rd=%llu", - disk->blkdeviotune.read_iops_sec); - } - - if (disk->blkdeviotune.write_iops_sec) { - virBufferAsprintf(&opt, ",iops_wr=%llu", - disk->blkdeviotune.write_iops_sec); - } - - if (disk->blkdeviotune.total_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_max=%llu", - disk->blkdeviotune.total_bytes_sec_max); - } - - if (disk->blkdeviotune.read_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_rd_max=%llu", - disk->blkdeviotune.read_bytes_sec_max); - } - - if (disk->blkdeviotune.write_bytes_sec_max) { - virBufferAsprintf(&opt, ",bps_wr_max=%llu", - disk->blkdeviotune.write_bytes_sec_max); - } + IOTUNE_ADD(total_bytes_sec, "bps-total"); + IOTUNE_ADD(read_bytes_sec, "bps-read"); + IOTUNE_ADD(write_bytes_sec, "bps-write"); + IOTUNE_ADD(total_iops_sec, "iops-total"); + IOTUNE_ADD(read_iops_sec, "iops-read"); + IOTUNE_ADD(write_iops_sec, "iops-write"); - if (disk->blkdeviotune.total_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_max=%llu", - disk->blkdeviotune.total_iops_sec_max); - } + IOTUNE_ADD(total_bytes_sec_max, "bps-total-max"); + IOTUNE_ADD(read_bytes_sec_max, "bps-read-max"); + IOTUNE_ADD(write_bytes_sec_max, "bps-write-max"); + IOTUNE_ADD(total_iops_sec_max, "iops-total-max"); + IOTUNE_ADD(read_iops_sec_max, "iops-read-max"); + IOTUNE_ADD(write_iops_sec_max, "iops-write-max"); - if (disk->blkdeviotune.read_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_rd_max=%llu", - disk->blkdeviotune.read_iops_sec_max); - } + IOTUNE_ADD(size_iops_sec, "iops-size"); - if (disk->blkdeviotune.write_iops_sec_max) { - virBufferAsprintf(&opt, ",iops_wr_max=%llu", - disk->blkdeviotune.write_iops_sec_max); - } - - if (disk->blkdeviotune.size_iops_sec) { - virBufferAsprintf(&opt, ",iops_size=%llu", - disk->blkdeviotune.size_iops_sec); - } +#undef IOTUNE_ADD if (virBufferCheckError(&opt) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args index 66e1c10..58c15c8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max.args @@ -18,10 +18,14 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ -cache=none,bps=5000,iops=6000,bps_max=10000,iops_max=11000 \ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\ +throttling.bps-total-max=10000,throttling.iops-total-max=11000 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\ -cache=none,bps_rd=5000,bps_wr=5500,iops_rd=3500,iops_wr=4000,bps_rd_max=6000,\ -bps_wr_max=6500,iops_rd_max=7000,iops_wr_max=7500,iops_size=2000 \ +cache=none,throttling.bps-read=5000,throttling.bps-write=5500,\ +throttling.iops-read=3500,throttling.iops-write=4000,\ +throttling.bps-read-max=6000,throttling.bps-write-max=6500,\ +throttling.iops-read-max=7000,throttling.iops-write-max=7500,\ +throttling.iops-size=2000 \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args index 1f9983f..11833e6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args @@ -18,9 +18,10 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ -cache=none,bps=5000,iops=6000 \ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/QEMUGuest2,format=qcow2,if=none,id=drive-ide0-0-1,\ -cache=none,bps_rd=5000,bps_wr=5000,iops=7000 \ +cache=none,throttling.bps-read=5000,throttling.bps-write=5000,\ +throttling.iops-total=7000 \ -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 2.7.4

Add new options to allow proving a duration/length in seconds to allow the bps/iops (and friends) to occur: 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 | 48 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 86126d0..5744e25 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2321,6 +2321,54 @@ int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, # define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX "write_iops_sec_max" /** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by total_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH "total_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by read_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH "read_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by write_bytes_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH "write_bytes_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by total_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH "total_iops_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by read_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH "read_iops_sec_max_length" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH: + * + * Macro for the BlockIoTune tunable weight: it represents the duration in + * seconds for the burst allowed by write_iops_sec_max, as a ullong. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH "write_iops_sec_max" + +/** * VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC: * Macro for the BlockIoTune tunable weight: it represents the size * I/O operations per second permitted through a block device, as a ullong. -- 2.7.4

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 | 106 ++++++++++++++++++++++++++++++++++++++- 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, 211 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5744e25..beeedc6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3845,6 +3845,60 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec" /** + * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.total_bytes_sec_max, + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH "blkdeviotune.total_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.read_bytes_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH "blkdeviotune.read_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.write_bytes_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH "blkdeviotune.write_bytes_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.total_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH "blkdeviotune.total_iops_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.read_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH "blkdeviotune.read_iops_sec_max_length" + +/** + * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH: + * + * Macro represents the length in seconds allowed for a burst period + * for the blkdeviotune.write_iops_sec_max + * as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH "blkdeviotune.write_iops_sec_max_length" + +/** * virConnectDomainEventTunableCallback: * @conn: connection object * @dom: domain on which the event occurred diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce90c27..2869951 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -548,6 +548,12 @@ struct _virDomainBlockIoTuneInfo { unsigned long long read_iops_sec_max; unsigned long long write_iops_sec_max; unsigned long long size_iops_sec; + unsigned long long total_bytes_sec_max_length; + unsigned long long read_bytes_sec_max_length; + unsigned long long write_bytes_sec_max_length; + unsigned long long total_iops_sec_max_length; + unsigned long long read_iops_sec_max_length; + unsigned long long write_iops_sec_max_length; }; typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..3b04754 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -113,7 +113,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_MEM_PARAM 3 #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH 12 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 19 #define QEMU_NB_NUMA_PARAM 2 @@ -17262,7 +17263,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; @@ -17298,6 +17302,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 +17465,60 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, param->value.ul) < 0) goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { + info.total_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { + info.read_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { + info.write_bytes_sec_max_length = param->value.ul; + set_bytes_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { + info.total_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { + info.read_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { + info.write_iops_sec_max_length = param->value.ul; + set_iops_max_length = true; + if (virTypedParamsAddULLong(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, + param->value.ul) < 0) + goto endjob; } } @@ -17496,6 +17566,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); + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block I/O throttling not supported with this " @@ -17511,6 +17584,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + if (!supportMaxLengthOptions && + (set_iops_max_length || set_bytes_max_length)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("a block I/O throttling length parameter is not " + "supported with this QEMU binary")); + goto endjob; + } + if (!(disk = qemuDomainDiskByName(def, path))) goto endjob; @@ -17543,6 +17624,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!set_size_iops) info.size_iops_sec = oldinfo->size_iops_sec; + if (!set_bytes_max_length) { + info.total_bytes_sec_max_length = oldinfo->total_bytes_sec_max_length; + info.read_bytes_sec_max_length = oldinfo->read_bytes_sec_max_length; + info.write_bytes_sec_max_length = oldinfo->write_bytes_sec_max_length; + } + if (!set_iops_max_length) { + info.total_iops_sec_max_length = oldinfo->total_iops_sec_max_length; + info.read_iops_sec_max_length = oldinfo->read_iops_sec_max_length; + info.write_iops_sec_max_length = oldinfo->write_iops_sec_max_length; + } #define CHECK_MAX(val) \ do { \ @@ -17566,7 +17657,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, - &info, supportMaxOptions); + &info, supportMaxOptions, + supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) @@ -17667,6 +17759,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_LENGTH; } if (*nparams == 0) { @@ -17730,6 +17825,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); + BLOCK_IOTUNE_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, total_bytes_sec_max_length); + BLOCK_IOTUNE_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length); + BLOCK_IOTUNE_ASSIGN(WRITE_BYTES_SEC_MAX_LENGTH, write_bytes_sec_max_length); + + BLOCK_IOTUNE_ASSIGN(TOTAL_IOPS_SEC_MAX_LENGTH, total_iops_sec_max_length); + BLOCK_IOTUNE_ASSIGN(READ_IOPS_SEC_MAX_LENGTH, read_iops_sec_max_length); + BLOCK_IOTUNE_ASSIGN(WRITE_IOPS_SEC_MAX_LENGTH, write_iops_sec_max_length); #undef BLOCK_IOTUNE_ASSIGN ret = 0; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 8083a36..f800efd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3423,14 +3423,17 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions) + bool supportMaxOptions, + bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); QEMU_CHECK_MONITOR(mon); if (mon->json) - return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, supportMaxOptions); + return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, + supportMaxOptions, + supportMaxLengthOptions); else return qemuMonitorTextSetBlockIoThrottle(mon, device, info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7d78e5b..700dff9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -873,7 +873,8 @@ int qemuMonitorOpenGraphics(qemuMonitorPtr mon, int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions); + bool supportMaxOptions, + bool supportMaxLengthOptions); int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, const char *device, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 43a3fa7..6b26cf9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4559,6 +4559,12 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, GET_THROTTLE_STATS_OPTIONAL("iops_rd_max", read_iops_sec_max); GET_THROTTLE_STATS_OPTIONAL("iops_wr_max", write_iops_sec_max); GET_THROTTLE_STATS_OPTIONAL("iops_size", size_iops_sec); + GET_THROTTLE_STATS_OPTIONAL("bps_max_length", total_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("bps_rd_max_length", read_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("bps_wr_max_length", write_bytes_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_max_length", total_iops_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_rd_max_length", read_iops_sec_max_length); + GET_THROTTLE_STATS_OPTIONAL("iops_wr_max_length", write_iops_sec_max_length); break; } @@ -4580,7 +4586,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions) + bool supportMaxOptions, + bool supportMaxLengthOptions) { int ret = -1; virJSONValuePtr cmd = NULL; @@ -4589,7 +4596,8 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, /* The qemu capability check has already been made in * qemuDomainSetBlockIoTune. NB, once a NULL is found in * the sequence, qemuMonitorJSONMakeCommand will stop. So - * let's make use of that when !supportMaxOptions */ + * let's make use of that when !supportMaxOptions and + * similarly when !supportMaxLengthOptions */ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", "s:device", device, "U:bps", info->total_bytes_sec, @@ -4606,6 +4614,19 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, "U:iops_rd_max", info->read_iops_sec_max, "U:iops_wr_max", info->write_iops_sec_max, "U:iops_size", info->size_iops_sec, + !supportMaxLengthOptions ? NULL : + "U:bps_max_length", + info->total_bytes_sec_max_length, + "U:bps_rd_max_length", + info->read_bytes_sec_max_length, + "U:bps_wr_max_length", + info->write_bytes_sec_max_length, + "U:iops_max_length", + info->total_iops_sec_max_length, + "U:iops_rd_max_length", + info->read_iops_sec_max_length, + "U:iops_wr_max_length", + info->write_iops_sec_max_length, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 6a5eb3b..77b2e02 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -327,7 +327,8 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, - bool supportMaxOptions); + bool supportMaxOptions, + bool supportMaxLengthOptions); int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, const char *device, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index cbc39c6..d4f6250 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -67,6 +67,12 @@ const char *queryBlockReply = " \"iops_rd_max\": 11," " \"iops_wr_max\": 12," " \"iops_size\": 13," +" \"bps_max_length\": 14," +" \"bps_rd_max_length\": 15," +" \"bps_wr_max_length\": 16," +" \"iops_max_length\": 17," +" \"iops_rd_max_length\": 18," +" \"iops_wr_max_length\": 19," " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," " \"encryption_key_missing\": false" " }," @@ -1885,7 +1891,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1; - expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || qemuMonitorTestAddItemParams(test, "block_set_io_throttle", @@ -1897,6 +1903,12 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) "bps_wr_max", "9", "iops_max", "10", "iops_rd_max", "11", "iops_wr_max", "12", "iops_size", "13", + "bps_max_length", "14", + "bps_rd_max_length", "15", + "bps_wr_max_length", "16", + "iops_max_length", "17", + "iops_rd_max_length", "18", + "iops_wr_max_length", "19", NULL, NULL) < 0) goto cleanup; @@ -1911,7 +1923,8 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) } if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), - "drive-virtio-disk1", &info, true) < 0) + "drive-virtio-disk1", &info, true, + true) < 0) goto cleanup; ret = 0; -- 2.7.4

On 28/09/16 22:27, John Ferlan wrote: While I totally support ^^this kind of macro-based code cleanup, I'm rather skeptic about ^^this because IMHO it's just too much in terms of code duplication cleanup. I think that in this case (like in many other cases) we should try to find the sweet spot between line removal and the time programmer spends reading these macros. We could use exactly the same approach as we do with STORE_MEM_RECORD, QEMU_ADD_NET_PARAM, PARSE_MEMTUNE_PARAM. Now, I know that in the original hunk below was a massive if-else if block which compared to a bunch of ifs like these: SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); SET_IOTUNE_FIELD(ARG1,ARG2,ARG3); would prevent testing of any extra conditions once a single condition has been satisfied. But I believe that, provided that param->field does not change within any of the 'if' blocks (which it doesn't), the compiler would optimize the two example hunks below and produce an identical assembly for both. if (foo == 0) bar0(); else if (foo == 1) bar1(); else if (foo == 2) bar2(); ################## if (foo == 0) bar0(); if (foo == 1) bar1(); if (foo == 2) bar2(); Erik

On 30/09/16 12:57, John Ferlan wrote: Yeah, I also posted some suggestion in my original reply, have a look at that, to make it short I think that construction like this might work as well and is still slightly more readable: SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); I purposely got rid of the if-else-if construction altogether because the compiler might actually optimize it, as I said, see my original reply to this patch and tell me what you think. Erik

On 30/09/16 13:47, John Ferlan wrote: Almost, but what I had in mind was to just keep one of the macros - be it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then end up with a snippet like the one in the attached patch. Erik PS: Sorry for attaching a patch, but thunderbird's editor truly sucks (pasting from vim is a total nightmare)...big time...and I really have to switch to some normal mail client.

On 03/10/16 14:16, John Ferlan wrote: Yeah, I could have done that, adjusting the message I sent along with the patch (I also tried selective wrap but that certainly didn't bring satisfactory results). Instead, I rage quit and thought that attaching the patch might not just be the worst possible way at all. Anyway, I'm starting to convince myself that spending several hours configuring mutt might as well turn out as a plausible solution of all email-related future problems than sticking with thunderbird after all. Well, I do see your point, I just have a different feeling about that. When I look at the qemuDomainSetBlockIoTune function, I see a bunch of variables defined, among which all the set_(bytes|iops) variables reside. Let's consider a massive function where you introduced a macro to get rid of all the duplicate code. What I expect from the macro is that I clearly see what parameters it takes so I can very simply identify which variables will be affected by the macro whatever it is trying to accomplish. By introducing another macro representing an additional meta layer of name processing, I fail to see what arguments will be eventually affected by the macro machinery at first glance. Your initial idea which was identical to my proposal makes it clear what variables will be affected by the macro and the only thing that needs to be handled is prefixing the enums with VIR_DOMAIN_BLOCK_IOTUNE. In my opinion, by visually grouping the related blocks of code can also lead to certain amount of cleanliness. Another thing, since in patch 4 and 9 you went the way I'm currently describing, I'm not a fan of treating essentially the same thing differently. Erik

Rework the code to create a couple of macros to hide all the comparisons for each of the 6 "types" of data fields to collect. Each macro will check the base name, then the base name + "_max", and base name + "_max_length". Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- "Formalized" patch based on the 08.5/12 conversation src/qemu/qemu_driver.c | 203 ++++++++----------------------------------------- 1 file changed, 32 insertions(+), 171 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5322405..9e9c8d6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17377,6 +17377,27 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob; +#define SET_IOTUNE(FIELD, BOOL, CONST) \ + do { \ + info.FIELD = params->value.ul; \ + set_##BOOL = true; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto endjob; \ + } while (0); + +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ + SET_IOTUNE(FIELD, BOOL, CONST); \ + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \ + SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \ + } else if (STREQ(param->field, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \ + SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \ + } + for (i = 0; i < nparams; i++) { virTypedParameterPtr param = &params[i]; @@ -17387,178 +17408,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { - info.total_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { - info.read_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { - info.write_bytes_sec = param->value.ul; - set_bytes = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { - info.total_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { - info.read_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { - info.write_iops_sec = param->value.ul; - set_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) { - info.total_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) { - info.read_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) { - info.write_bytes_sec_max = param->value.ul; - set_bytes_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) { - info.total_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) { - info.read_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) { - info.write_iops_sec_max = param->value.ul; - set_iops_max = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) { - info.size_iops_sec = param->value.ul; - set_size_iops = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) { - info.total_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) { - info.read_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) { - info.write_bytes_sec_max_length = param->value.ul; - set_bytes_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) { - info.total_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) { - info.read_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } else if (STREQ(param->field, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) { - info.write_iops_sec_max_length = param->value.ul; - set_iops_max_length = true; - if (virTypedParamsAddULLong(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, - param->value.ul) < 0) - goto endjob; - } + SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC); + SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC); + SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC); + SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC); + SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC); + SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) + SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC); + } +#undef SET_IOTUNE +#undef SET_IOTUNE_FIELD if ((info.total_bytes_sec && info.read_bytes_sec) || (info.total_bytes_sec && info.write_bytes_sec)) { -- 2.7.4

Rather than copy-paste - use a macro Unfortunately due to how the RNG schema was written keeping the 'value' and 'value'_max next to each other in the XML causes a schema failure, so the FORMAT has to write out singly rather than optimizing to write out both values at once Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 77 +++++++++++++------------------------------------- 1 file changed, 19 insertions(+), 58 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd34cec..f53d6c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20061,6 +20061,12 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, } +#define FORMAT_IOTUNE(val) \ + if (def->blkdeviotune.val) { \ + virBufferAsprintf(buf, "<" #val ">%llu</" #val ">\n", \ + def->blkdeviotune.val); \ + } + static int virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, @@ -20248,66 +20254,20 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec) { virBufferAddLit(buf, "<iotune>\n"); virBufferAdjustIndent(buf, 2); - if (def->blkdeviotune.total_bytes_sec) { - virBufferAsprintf(buf, "<total_bytes_sec>%llu</total_bytes_sec>\n", - def->blkdeviotune.total_bytes_sec); - } - - if (def->blkdeviotune.read_bytes_sec) { - virBufferAsprintf(buf, "<read_bytes_sec>%llu</read_bytes_sec>\n", - def->blkdeviotune.read_bytes_sec); - - } - - if (def->blkdeviotune.write_bytes_sec) { - virBufferAsprintf(buf, "<write_bytes_sec>%llu</write_bytes_sec>\n", - def->blkdeviotune.write_bytes_sec); - } - - if (def->blkdeviotune.total_iops_sec) { - virBufferAsprintf(buf, "<total_iops_sec>%llu</total_iops_sec>\n", - def->blkdeviotune.total_iops_sec); - } - - if (def->blkdeviotune.read_iops_sec) { - virBufferAsprintf(buf, "<read_iops_sec>%llu</read_iops_sec>\n", - def->blkdeviotune.read_iops_sec); - } - if (def->blkdeviotune.write_iops_sec) { - virBufferAsprintf(buf, "<write_iops_sec>%llu</write_iops_sec>\n", - def->blkdeviotune.write_iops_sec); - } - - if (def->blkdeviotune.total_bytes_sec_max) { - virBufferAsprintf(buf, "<total_bytes_sec_max>%llu</total_bytes_sec_max>\n", - def->blkdeviotune.total_bytes_sec_max); - } - - if (def->blkdeviotune.read_bytes_sec_max) { - virBufferAsprintf(buf, "<read_bytes_sec_max>%llu</read_bytes_sec_max>\n", - def->blkdeviotune.read_bytes_sec_max); - } - - if (def->blkdeviotune.write_bytes_sec_max) { - virBufferAsprintf(buf, "<write_bytes_sec_max>%llu</write_bytes_sec_max>\n", - def->blkdeviotune.write_bytes_sec_max); - } - - if (def->blkdeviotune.total_iops_sec_max) { - virBufferAsprintf(buf, "<total_iops_sec_max>%llu</total_iops_sec_max>\n", - def->blkdeviotune.total_iops_sec_max); - } + FORMAT_IOTUNE(total_bytes_sec); + FORMAT_IOTUNE(read_bytes_sec); + FORMAT_IOTUNE(write_bytes_sec); + FORMAT_IOTUNE(total_iops_sec); + FORMAT_IOTUNE(read_iops_sec); + FORMAT_IOTUNE(write_iops_sec); - if (def->blkdeviotune.read_iops_sec_max) { - virBufferAsprintf(buf, "<read_iops_sec_max>%llu</read_iops_sec_max>\n", - def->blkdeviotune.read_iops_sec_max); - } - - if (def->blkdeviotune.write_iops_sec_max) { - virBufferAsprintf(buf, "<write_iops_sec_max>%llu</write_iops_sec_max>\n", - def->blkdeviotune.write_iops_sec_max); - } + FORMAT_IOTUNE(total_bytes_sec_max); + FORMAT_IOTUNE(read_bytes_sec_max); + FORMAT_IOTUNE(write_bytes_sec_max); + FORMAT_IOTUNE(total_iops_sec_max); + FORMAT_IOTUNE(read_iops_sec_max); + FORMAT_IOTUNE(write_iops_sec_max); if (def->blkdeviotune.size_iops_sec) { virBufferAsprintf(buf, "<size_iops_sec>%llu</size_iops_sec>\n", @@ -20339,6 +20299,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</disk>\n"); return 0; } +#undef FORMAT_IOTUNE static int -- 2.7.4

Modify _virDomainBlockIoTuneInfo and rng schema to support the _length options for bps/iops throttling values. Document the new values. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 40 ++++++++++++- docs/schemas/domaincommon.rng | 38 +++++++++++++ src/conf/domain_conf.c | 45 ++++++++++++++- .../qemuxml2argv-blkdeviotune-max-length.xml | 65 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-max-length.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f48a4d8..d88f084 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2616,7 +2616,45 @@ maximum write I/O operations per second.</dd> <dt><code>size_iops_sec</code></dt> <dd>The optional <code>size_iops_sec</code> element is the - size of I/O operations per second.</dd> + size of I/O operations per second. + <p> + <span class="since">Throughput limits since 1.2.11 and QEMU 1.7</span> + </p> + </dd> + <dt><code>total_bytes_sec_max_length</code></dt> + <dd>The optional <code>total_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>total_bytes_sec_max</code> burst period. Only valid + when the <code>total_bytes_sec_max</code> is set.</dd> + <dt><code>read_bytes_sec_max_length</code></dt> + <dd>The optional <code>read_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>read_bytes_sec_max</code> burst period. Only valid + when the <code>read_bytes_sec_max</code> is set.</dd> + <dt><code>write_bytes_sec_max</code></dt> + <dd>The optional <code>write_bytes_sec_max_length</code> + element is the maximum duration in seconds for the + <code>write_bytes_sec_max</code> burst period. Only valid + when the if <code>write_bytes_sec_max</code> is set.</dd> + <dt><code>total_iops_sec_max_length</code></dt> + <dd>The optional <code>total_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>total_iops_sec_max</code> burst period. Only valid + when the <code>total_iops_sec_max</code> is set.</dd> + <dt><code>read_iops_sec_max_length</code></dt> + <dd>The optional <code>read_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>read_iops_sec_max</code> burst period. Only valid + when the <code>read_iops_sec_max</code> is set.</dd> + <dt><code>write_iops_sec_max</code></dt> + <dd>The optional <code>write_iops_sec_max_length</code> + element is the maximum duration in seconds for the + <code>write_iops_sec_max</code> burst period. Only valid + when the <code>write_iops_sec_max</code> is set. + <p> + <span class="since">Throughput length since 2.3.0 and QEMU 2.6</span> + </p> + </dd> </dl> </dd> <dt><code>driver</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 95c7882..0c3a411 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4949,6 +4949,44 @@ <data type="unsignedLong"/> </element> </optional> + <choice> + <element name="total_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_bytes_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> + <choice> + <element name="total_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_iops_sec_max_length"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 861a15d..6e405c4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7134,6 +7134,21 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, _("disk iotune field '%s_max' must be an integer"), \ #val); \ return -1; \ + } \ + if (virXPathULongLong("string(./iotune/" #val "_max_length)", \ + ctxt, &def->blkdeviotune.val##_max_length) == -2) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune field '%s_max_length' must be " \ + "an integer"), #val); \ + return -1; \ + } + +#define CHECK_IOTUNE_MAX_LENGTH(val) \ + if (def->blkdeviotune.val##_length && !def->blkdeviotune.val) { \ + virReportError(VIR_ERR_XML_ERROR, \ + _("disk iotune '%s'_length cannot be set unless " \ + "disk iotune '%s' is also set"), #val, #val); \ + return -1; \ } static int @@ -7155,6 +7170,13 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; } + //PARSE_IOTUNE(total_bytes_sec_max_length); + //PARSE_IOTUNE(read_bytes_sec_max_length); + //PARSE_IOTUNE(write_bytes_sec_max_length); + //PARSE_IOTUNE(total_iops_sec_max_length); + //PARSE_IOTUNE(read_iops_sec_max_length); + //PARSE_IOTUNE(write_iops_sec_max_length); + if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || (def->blkdeviotune.total_bytes_sec && @@ -7195,8 +7217,16 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return -1; } + CHECK_IOTUNE_MAX_LENGTH(total_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(read_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(write_bytes_sec_max); + CHECK_IOTUNE_MAX_LENGTH(total_iops_sec_max); + CHECK_IOTUNE_MAX_LENGTH(read_iops_sec_max); + CHECK_IOTUNE_MAX_LENGTH(write_iops_sec_max); + return 0; } +#undef CHECK_IOTUNE_MAX_LENGTH #undef PARSE_IOTUNE @@ -20257,7 +20287,13 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.total_iops_sec_max || def->blkdeviotune.read_iops_sec_max || def->blkdeviotune.write_iops_sec_max || - def->blkdeviotune.size_iops_sec) { + def->blkdeviotune.size_iops_sec || + def->blkdeviotune.total_bytes_sec_max_length || + def->blkdeviotune.read_bytes_sec_max_length || + def->blkdeviotune.write_bytes_sec_max_length || + def->blkdeviotune.total_iops_sec_max_length || + def->blkdeviotune.read_iops_sec_max_length || + def->blkdeviotune.write_iops_sec_max_length) { virBufferAddLit(buf, "<iotune>\n"); virBufferAdjustIndent(buf, 2); @@ -20280,6 +20316,13 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); } + FORMAT_IOTUNE(total_bytes_sec_max_length); + FORMAT_IOTUNE(read_bytes_sec_max_length); + FORMAT_IOTUNE(write_bytes_sec_max_length); + FORMAT_IOTUNE(total_iops_sec_max_length); + FORMAT_IOTUNE(read_iops_sec_max_length); + FORMAT_IOTUNE(write_iops_sec_max_length); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</iotune>\n"); } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml new file mode 100644 index 0000000..c740751 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml @@ -0,0 +1,65 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>5000</total_bytes_sec> + <total_iops_sec>6000</total_iops_sec> + <total_bytes_sec_max>10000</total_bytes_sec_max> + <total_iops_sec_max>11000</total_iops_sec_max> + <total_bytes_sec_max_length>3</total_bytes_sec_max_length> + <total_iops_sec_max_length>5</total_iops_sec_max_length> + </iotune> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <iotune> + <read_bytes_sec>5000</read_bytes_sec> + <write_bytes_sec>5500</write_bytes_sec> + <read_iops_sec>3500</read_iops_sec> + <write_iops_sec>4000</write_iops_sec> + <read_bytes_sec_max>6000</read_bytes_sec_max> + <write_bytes_sec_max>6500</write_bytes_sec_max> + <read_iops_sec_max>7000</read_iops_sec_max> + <write_iops_sec_max>7500</write_iops_sec_max> + <size_iops_sec>2000</size_iops_sec> + <read_bytes_sec_max_length>3</read_bytes_sec_max_length> + <write_bytes_sec_max_length>5</write_bytes_sec_max_length> + <read_iops_sec_max_length>7</read_iops_sec_max_length> + <write_iops_sec_max_length>9</write_iops_sec_max_length> + </iotune> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml new file mode 120000 index 0000000..67f3e9a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4b58986..5c50512 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -608,6 +608,7 @@ mymain(void) DO_TEST("usb-redir-filter-version", NONE); DO_TEST("blkdeviotune", NONE); DO_TEST("blkdeviotune-max", NONE); + DO_TEST("blkdeviotune-max-length", NONE); DO_TEST("controller-usb-order", NONE); DO_TEST_FULL("seclabel-dynamic-baselabel", WHEN_INACTIVE, GIC_NONE, NONE); -- 2.7.4

On 23.09.2016 14:56, John Ferlan wrote: Whoa, probably a leftover from development? Drop it. Michal