[libvirt] [PATCH 0/6] Add group_name support for <iotune>
https://bugzilla.redhat.com/show_bug.cgi?id=1336564 Add support for "group" name in <iotune>. The odd thing about the <iotune> group name is that support was added in qemu after the _max parameters, but before the _max_length - so when building the command line - that needs to be followed. At least it's far less complicated than the _length parameters. John Ferlan (6): include: Add new "group_name" definition for iotune throttling caps: Add new capability for the iotune group name qemu: Add support for parsing iotune group setting conf: Add support for blkiotune group_name option qemu: Add the group name option to the iotune command line virsh: Add group name to blkdeviotune output docs/formatdomain.html.in | 11 ++++ docs/schemas/domaincommon.rng | 5 ++ include/libvirt/libvirt-domain.h | 15 +++++ src/conf/domain_conf.c | 10 +++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 13 ++++ src/qemu/qemu_driver.c | 50 +++++++++++++-- src/qemu/qemu_monitor.c | 2 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 +++++- src/qemu/qemu_monitor_json.h | 1 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++----- .../qemuxml2argv-blkdeviotune-group-num.args | 32 ++++++++++ .../qemuxml2argv-blkdeviotune-group-num.xml | 61 ++++++++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../qemuxml2xmlout-blkdeviotune-group-num.xml | 1 + tests/qemuxml2xmltest.c | 1 + tools/virsh-domain.c | 17 +++++ tools/virsh.pod | 5 +- 28 files changed, 309 insertions(+), 26 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml -- 2.7.4
Add the new field to support sharing I/O throttling quota between multiple drives. Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5f50660..8c9876c 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2377,6 +2377,13 @@ int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, */ # define VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC "size_iops_sec" +/** + * VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME: + * Macro for the BlockIoTune tunable weight: it represents a group name to + * allow sharing of I/O throttling quota between multiple drives, as a string. + */ +# define VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME "group_name" + int virDomainSetBlockIoTune(virDomainPtr dom, const char *disk, -- 2.7.4
Add the capability to detect if the qemu binary can support the feature to use throttling.group. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + 9 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7a8202a..a3248ab 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -347,6 +347,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "machine-iommu", "virtio-vga", "drive-iotune-max-length", + "drive-iotune-group", ); @@ -2845,6 +2846,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "name", "guest", QEMU_CAPS_NAME_GUEST }, { "spice", "unix", QEMU_CAPS_SPICE_UNIX }, { "drive", "throttling.bps-total-max-length", QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH }, + { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 6e7a855..0e51df6 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -381,6 +381,7 @@ typedef enum { QEMU_CAPS_MACHINE_IOMMU, /* -machine iommu=on */ QEMU_CAPS_DEVICE_VIRTIO_VGA, /* -device virtio-vga */ QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH, /* -drive bps_max_length = and friends */ + QEMU_CAPS_DRIVE_IOTUNE_GROUP, /* -drive throttling.group=<name> */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml index 3162758..e65ebe6 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml @@ -181,6 +181,7 @@ <flag name='virtio-pci-disable-legacy'/> <flag name='machine-iommu'/> <flag name='virtio-vga'/> + <flag name='drive-iotune-group'/> <version>2004000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml index 62d42c3..fc00271 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -186,6 +186,7 @@ <flag name='virtio-pci-disable-legacy'/> <flag name='machine-iommu'/> <flag name='virtio-vga'/> + <flag name='drive-iotune-group'/> <version>2005000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml index 5c6a709..a397383 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -159,6 +159,7 @@ <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> <flag name='drive-iotune-max-length'/> + <flag name='drive-iotune-group'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml index 6ba97be..4ac2eb5 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -159,6 +159,7 @@ <flag name='smm'/> <flag name='virtio-pci-disable-legacy'/> <flag name='drive-iotune-max-length'/> + <flag name='drive-iotune-group'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml index 9174f58..7b38c30 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -154,6 +154,7 @@ <flag name='virtio-pci-disable-legacy'/> <flag name='virtio-vga'/> <flag name='drive-iotune-max-length'/> + <flag name='drive-iotune-group'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 1c309df..4d03d7a 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -193,6 +193,7 @@ <flag name='machine-iommu'/> <flag name='virtio-vga'/> <flag name='drive-iotune-max-length'/> + <flag name='drive-iotune-group'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index 2f168da..e971ecd 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -194,6 +194,7 @@ <flag name='query-hotpluggable-cpus'/> <flag name='virtio-vga'/> <flag name='drive-iotune-max-length'/> + <flag name='drive-iotune-group'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0)</package> -- 2.7.4
Add support to read/parse the iotune group setting for qemu. Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 8c9876c..1212e5a 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3854,6 +3854,14 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec" /** + * VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME: + * + * Macro represents the group name to be used, + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME "blkdeviotune.group_name" + +/** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: * * Macro represents the length in seconds allowed for a burst period diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..f41a783 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1644,6 +1644,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->vendor); VIR_FREE(def->product); VIR_FREE(def->domain_name); + VIR_FREE(def->blkdeviotune.group_name); virDomainDeviceInfoClear(&def->info); virObjectUnref(def->privateData); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..cb32685 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -548,6 +548,7 @@ struct _virDomainBlockIoTuneInfo { unsigned long long read_iops_sec_max; unsigned long long write_iops_sec_max; unsigned long long size_iops_sec; + char *group_name; unsigned long long total_bytes_sec_max_length; unsigned long long read_bytes_sec_max_length; unsigned long long write_bytes_sec_max_length; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8cec49..4af1cb0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -114,7 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP 14 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 20 #define QEMU_NB_NUMA_PARAM 2 @@ -17316,7 +17317,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, bool set_iops_max, bool set_size_iops, bool set_bytes_max_length, - bool set_iops_max_length) + bool set_iops_max_length, + bool set_group_name) { #define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ if (!BOOL) { \ @@ -17333,6 +17335,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, if (!set_size_iops) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!set_group_name) + VIR_STEAL_PTR(newinfo->group_name, oldinfo->group_name); /* 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 @@ -17389,9 +17393,11 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, bool set_bytes_max = false; bool set_iops_max = false; bool set_size_iops = false; + bool set_group_name = false; bool set_bytes_max_length = false; bool set_iops_max_length = false; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17428,6 +17434,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, @@ -17507,6 +17515,16 @@ 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); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + if (VIR_STRDUP(info.group_name, params->value.s) < 0) + goto endjob; + set_group_name = true; + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto endjob; + } SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17559,6 +17577,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); + supportGroupNameOption = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_GROUP); supportMaxLengthOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH); @@ -17577,6 +17597,13 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + if (!supportGroupNameOption && set_group_name) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the block I/O throttling group parameter is not " + "supported with this QEMU binary")); + goto endjob; + } + if (!supportMaxLengthOptions && (set_iops_max_length || set_bytes_max_length)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -17595,7 +17622,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name); #define CHECK_MAX(val) \ do { \ @@ -17623,6 +17651,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -17652,7 +17681,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name); conf_disk->blkdeviotune = info; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) @@ -17725,8 +17755,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; else if (!virQEMUCapsGet(priv->qemuCaps, - QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + QEMU_CAPS_DRIVE_IOTUNE_GROUP)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + else if (!virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP; } if (*nparams == 0) { @@ -17790,6 +17823,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); + if (*nparams < maxparams && + virTypedParameterAssign(¶ms[(*nparams)++], + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING, + reply.group_name) < 0) + goto endjob; + 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); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a5e14b2..950d476 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3428,6 +3428,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3437,6 +3438,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, if (mon->json) return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); else return qemuMonitorTextSetBlockIoThrottle(mon, device, info); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c3133c4..7476c11 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -877,6 +877,7 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions); int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c13832..7a08253 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4524,6 +4524,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i); virJSONValuePtr inserted; const char *current_dev; + const char *group_name; if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4549,7 +4550,6 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, "was not in expected format")); goto cleanup; } - GET_THROTTLE_STATS("bps", total_bytes_sec); GET_THROTTLE_STATS("bps_rd", read_bytes_sec); GET_THROTTLE_STATS("bps_wr", write_bytes_sec); @@ -4563,6 +4563,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); + + if ((group_name = virJSONValueObjectGetString(inserted, "group"))) { + if (VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; + } + 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); @@ -4591,17 +4597,23 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr result = NULL; + char *group_name = NULL; + + if (supportGroupNameOption && VIR_STRDUP(group_name, info->group_name) < 0) + return -1; /* 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 and - * similarly when !supportMaxLengthOptions */ + * similarly when !supportGroupNameOption as well as when + * when !supportMaxLengthOptions */ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", "s:device", device, "U:bps", info->total_bytes_sec, @@ -4618,6 +4630,8 @@ 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, + !supportGroupNameOption ? NULL : + "s:group", group_name, !supportMaxLengthOptions ? NULL : "P:bps_max_length", info->total_bytes_sec_max_length, @@ -4633,7 +4647,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, info->write_iops_sec_max_length, NULL); if (!cmd) - return -1; + goto cleanup; if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) goto cleanup; @@ -4657,6 +4671,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, ret = 0; cleanup: + VIR_FREE(group_name); virJSONValueFree(cmd); virJSONValueFree(result); return ret; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 77b2e02..55bdfe8 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -328,6 +328,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions); int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d8fd958..19bfc2a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -67,12 +67,13 @@ 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," +" \"group\": \"group14\"," +" \"bps_max_length\": 15," +" \"bps_rd_max_length\": 16," +" \"bps_wr_max_length\": 17," +" \"iops_max_length\": 18," +" \"iops_rd_max_length\": 19," +" \"iops_wr_max_length\": 20," " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," " \"encryption_key_missing\": false" " }," @@ -2002,7 +2003,9 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1; - expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20}; + if (VIR_STRDUP(expectedInfo.group_name, "group14") < 0) + return -1; if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || qemuMonitorTestAddItemParams(test, "block_set_io_throttle", @@ -2014,12 +2017,13 @@ 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", + "group", "\"group14\"", + "bps_max_length", "15", + "bps_rd_max_length", "16", + "bps_wr_max_length", "17", + "iops_max_length", "18", + "iops_rd_max_length", "19", + "iops_wr_max_length", "20", NULL, NULL) < 0) goto cleanup; @@ -2027,19 +2031,55 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) "drive-virtio-disk0", &info) < 0) goto cleanup; - if (memcmp(&info, &expectedInfo, sizeof(info)) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid @info"); +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + goto cleanup; \ + } \ + if (info.field##_max != expectedInfo.field##_max) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max=%llu != expected=%llu", \ + #field, info.field##_max, expectedInfo.field##_max); \ + goto cleanup; \ + } \ + if (info.field##_max_length != expectedInfo.field##_max_length) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max_length=%llu != expected=%llu", \ + #field, info.field##_max_length, \ + expectedInfo.field##_max_length); \ + goto cleanup; \ + } + VALIDATE_IOTUNE(total_bytes_sec); + VALIDATE_IOTUNE(read_bytes_sec); + VALIDATE_IOTUNE(write_bytes_sec); + VALIDATE_IOTUNE(total_iops_sec); + VALIDATE_IOTUNE(read_iops_sec); + VALIDATE_IOTUNE(write_iops_sec); + if (info.size_iops_sec != expectedInfo.size_iops_sec) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.size_iops_sec=%llu != expected=%llu", + info.size_iops_sec, expectedInfo.size_iops_sec); + goto cleanup; + } + if (STRNEQ(info.group_name, expectedInfo.group_name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.group_name=%s != expected=%s", + info.group_name, expectedInfo.group_name); goto cleanup; } +#undef VALIDATE_IOTUNE if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), "drive-virtio-disk1", &info, true, - true) < 0) + true, true) < 0) goto cleanup; ret = 0; cleanup: + VIR_FREE(info.group_name); + VIR_FREE(expectedInfo.group_name); qemuMonitorTestFree(test); return ret; } -- 2.7.4
On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote:
Add support to read/parse the iotune group setting for qemu.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 8c9876c..1212e5a 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3854,6 +3854,14 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec"
/** + * VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME: + * + * Macro represents the group name to be used, + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME "blkdeviotune.group_name" + +/** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: * * Macro represents the length in seconds allowed for a burst period diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..f41a783 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1644,6 +1644,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->vendor); VIR_FREE(def->product); VIR_FREE(def->domain_name); + VIR_FREE(def->blkdeviotune.group_name); virDomainDeviceInfoClear(&def->info); virObjectUnref(def->privateData);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..cb32685 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -548,6 +548,7 @@ struct _virDomainBlockIoTuneInfo { unsigned long long read_iops_sec_max; unsigned long long write_iops_sec_max; unsigned long long size_iops_sec; + char *group_name; unsigned long long total_bytes_sec_max_length; unsigned long long read_bytes_sec_max_length; unsigned long long write_bytes_sec_max_length; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8cec49..4af1cb0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -114,7 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver");
#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP 14 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 20
#define QEMU_NB_NUMA_PARAM 2
@@ -17316,7 +17317,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, bool set_iops_max, bool set_size_iops, bool set_bytes_max_length, - bool set_iops_max_length) + bool set_iops_max_length, + bool set_group_name) { #define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ if (!BOOL) { \ @@ -17333,6 +17335,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo,
if (!set_size_iops) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!set_group_name) + VIR_STEAL_PTR(newinfo->group_name, oldinfo->group_name);
This changes the first structure, and because of that it can leak in qemuDomainSetBlockIoTune() where it is not free()'d (because it didn't have to be before). For example if qemuMonitorSetBlockIoThrottle() fails. It also looks like it might be uninitialized because instead of doing ifo = {0}; we call memset() lot of lines later. Just something you might need to wonder about when changing it.
/* 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 @@ -17389,9 +17393,11 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, bool set_bytes_max = false; bool set_iops_max = false; bool set_size_iops = false; + bool set_group_name = false; bool set_bytes_max_length = false; bool set_iops_max_length = false; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17428,6 +17434,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, @@ -17507,6 +17515,16 @@ 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); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + if (VIR_STRDUP(info.group_name, params->value.s) < 0) + goto endjob; + set_group_name = true; + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto endjob; + }
SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17559,6 +17577,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); + supportGroupNameOption = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_GROUP); supportMaxLengthOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
@@ -17577,6 +17597,13 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
+ if (!supportGroupNameOption && set_group_name) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the block I/O throttling group parameter is not " + "supported with this QEMU binary")); + goto endjob; + } + if (!supportMaxLengthOptions && (set_iops_max_length || set_bytes_max_length)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -17595,7 +17622,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name);
#define CHECK_MAX(val) \ do { \ @@ -17623,6 +17651,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -17652,7 +17681,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name); conf_disk->blkdeviotune = info; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) @@ -17725,8 +17755,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; else if (!virQEMUCapsGet(priv->qemuCaps, - QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + QEMU_CAPS_DRIVE_IOTUNE_GROUP)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + else if (!virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP;
This won't work in a corner-case if there's a QEMU out there that has backported support for just some things out of the order in which they were introduced. I don't know why we don't just easily do: if (supportGroupNameOption) maxparams++; if (supportMaxLengthOptions) maxparams += 6; where the numbers can be macros as well, of course.
}
if (*nparams == 0) { @@ -17790,6 +17823,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec);
+ if (*nparams < maxparams && + virTypedParameterAssign(¶ms[(*nparams)++], + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING, + reply.group_name) < 0) + goto endjob; +
As much as I'm looking at it, I still can't find why can't you use BLOCK_IOTUNE_ASSIGN. What's the reason for that?
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); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a5e14b2..950d476 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3428,6 +3428,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3437,6 +3438,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, if (mon->json) return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); else return qemuMonitorTextSetBlockIoThrottle(mon, device, info); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c3133c4..7476c11 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -877,6 +877,7 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions);
int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c13832..7a08253 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4524,6 +4524,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i); virJSONValuePtr inserted; const char *current_dev; + const char *group_name;
if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4549,7 +4550,6 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, "was not in expected format")); goto cleanup; } - GET_THROTTLE_STATS("bps", total_bytes_sec); GET_THROTTLE_STATS("bps_rd", read_bytes_sec); GET_THROTTLE_STATS("bps_wr", write_bytes_sec); @@ -4563,6 +4563,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); + + if ((group_name = virJSONValueObjectGetString(inserted, "group"))) { + if (VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; + }
If you want to save some indentation, VIR_STRDUP will handle NULLs nicely.
+ 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); @@ -4591,17 +4597,23 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr result = NULL; + char *group_name = NULL; + + if (supportGroupNameOption && VIR_STRDUP(group_name, info->group_name) < 0) + return -1;
What's the reason for this? qemuMonitorJSONMakeCommand() doesn't steals the pointer.
/* 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 and - * similarly when !supportMaxLengthOptions */ + * similarly when !supportGroupNameOption as well as when + * when !supportMaxLengthOptions */ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", "s:device", device, "U:bps", info->total_bytes_sec, @@ -4618,6 +4630,8 @@ 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, + !supportGroupNameOption ? NULL : + "s:group", group_name, !supportMaxLengthOptions ? NULL : "P:bps_max_length", info->total_bytes_sec_max_length,
Same applies here with the back-porting.
@@ -4633,7 +4647,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, info->write_iops_sec_max_length, NULL); if (!cmd) - return -1; + goto cleanup;
if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) goto cleanup; @@ -4657,6 +4671,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
ret = 0; cleanup: + VIR_FREE(group_name); virJSONValueFree(cmd); virJSONValueFree(result); return ret;
These two hunks can be skipped if there's no need for the strdup.
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 77b2e02..55bdfe8 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -328,6 +328,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions);
int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d8fd958..19bfc2a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -67,12 +67,13 @@ 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," +" \"group\": \"group14\"," +" \"bps_max_length\": 15," +" \"bps_rd_max_length\": 16," +" \"bps_wr_max_length\": 17," +" \"iops_max_length\": 18," +" \"iops_rd_max_length\": 19," +" \"iops_wr_max_length\": 20," " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," " \"encryption_key_missing\": false" " }," @@ -2002,7 +2003,9 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1;
- expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20}; + if (VIR_STRDUP(expectedInfo.group_name, "group14") < 0) + return -1;
if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || qemuMonitorTestAddItemParams(test, "block_set_io_throttle", @@ -2014,12 +2017,13 @@ 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", + "group", "\"group14\"", + "bps_max_length", "15", + "bps_rd_max_length", "16", + "bps_wr_max_length", "17", + "iops_max_length", "18", + "iops_rd_max_length", "19", + "iops_wr_max_length", "20", NULL, NULL) < 0) goto cleanup;
@@ -2027,19 +2031,55 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) "drive-virtio-disk0", &info) < 0) goto cleanup;
- if (memcmp(&info, &expectedInfo, sizeof(info)) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid @info"); +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + goto cleanup; \ + } \ + if (info.field##_max != expectedInfo.field##_max) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max=%llu != expected=%llu", \ + #field, info.field##_max, expectedInfo.field##_max); \ + goto cleanup; \ + } \ + if (info.field##_max_length != expectedInfo.field##_max_length) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max_length=%llu != expected=%llu", \ + #field, info.field##_max_length, \ + expectedInfo.field##_max_length); \ + goto cleanup; \ + } + VALIDATE_IOTUNE(total_bytes_sec); + VALIDATE_IOTUNE(read_bytes_sec); + VALIDATE_IOTUNE(write_bytes_sec); + VALIDATE_IOTUNE(total_iops_sec); + VALIDATE_IOTUNE(read_iops_sec); + VALIDATE_IOTUNE(write_iops_sec); + if (info.size_iops_sec != expectedInfo.size_iops_sec) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.size_iops_sec=%llu != expected=%llu", + info.size_iops_sec, expectedInfo.size_iops_sec); + goto cleanup; + } + if (STRNEQ(info.group_name, expectedInfo.group_name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.group_name=%s != expected=%s", + info.group_name, expectedInfo.group_name); goto cleanup; } +#undef VALIDATE_IOTUNE
I don't feel like this above is cleaner than comparing the strings, clearing them out and then keeping the memcmp(). Well, at least we get better error. But it could be in its own function.
if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), "drive-virtio-disk1", &info, true, - true) < 0) + true, true) < 0) goto cleanup;
ret = 0; cleanup: + VIR_FREE(info.group_name); + VIR_FREE(expectedInfo.group_name); qemuMonitorTestFree(test); return ret; } -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 02, 2016 at 03:24:10PM +0100, Martin Kletzander wrote:
On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote:
Add support to read/parse the iotune group setting for qemu.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-)
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d8fd958..19bfc2a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2002,7 +2003,9 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1;
- expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20};
Also I believe the numbers are different just to check that they won't be swapped or something. You can keep them, it doesn't have to be the number of the parameter.
On 11/02/2016 10:24 AM, Martin Kletzander wrote:
On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote:
Add support to read/parse the iotune group setting for qemu.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 8c9876c..1212e5a 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -3854,6 +3854,14 @@ typedef void (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC "blkdeviotune.size_iops_sec"
/** + * VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME: + * + * Macro represents the group name to be used, + * as VIR_TYPED_PARAM_STRING. + */ +# define VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME "blkdeviotune.group_name" + +/** * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: * * Macro represents the length in seconds allowed for a burst period diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03506cb..f41a783 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1644,6 +1644,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->vendor); VIR_FREE(def->product); VIR_FREE(def->domain_name); + VIR_FREE(def->blkdeviotune.group_name); virDomainDeviceInfoClear(&def->info); virObjectUnref(def->privateData);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24aa79c..cb32685 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -548,6 +548,7 @@ struct _virDomainBlockIoTuneInfo { unsigned long long read_iops_sec_max; unsigned long long write_iops_sec_max; unsigned long long size_iops_sec; + char *group_name; unsigned long long total_bytes_sec_max_length; unsigned long long read_bytes_sec_max_length; unsigned long long write_bytes_sec_max_length; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b8cec49..4af1cb0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -114,7 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver");
#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP 14 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 20
#define QEMU_NB_NUMA_PARAM 2
@@ -17316,7 +17317,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, bool set_iops_max, bool set_size_iops, bool set_bytes_max_length, - bool set_iops_max_length) + bool set_iops_max_length, + bool set_group_name) { #define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ if (!BOOL) { \ @@ -17333,6 +17335,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo,
if (!set_size_iops) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!set_group_name) + VIR_STEAL_PTR(newinfo->group_name, oldinfo->group_name);
This changes the first structure, and because of that it can leak in qemuDomainSetBlockIoTune() where it is not free()'d (because it didn't have to be before). For example if qemuMonitorSetBlockIoThrottle() fails. It also looks like it might be uninitialized because instead of doing ifo = {0}; we call memset() lot of lines later. Just something you might need to wonder about when changing it.
Good catch on the leak... 'info' is filled in from parameters provided via 'params', then any values currently set in the saved domain values (disk->blkdeviotune and conf_disk->blkdeviotune) are defaulted to the 'info' values since shortly after the setting of the default values, the 'info' value is assigned into the saved domain value location again via the structure assignment... So, I'll add a couple of "info.group_name = NULL" after the structure assignments and a VIR_FREE(info.group_name) The memset(info, 0, sizeof(info)); is done before any call to set the defaults, so unless I'm missing something subtle that you're pointing out, I'll assume the "either" info = {0} or memset would do the proper initialization. If you'd prefer that the {0} is used, that's fine I can change it. Since the memset had been used when I started this exercise, I didn't change it.
/* 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 @@ -17389,9 +17393,11 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, bool set_bytes_max = false; bool set_iops_max = false; bool set_size_iops = false; + bool set_group_name = false; bool set_bytes_max_length = false; bool set_iops_max_length = false; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17428,6 +17434,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING,
VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, VIR_TYPED_PARAM_ULLONG,
VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, @@ -17507,6 +17515,16 @@ 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); + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + if (VIR_STRDUP(info.group_name, params->value.s) < 0) + goto endjob; + set_group_name = true; + if (virTypedParamsAddString(&eventParams, &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto endjob; + }
SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17559,6 +17577,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); + supportGroupNameOption = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_GROUP); supportMaxLengthOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
@@ -17577,6 +17597,13 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
+ if (!supportGroupNameOption && set_group_name) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the block I/O throttling group parameter is not " + "supported with this QEMU binary")); + goto endjob; + } + if (!supportMaxLengthOptions && (set_iops_max_length || set_bytes_max_length)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -17595,7 +17622,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name);
#define CHECK_MAX(val) \ do { \ @@ -17623,6 +17651,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -17652,7 +17681,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_bytes, set_iops, set_bytes_max, set_iops_max, set_size_iops, set_bytes_max_length, - set_iops_max_length); + set_iops_max_length, + set_group_name); conf_disk->blkdeviotune = info; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) @@ -17725,8 +17755,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; else if (!virQEMUCapsGet(priv->qemuCaps, - QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + QEMU_CAPS_DRIVE_IOTUNE_GROUP)) maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + else if (!virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP;
This won't work in a corner-case if there's a QEMU out there that has backported support for just some things out of the order in which they were introduced. I don't know why we don't just easily do:
if (supportGroupNameOption) maxparams++; if (supportMaxLengthOptions) maxparams += 6;
where the numbers can be macros as well, of course.
We start with 20 (assume everything is possible) and then reset max to some lower value based upon what capabilities exist assuming that no one would really backport a 2.6 feature (_LENGTH) into 2.4 or a 2.4 (group_name) feature into 1.7 or a 1.7 feature (_MAX) into something before 1.7. Furthermore the _MAX and _LENGTH are "tied" together. That is there's no way for _LENGTH to exist without _MAX. The only oddball is the GROUP_NAME, but it's incestuously related as well ;-) I'll make a separate patch before this one to address this.
}
if (*nparams == 0) { @@ -17790,6 +17823,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec);
+ if (*nparams < maxparams && + virTypedParameterAssign(¶ms[(*nparams)++], + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + VIR_TYPED_PARAM_STRING, + reply.group_name) < 0) + goto endjob; +
As much as I'm looking at it, I still can't find why can't you use BLOCK_IOTUNE_ASSIGN. What's the reason for that?
The macro uses VIR_TYPED_PARAM_ULLONG while the above uses VIR_TYPED_PARAM_STRING
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); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a5e14b2..950d476 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3428,6 +3428,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3437,6 +3438,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, if (mon->json) return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, supportMaxOptions, + supportGroupNameOption,
supportMaxLengthOptions); else return qemuMonitorTextSetBlockIoThrottle(mon, device, info); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c3133c4..7476c11 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -877,6 +877,7 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions);
int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c13832..7a08253 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4524,6 +4524,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i); virJSONValuePtr inserted; const char *current_dev; + const char *group_name;
if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4549,7 +4550,6 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, "was not in expected format")); goto cleanup; } - GET_THROTTLE_STATS("bps", total_bytes_sec); GET_THROTTLE_STATS("bps_rd", read_bytes_sec); GET_THROTTLE_STATS("bps_wr", write_bytes_sec); @@ -4563,6 +4563,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); + + if ((group_name = virJSONValueObjectGetString(inserted, "group"))) { + if (VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; + }
If you want to save some indentation, VIR_STRDUP will handle NULLs nicely.
OK... I was blindly copying (to a degree) the _OPTIONAL macro logic.
+ 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); @@ -4591,17 +4597,23 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr result = NULL; + char *group_name = NULL; + + if (supportGroupNameOption && VIR_STRDUP(group_name, info->group_name) < 0) + return -1;
What's the reason for this? qemuMonitorJSONMakeCommand() doesn't steals the pointer.
This is a relic of how I initially approached adding group_name support. I was going to go with a number, then generate a group_name based on that number and a self defined prefix (e.g. a virAsprintf(group_name, "libvirt_iotune_group", info->group_num);) - that persisted in patch 4 were you can see the tests used that... Anyway, I'll undo that here...
/* 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 and - * similarly when !supportMaxLengthOptions */ + * similarly when !supportGroupNameOption as well as when + * when !supportMaxLengthOptions */ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", "s:device", device, "U:bps", info->total_bytes_sec, @@ -4618,6 +4630,8 @@ 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, + !supportGroupNameOption ? NULL : + "s:group", group_name, !supportMaxLengthOptions ? NULL : "P:bps_max_length", info->total_bytes_sec_max_length,
Same applies here with the back-porting.
Similar to the previous, it does exist this way currently, but I'll make a separate patch before this one for this as well to address the concern.
@@ -4633,7 +4647,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, info->write_iops_sec_max_length, NULL); if (!cmd) - return -1; + goto cleanup;
if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) goto cleanup; @@ -4657,6 +4671,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
ret = 0; cleanup: + VIR_FREE(group_name); virJSONValueFree(cmd); virJSONValueFree(result); return ret;
These two hunks can be skipped if there's no need for the strdup.
yep.
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 77b2e02..55bdfe8 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -328,6 +328,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions);
int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d8fd958..19bfc2a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -67,12 +67,13 @@ 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," +" \"group\": \"group14\"," +" \"bps_max_length\": 15," +" \"bps_rd_max_length\": 16," +" \"bps_wr_max_length\": 17," +" \"iops_max_length\": 18," +" \"iops_rd_max_length\": 19," +" \"iops_wr_max_length\": 20," " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," " \"encryption_key_missing\": false" " }," @@ -2002,7 +2003,9 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) if (!test) return -1;
- expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20}; + if (VIR_STRDUP(expectedInfo.group_name, "group14") < 0) + return -1;
FWIW: From your followup... Using the numbers as is also makes it easier to do the "math" of the maxparams logic.
if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || qemuMonitorTestAddItemParams(test, "block_set_io_throttle", @@ -2014,12 +2017,13 @@ 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", + "group", "\"group14\"", + "bps_max_length", "15", + "bps_rd_max_length", "16", + "bps_wr_max_length", "17", + "iops_max_length", "18", + "iops_rd_max_length", "19", + "iops_wr_max_length", "20", NULL, NULL) < 0) goto cleanup;
@@ -2027,19 +2031,55 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) "drive-virtio-disk0", &info) < 0) goto cleanup;
- if (memcmp(&info, &expectedInfo, sizeof(info)) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Invalid @info"); +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + goto cleanup; \ + } \ + if (info.field##_max != expectedInfo.field##_max) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max=%llu != expected=%llu", \ + #field, info.field##_max, expectedInfo.field##_max); \ + goto cleanup; \ + } \ + if (info.field##_max_length != expectedInfo.field##_max_length) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s_max_length=%llu != expected=%llu", \ + #field, info.field##_max_length, \ + expectedInfo.field##_max_length); \ + goto cleanup; \ + } + VALIDATE_IOTUNE(total_bytes_sec); + VALIDATE_IOTUNE(read_bytes_sec); + VALIDATE_IOTUNE(write_bytes_sec); + VALIDATE_IOTUNE(total_iops_sec); + VALIDATE_IOTUNE(read_iops_sec); + VALIDATE_IOTUNE(write_iops_sec); + if (info.size_iops_sec != expectedInfo.size_iops_sec) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.size_iops_sec=%llu != expected=%llu", + info.size_iops_sec, expectedInfo.size_iops_sec); + goto cleanup; + } + if (STRNEQ(info.group_name, expectedInfo.group_name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "info.group_name=%s != expected=%s", + info.group_name, expectedInfo.group_name); goto cleanup; } +#undef VALIDATE_IOTUNE
I don't feel like this above is cleaner than comparing the strings, clearing them out and then keeping the memcmp(). Well, at least we get better error. But it could be in its own function.
I'll make a separate function. I prefer the changes because they let one know which parameter was incorrect rather than just a "Invalid @info". Tks - John
if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), "drive-virtio-disk1", &info, true, - true) < 0) + true, true) < 0) goto cleanup;
ret = 0; cleanup: + VIR_FREE(info.group_name); + VIR_FREE(expectedInfo.group_name); qemuMonitorTestFree(test); return ret; } -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote:
Add support to read/parse the iotune group setting for qemu.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-)
[...]
@@ -17316,7 +17317,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, bool set_iops_max, bool set_size_iops, bool set_bytes_max_length, - bool set_iops_max_length) + bool set_iops_max_length, + bool set_group_name) {
Additionally to what Martin suggested in his review, I'd like to reiterate the idea of converting the 8 booleans to OR'd flags. Erik
On 11/02/2016 12:37 PM, Erik Skultety wrote:
On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote:
Add support to read/parse the iotune group setting for qemu.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 8 +++++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- src/qemu/qemu_monitor.c | 2 ++ src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 21 ++++++++++-- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 74 +++++++++++++++++++++++++++++++--------- 9 files changed, 134 insertions(+), 25 deletions(-)
[...]
@@ -17316,7 +17317,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, bool set_iops_max, bool set_size_iops, bool set_bytes_max_length, - bool set_iops_max_length) + bool set_iops_max_length, + bool set_group_name) {
Additionally to what Martin suggested in his review, I'd like to reiterate the idea of converting the 8 booleans to OR'd flags.
OK - I'll make an adjustment in a separate patch. John
Modify _virDomainBlockIoTuneInfo and rng schema to support the group_name option for iotune throttling. Document the new value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 11 ++++ docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 9 ++++ .../qemuxml2argv-blkdeviotune-group-num.xml | 61 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-group-num.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 88 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c70377b..8d30737 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2622,6 +2622,17 @@ <span class="since">Throughput limits since 1.2.11 and QEMU 1.7</span> </p> </dd> + <dt><code>group_name</code></dt> + <dd>The optional <code>group_name</code> provides the cability + to share I/O throttling quota between multiple drives. This + prevents end-users from circumventing a hosting provider's + throttling policy by splitting 1 large drive in N small drives + and getting N times the normal throttling quota. Any name may + be used. + <p> + <span class="since">group_name since 2.5.0 and QEMU 2.4</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 diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index dba9187..9373a65 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4963,6 +4963,11 @@ <data type="unsignedLong"/> </element> </optional> + <optional> + <element name="group_name"> + <text/> + </element> + </optional> <choice> <element name="total_bytes_sec_max_length"> <data type="unsignedLong"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f41a783..8ad52d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7210,6 +7210,9 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, PARSE_IOTUNE(read_iops_sec_max_length); PARSE_IOTUNE(write_iops_sec_max_length); + def->blkdeviotune.group_name = + virXPathString("string(./iotune/group_name)", ctxt); + if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) || (def->blkdeviotune.total_bytes_sec && @@ -20388,6 +20391,7 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.read_iops_sec_max || def->blkdeviotune.write_iops_sec_max || def->blkdeviotune.size_iops_sec || + def->blkdeviotune.group_name || def->blkdeviotune.total_bytes_sec_max_length || def->blkdeviotune.read_bytes_sec_max_length || def->blkdeviotune.write_bytes_sec_max_length || @@ -20416,6 +20420,11 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); } + if (def->blkdeviotune.group_name) { + virBufferAsprintf(buf, "<group_name>%s</group_name>\n", + def->blkdeviotune.group_name); + } + FORMAT_IOTUNE(total_bytes_sec_max_length); FORMAT_IOTUNE(read_bytes_sec_max_length); FORMAT_IOTUNE(write_bytes_sec_max_length); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml new file mode 100644 index 0000000..e2a9999 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml @@ -0,0 +1,61 @@ +<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> + <group_name>libvirt_iotune_group1</group_name> + </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> + <group_name>libvirt_iotune_group2</group_name> + </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-group-num.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml new file mode 120000 index 0000000..2ed9a68 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 496ed13..6087043 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -609,6 +609,7 @@ mymain(void) DO_TEST("usb-redir-filter-version", NONE); DO_TEST("blkdeviotune", NONE); DO_TEST("blkdeviotune-max", NONE); + DO_TEST("blkdeviotune-group-num", NONE); DO_TEST("blkdeviotune-max-length", NONE); DO_TEST("controller-usb-order", NONE); -- 2.7.4
On Mon, Oct 31, 2016 at 05:23:00PM -0400, John Ferlan wrote:
Modify _virDomainBlockIoTuneInfo and rng schema to support the group_name option for iotune throttling. Document the new value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 11 ++++ docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 9 ++++ .../qemuxml2argv-blkdeviotune-group-num.xml | 61 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-group-num.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 88 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c70377b..8d30737 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2622,6 +2622,17 @@ <span class="since">Throughput limits since 1.2.11 and QEMU 1.7</span> </p> </dd> + <dt><code>group_name</code></dt> + <dd>The optional <code>group_name</code> provides the cability + to share I/O throttling quota between multiple drives. This + prevents end-users from circumventing a hosting provider's + throttling policy by splitting 1 large drive in N small drives + and getting N times the normal throttling quota. Any name may + be used.
I'm really scared when I see that "any name may be used", I would rather put a sensible set of restrictions here that we can remove later, but let's go with "any name" for now. [...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f41a783..8ad52d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20416,6 +20420,11 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); }
+ if (def->blkdeviotune.group_name) { + virBufferAsprintf(buf, "<group_name>%s</group_name>\n", + def->blkdeviotune.group_name);
Because of that you need to escape it here. Case that requires this would be nice to have in tests.
On 11/02/2016 10:38 AM, Martin Kletzander wrote:
On Mon, Oct 31, 2016 at 05:23:00PM -0400, John Ferlan wrote:
Modify _virDomainBlockIoTuneInfo and rng schema to support the group_name option for iotune throttling. Document the new value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 11 ++++ docs/schemas/domaincommon.rng | 5 ++ src/conf/domain_conf.c | 9 ++++ .../qemuxml2argv-blkdeviotune-group-num.xml | 61 ++++++++++++++++++++++ .../qemuxml2xmlout-blkdeviotune-group-num.xml | 1 + tests/qemuxml2xmltest.c | 1 + 6 files changed, 88 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.xml create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-group-num.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c70377b..8d30737 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2622,6 +2622,17 @@ <span class="since">Throughput limits since 1.2.11 and QEMU 1.7</span> </p> </dd> + <dt><code>group_name</code></dt> + <dd>The optional <code>group_name</code> provides the cability + to share I/O throttling quota between multiple drives. This + prevents end-users from circumventing a hosting provider's + throttling policy by splitting 1 large drive in N small drives + and getting N times the normal throttling quota. Any name may + be used.
I'm really scared when I see that "any name may be used", I would rather put a sensible set of restrictions here that we can remove later, but let's go with "any name" for now.
[...]
OK - I originally what just going to go with a number, but I recall receiving negative reviews on some change where I fabricated a name, so I figured I'd go with any name... Besides qemu would receive "any name" and no sense trying guess/assume someone else's namespace.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f41a783..8ad52d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20416,6 +20420,11 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); }
+ if (def->blkdeviotune.group_name) { + virBufferAsprintf(buf, "<group_name>%s</group_name>\n", + def->blkdeviotune.group_name);
Because of that you need to escape it here. Case that requires this would be nice to have in tests.
OK virBufferEscapeString it is... Similar change for patch 5, plus I made the requested patch 6 adjustment (email reduction initiative). John
Add in the block I/O throttling group parameter to the command line if supported. If not supported, fail command creation. Add the xml2argvtest for testing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 13 +++++++++ .../qemuxml2argv-blkdeviotune-group-num.args | 32 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 49 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b68da3d..cf1512a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1773,6 +1773,15 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, goto error; } + /* block I/O group 2.4 */ + if (disk->blkdeviotune.group_name && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("the block I/O throttling group parameter is " + "not supported with this QEMU binary")); + goto error; + } + /* block I/O throttling length 2.6 */ if ((disk->blkdeviotune.total_bytes_sec_max_length || disk->blkdeviotune.read_bytes_sec_max_length || @@ -1827,6 +1836,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, IOTUNE_ADD(write_iops_sec_max, "iops-write-max"); IOTUNE_ADD(size_iops_sec, "iops-size"); + if (disk->blkdeviotune.group_name) { + virBufferAsprintf(&opt, ",throttling.group=%s", + disk->blkdeviotune.group_name); + } IOTUNE_ADD(total_bytes_sec_max_length, "bps-total-max-length"); IOTUNE_ADD(read_bytes_sec_max_length, "bps-read-max-length"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args new file mode 100644 index 0000000..b5a5cc0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args @@ -0,0 +1,32 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\ +cache=none,throttling.bps-total=5000,throttling.iops-total=6000,\ +throttling.bps-total-max=10000,throttling.iops-total-max=11000,\ +throttling.group=libvirt_iotune_group1 \ +-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,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,throttling.group=libvirt_iotune_group2 \ +-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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 90d6aaf..9234a2d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1531,6 +1531,10 @@ mymain(void) DO_TEST("blkdeviotune-max", QEMU_CAPS_DRIVE_IOTUNE, QEMU_CAPS_DRIVE_IOTUNE_MAX); + DO_TEST("blkdeviotune-group-num", + QEMU_CAPS_DRIVE_IOTUNE, + QEMU_CAPS_DRIVE_IOTUNE_MAX, + QEMU_CAPS_DRIVE_IOTUNE_GROUP); DO_TEST("blkdeviotune-max-length", QEMU_CAPS_DRIVE_IOTUNE, QEMU_CAPS_DRIVE_IOTUNE_MAX, -- 2.7.4
On Mon, Oct 31, 2016 at 05:23:01PM -0400, John Ferlan wrote:
Add in the block I/O throttling group parameter to the command line if supported. If not supported, fail command creation.
Add the xml2argvtest for testing.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 13 +++++++++ .../qemuxml2argv-blkdeviotune-group-num.args | 32 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++ 3 files changed, 49 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-group-num.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b68da3d..cf1512a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1827,6 +1836,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, IOTUNE_ADD(write_iops_sec_max, "iops-write-max");
IOTUNE_ADD(size_iops_sec, "iops-size"); + if (disk->blkdeviotune.group_name) { + virBufferAsprintf(&opt, ",throttling.group=%s", + disk->blkdeviotune.group_name);
And you need to escape it for the command-line here just in case it has a comma in it for example.
https://bugzilla.redhat.com/show_bug.cgi?id=1336564 Add the ability to set/display the group_name for block device iotune Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 17 +++++++++++++++++ tools/virsh.pod | 5 ++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 184f64d..9451218 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1263,6 +1263,10 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .type = VSH_OT_INT, .help = N_("I/O size in bytes") }, + {.name = "group_name", + .type = VSH_OT_STRING, + .help = N_("group name to share I/O quota between multiple drives") + }, {.name = "total_bytes_sec_max_length", .type = VSH_OT_ALIAS, .help = "total-bytes-sec-max-length" @@ -1322,6 +1326,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *name, *disk; + const char *group_name = NULL; unsigned long long value; int nparams = 0; int maxparams = 0; @@ -1393,6 +1398,18 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE + rv = vshCommandOptStringQuiet(ctl, cmd, "group_name", &group_name); + if (rv < 0) { + vshError(ctl, "%s", _("Unable to parse group parameter")); + goto cleanup; + } else if (rv > 0) { + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + group_name) < 0) + goto save_error; + } + + if (nparams == 0) { if (virDomainGetBlockIoTune(dom, NULL, NULL, &nparams, flags) != 0) { vshError(ctl, "%s", diff --git a/tools/virsh.pod b/tools/virsh.pod index f278fec..e98cd0f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1139,7 +1139,7 @@ command. [I<read-bytes-sec-max-length>] [I<write-bytes-sec-max-length>]] [[I<total-iops-sec-max-length>] | [I<read-iops-sec-max-length>] [I<write-iops-sec-max-length>]] -[I<size-iops-sec>] +[I<size-iops-sec>] [I<group_name>] Set or query the block disk io parameters for a block device of I<domain>. I<device> specifies a unique target name (<target dev='name'/>) or source @@ -1179,6 +1179,9 @@ read I/O operations limit. I<--write-iops-sec-max-length> specifies duration in seconds to allow maximum write I/O operations limit. I<--size-iops-sec> specifies size I/O operations limit per second. +I<--group_name> specifies group name to share I/O quota between multiple drives. +For a qemu domain, if no name is provided, then the default is to have a single +group for each I<device>. Older versions of virsh only accepted these options with underscore instead of dash, as in I<--total_bytes_sec>. -- 2.7.4
On Mon, Oct 31, 2016 at 05:23:02PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1336564
Add the ability to set/display the group_name for block device iotune
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 17 +++++++++++++++++ tools/virsh.pod | 5 ++++- 2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 184f64d..9451218 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1263,6 +1263,10 @@ static const vshCmdOptDef opts_blkdeviotune[] = { .type = VSH_OT_INT, .help = N_("I/O size in bytes") }, + {.name = "group_name", + .type = VSH_OT_STRING, + .help = N_("group name to share I/O quota between multiple drives") + }, {.name = "total_bytes_sec_max_length", .type = VSH_OT_ALIAS, .help = "total-bytes-sec-max-length" @@ -1322,6 +1326,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; const char *name, *disk; + const char *group_name = NULL; unsigned long long value; int nparams = 0; int maxparams = 0; @@ -1393,6 +1398,18 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) VSH_ADD_IOTUNE(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); #undef VSH_ADD_IOTUNE
+ rv = vshCommandOptStringQuiet(ctl, cmd, "group_name", &group_name);
Just use vshCommandOptStringReq().
+ if (rv < 0) { + vshError(ctl, "%s", _("Unable to parse group parameter")); + goto cleanup; + } else if (rv > 0) { + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + group_name) < 0) + goto save_error; + } + + if (nparams == 0) { if (virDomainGetBlockIoTune(dom, NULL, NULL, &nparams, flags) != 0) { vshError(ctl, "%s", diff --git a/tools/virsh.pod b/tools/virsh.pod index f278fec..e98cd0f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1139,7 +1139,7 @@ command. [I<read-bytes-sec-max-length>] [I<write-bytes-sec-max-length>]] [[I<total-iops-sec-max-length>] | [I<read-iops-sec-max-length>] [I<write-iops-sec-max-length>]] -[I<size-iops-sec>] +[I<size-iops-sec>] [I<group_name>]
Set or query the block disk io parameters for a block device of I<domain>. I<device> specifies a unique target name (<target dev='name'/>) or source @@ -1179,6 +1179,9 @@ read I/O operations limit. I<--write-iops-sec-max-length> specifies duration in seconds to allow maximum write I/O operations limit. I<--size-iops-sec> specifies size I/O operations limit per second. +I<--group_name> specifies group name to share I/O quota between multiple drives. +For a qemu domain, if no name is provided, then the default is to have a single +group for each I<device>.
Older versions of virsh only accepted these options with underscore instead of dash, as in I<--total_bytes_sec>. -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 31, 2016 at 05:22:56PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1336564
Add support for "group" name in <iotune>.
The odd thing about the <iotune> group name is that support was added in qemu after the _max parameters, but before the _max_length - so when building the command line - that needs to be followed.
At least it's far less complicated than the _length parameters.
Apart from what was mentioned, I think the series is fine, just few patches need some love. What was not talked about is ACKable, but I think v2 is appropriate in this case. Martin
participants (3)
-
Erik Skultety -
John Ferlan -
Martin Kletzander