[libvirt] [REPOST PATCH v2 0/9] Add group_name support for <iotune>
This is just a REPOST of the v2 series: http://www.redhat.com/archives/libvir-list/2016-November/msg00363.html The only difference being updating to the current top of tree of commit id '0b4c3bd30'. I did *not* add the NEWS change yet as that's newer than this, but will update NEWS with this once/if this is ACK'd using whatever format becomes agreed upon for the file contents/formatting options. John Ferlan (9): include: Add new "group_name" definition for iotune throttling caps: Add new capability for the iotune group name qemu: Adjust maxparams logic for qemuDomainGetBlockIoTune qemu: Alter qemuMonitorJSONSetBlockIoThrottle command logic qemu: Adjust various bool BlockIoTune set_ values into mask 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 | 177 +++++++++++++-------- src/qemu/qemu_monitor.c | 2 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 97 ++++++----- 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 | 88 +++++++--- .../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, 428 insertions(+), 124 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 cfd090c..02b3ecc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -352,6 +352,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "ivshmem-doorbell", /* 240 */ "query-qmp-schema", "gluster.debug_level", + "drive-iotune-group", ); @@ -2858,6 +2859,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 9163572..4d5d201 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -387,6 +387,7 @@ typedef enum { QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL, /* -device ivshmem-doorbell */ QEMU_CAPS_QUERY_QMP_SCHEMA, /* query-qmp-schema command */ QEMU_CAPS_GLUSTER_DEBUG_LEVEL, /* -drive gluster.debug_level={0..9} */ + 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 bea01c7..cca0361 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -187,6 +187,7 @@ <flag name='machine-iommu'/> <flag name='virtio-vga'/> <flag name='query-qmp-schema'/> + <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 b283444..b42b690 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -162,6 +162,7 @@ <flag name='ivshmem-plain'/> <flag name='ivshmem-doorbell'/> <flag name='query-qmp-schema'/> + <flag name='drive-iotune-group'/> <version>2006000</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 612a747..7b1b0e6 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -162,6 +162,7 @@ <flag name='ivshmem-plain'/> <flag name='ivshmem-doorbell'/> <flag name='query-qmp-schema'/> + <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 5f22ed5..74513f2 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -159,6 +159,7 @@ <flag name='ivshmem-plain'/> <flag name='ivshmem-doorbell'/> <flag name='query-qmp-schema'/> + <flag name='drive-iotune-group'/> <version>2006000</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 e9bd5bb..20fe2e7 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -196,6 +196,7 @@ <flag name='ivshmem-plain'/> <flag name='ivshmem-doorbell'/> <flag name='query-qmp-schema'/> + <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 b6e34b0..29b9df9 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -198,6 +198,7 @@ <flag name='ivshmem-doorbell'/> <flag name='query-qmp-schema'/> <flag name='gluster.debug_level'/> + <flag name='drive-iotune-group'/> <version>2007000</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0)</package> -- 2.7.4
Rather than using negative logic and setting the maxparams to a lesser value based on which capabilities exist, alter the logic to modify the maxparams based on a base value plus the found capabilities. Reduces the chance that some backported feature produces an incorrect value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87d219f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,9 +112,12 @@ 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_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 +#define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS) #define QEMU_NB_NUMA_PARAM 2 @@ -17719,7 +17722,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, virDomainBlockIoTuneInfo reply; char *device = NULL; int ret = -1; - int maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH; + int maxparams; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -17753,11 +17756,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; } - 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)) - maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; + } else { + maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS; } if (*nparams == 0) { -- 2.7.4
On Mon, Nov 21, 2016 at 06:35:48PM -0500, John Ferlan wrote:
Rather than using negative logic and setting the maxparams to a lesser value based on which capabilities exist, alter the logic to modify the maxparams based on a base value plus the found capabilities. Reduces the chance that some backported feature produces an incorrect value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87d219f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,9 +112,12 @@ 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_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 +#define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
#define QEMU_NB_NUMA_PARAM 2
@@ -17719,7 +17722,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, virDomainBlockIoTuneInfo reply; char *device = NULL; int ret = -1; - int maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH; + int maxparams;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -17753,11 +17756,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; }
- 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)) - maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; + } else { + maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS; }
I'm curious about the else branch. Can you elaborate more on why do you assume qemu supports all the features when you retrieve a persistentdef vs livedef? From what I understand, this is one of the APIs that you have to call twice, since the RPC layer does not allocate a structure of an appropriate size automatically. So with the first run, you say, please allocate a structure of size maxparams equal _ALL_PARAMS (without knowing that qemu actually supports all these features) and rerun. Then during the second run, you run BLOCK_IOTUNE_ASSIGN on all of them, what am I missing? Erik
if (*nparams == 0) { -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/25/2016 10:28 AM, Erik Skultety wrote:
On Mon, Nov 21, 2016 at 06:35:48PM -0500, John Ferlan wrote:
Rather than using negative logic and setting the maxparams to a lesser value based on which capabilities exist, alter the logic to modify the maxparams based on a base value plus the found capabilities. Reduces the chance that some backported feature produces an incorrect value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87d219f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,9 +112,12 @@ 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_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 +#define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
#define QEMU_NB_NUMA_PARAM 2
@@ -17719,7 +17722,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, virDomainBlockIoTuneInfo reply; char *device = NULL; int ret = -1; - int maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH;
NB: "current" persistent def max - moved just to make it clearer what the max was vis-a-vis persistent vs. live.
+ int maxparams;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -17753,11 +17756,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; }
- 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)) - maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; + } else { + maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS; }
I'm curious about the else branch. Can you elaborate more on why do you assume qemu supports all the features when you retrieve a persistentdef vs livedef?
History mainly. How does one choose otherwise? When it's persistent, then you can get/return all 'valid' values vs. when it's live you can only get/return those that the domain can retrieve. Typically the order of fetching is done weighted towards which version the value was added. That's why I stuck group_name between the "max" and "max_length" - so it has a chance of being fetched instead of one max_length value as I know it was added to qemu in between the two.
From what I understand, this is one of the APIs that you have to call twice, since the RPC layer does not allocate a structure of an appropriate size automatically. So with the first run, you say, please allocate a structure of size maxparams equal _ALL_PARAMS (without knowing that qemu actually supports all these features) and rerun.
True - it's one of those pass 0 or pass 0 and/or some other value in order to indicate that you want to get a count so that you can allocate an appropriately sized buffer.
Then during the second run, you run BLOCK_IOTUNE_ASSIGN on all of them, what am I missing?
Not quite sure what you're asking - if dig into the macro you'll note a comparison "if (*nparams < maxparams && " which essentially ensures that we'll only ever fill in as many as we should. On the "second" pass through the code we'll hit the else of the "if (*nparams == 0) {" which can further 'refine' maxparams to be whatever was passed in *nparams (e.g. whatever was allocated by the caller). Similar to any other chunk of code there certainly is assumption that the allocation was done correctly. FWIW: Let's say on your first pass the domain isn't running and you return ALL, but on the second pass the domain is running and maxparams gets set to MAX. That means we'll fetch up to 'MAX' from the running domain with the other entries being left at whatever they were initialized to by the caller. We'll also return MAX (a lesser value) in nparams from the call indicating to the caller that less values were fetched than the size of the buffer. It's up to the caller to recognize that if they so desire. They could also print out whatever size they passed in. John
Erik
if (*nparams == 0) { -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 28, 2016 at 06:53:35AM -0500, John Ferlan wrote:
On 11/25/2016 10:28 AM, Erik Skultety wrote:
On Mon, Nov 21, 2016 at 06:35:48PM -0500, John Ferlan wrote:
Rather than using negative logic and setting the maxparams to a lesser value based on which capabilities exist, alter the logic to modify the maxparams based on a base value plus the found capabilities. Reduces the chance that some backported feature produces an incorrect value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d039255..87d219f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,9 +112,12 @@ 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_MAX_LENGTH 19 +#define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 +#define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
#define QEMU_NB_NUMA_PARAM 2
@@ -17719,7 +17722,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, virDomainBlockIoTuneInfo reply; char *device = NULL; int ret = -1; - int maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH;
NB: "current" persistent def max - moved just to make it clearer what the max was vis-a-vis persistent vs. live.
+ int maxparams;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -17753,11 +17756,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; }
- 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)) - maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; + maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; + } else { + maxparams = QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS; }
I'm curious about the else branch. Can you elaborate more on why do you assume qemu supports all the features when you retrieve a persistentdef vs livedef?
History mainly. How does one choose otherwise? When it's persistent, then you can get/return all 'valid' values vs. when it's live you can only get/return those that the domain can retrieve. Typically the order of fetching is done weighted towards which version the value was added. That's why I stuck group_name between the "max" and "max_length" - so it has a chance of being fetched instead of one max_length value as I know it was added to qemu in between the two.
From what I understand, this is one of the APIs that you have to call twice, since the RPC layer does not allocate a structure of an appropriate size automatically. So with the first run, you say, please allocate a structure of size maxparams equal _ALL_PARAMS (without knowing that qemu actually supports all these features) and rerun.
True - it's one of those pass 0 or pass 0 and/or some other value in order to indicate that you want to get a count so that you can allocate an appropriately sized buffer.
Then during the second run, you run BLOCK_IOTUNE_ASSIGN on all of them, what am I missing?
Not quite sure what you're asking - if dig into the macro you'll note a comparison "if (*nparams < maxparams && " which essentially ensures that we'll only ever fill in as many as we should. On the "second" pass through the code we'll hit the else of the "if (*nparams == 0) {" which can further 'refine' maxparams to be whatever was passed in *nparams (e.g. whatever was allocated by the caller). Similar to any other chunk of code there certainly is assumption that the allocation was done correctly.
Nah, I missed this line: reply = disk->blkdeviotune; - it all makes more sense now and all the 'invalid' - more like 'unsupported' - attributes will just be zeroed out automatically during the initial allocation. Thanks anyway, Erik
FWIW: Let's say on your first pass the domain isn't running and you return ALL, but on the second pass the domain is running and maxparams gets set to MAX. That means we'll fetch up to 'MAX' from the running domain with the other entries being left at whatever they were initialized to by the caller. We'll also return MAX (a lesser value) in nparams from the call indicating to the caller that less values were fetched than the size of the buffer. It's up to the caller to recognize that if they so desire. They could also print out whatever size they passed in.
John
Erik
if (*nparams == 0) { -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Currently we build the JSON object for the "block_set_io_throttle" command using the knowledge that a NULL for a support*Options boolean would essentially ignore the rest of the arguments. This may not work properly if some capability was backported, plus it just looks rather ugly. So instead, build the "base" arguments and then if the support*Option bool capability is set, add in the arguments on the fly. Then append those arguments to the basic command and send to qemu. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 83 +++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 90d74d5..c2b81b7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4568,45 +4568,55 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr result = NULL; + virJSONValuePtr args = NULL; - /* 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 */ - cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", - "s:device", device, - "U:bps", info->total_bytes_sec, - "U:bps_rd", info->read_bytes_sec, - "U:bps_wr", info->write_bytes_sec, - "U:iops", info->total_iops_sec, - "U:iops_rd", info->read_iops_sec, - "U:iops_wr", info->write_iops_sec, - !supportMaxOptions ? NULL : - "U:bps_max", info->total_bytes_sec_max, - "U:bps_rd_max", info->read_bytes_sec_max, - "U:bps_wr_max", info->write_bytes_sec_max, - "U:iops_max", info->total_iops_sec_max, - "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 : - "P:bps_max_length", - info->total_bytes_sec_max_length, - "P:bps_rd_max_length", - info->read_bytes_sec_max_length, - "P:bps_wr_max_length", - info->write_bytes_sec_max_length, - "P:iops_max_length", - info->total_iops_sec_max_length, - "P:iops_rd_max_length", - info->read_iops_sec_max_length, - "P:iops_wr_max_length", - info->write_iops_sec_max_length, - NULL); - if (!cmd) + if (!(cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", NULL))) return -1; + if (virJSONValueObjectCreate(&args, + "s:device", device, + "U:bps", info->total_bytes_sec, + "U:bps_rd", info->read_bytes_sec, + "U:bps_wr", info->write_bytes_sec, + "U:iops", info->total_iops_sec, + "U:iops_rd", info->read_iops_sec, + "U:iops_wr", info->write_iops_sec, + NULL) < 0) + goto cleanup; + + if (supportMaxOptions && + virJSONValueObjectAdd(args, + "U:bps_max", info->total_bytes_sec_max, + "U:bps_rd_max", info->read_bytes_sec_max, + "U:bps_wr_max", info->write_bytes_sec_max, + "U:iops_max", info->total_iops_sec_max, + "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, + NULL) < 0) + goto cleanup; + + if (supportMaxLengthOptions && + virJSONValueObjectAdd(args, + "P:bps_max_length", + info->total_bytes_sec_max_length, + "P:bps_rd_max_length", + info->read_bytes_sec_max_length, + "P:bps_wr_max_length", + info->write_bytes_sec_max_length, + "P:iops_max_length", + info->total_iops_sec_max_length, + "P:iops_rd_max_length", + info->read_iops_sec_max_length, + "P:iops_wr_max_length", + info->write_iops_sec_max_length, + NULL) < 0) + goto cleanup; + + if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) + goto cleanup; + args = NULL; /* obj owns reference to args now */ + if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) goto cleanup; @@ -4631,6 +4641,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, cleanup: virJSONValueFree(cmd); virJSONValueFree(result); + virJSONValueFree(args); return ret; } -- 2.7.4
Rather than have multiple bool values, create a single enum with bits representing what can be set. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 59 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87d219f..73b58d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17338,34 +17338,38 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, return ret; } +typedef enum { + QEMU_BLOCK_IOTUNE_SET_BYTES = 1 << 0, + QEMU_BLOCK_IOTUNE_SET_IOPS = 1 << 1, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, + QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, +} qemuBlockIoTuneSetFlags; + /* If the user didn't specify bytes limits, inherit previous values; * likewise if the user didn't specify iops limits. */ static void qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, virDomainBlockIoTuneInfoPtr oldinfo, - bool set_bytes, - bool set_iops, - bool set_bytes_max, - bool set_iops_max, - bool set_size_iops, - bool set_bytes_max_length, - bool set_iops_max_length) + qemuBlockIoTuneSetFlags set_flag) { #define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ - if (!BOOL) { \ + if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_##BOOL)) { \ newinfo->total_##FIELD = oldinfo->total_##FIELD; \ newinfo->read_##FIELD = oldinfo->read_##FIELD; \ newinfo->write_##FIELD = oldinfo->write_##FIELD; \ } - SET_IOTUNE_DEFAULTS(set_bytes, bytes_sec); - SET_IOTUNE_DEFAULTS(set_bytes_max, bytes_sec_max); - SET_IOTUNE_DEFAULTS(set_iops, iops_sec); - SET_IOTUNE_DEFAULTS(set_iops_max, iops_sec_max); + SET_IOTUNE_DEFAULTS(BYTES, bytes_sec); + SET_IOTUNE_DEFAULTS(BYTES_MAX, bytes_sec_max); + SET_IOTUNE_DEFAULTS(IOPS, iops_sec); + SET_IOTUNE_DEFAULTS(IOPS_MAX, iops_sec_max); #undef SET_IOTUNE_DEFAULTS - if (!set_size_iops) + if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; /* The length field is handled a bit differently. If not defined/set, @@ -17381,19 +17385,20 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, * our newinfo is clearing, then set max_length based on whether we * have a value in the family set/defined. */ #define SET_MAX_LENGTH(BOOL, FIELD) \ - if (!BOOL) \ + if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_##BOOL)) \ newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length; \ - else if (BOOL && oldinfo->FIELD##_max_length && \ + else if ((set_flag & QEMU_BLOCK_IOTUNE_SET_##BOOL) && \ + oldinfo->FIELD##_max_length && \ !newinfo->FIELD##_max_length) \ newinfo->FIELD##_max_length = (newinfo->FIELD || \ newinfo->FIELD##_max) ? 1 : 0; - SET_MAX_LENGTH(set_bytes_max_length, total_bytes_sec); - SET_MAX_LENGTH(set_bytes_max_length, read_bytes_sec); - SET_MAX_LENGTH(set_bytes_max_length, write_bytes_sec); - SET_MAX_LENGTH(set_iops_max_length, total_iops_sec); - SET_MAX_LENGTH(set_iops_max_length, read_iops_sec); - SET_MAX_LENGTH(set_iops_max_length, write_iops_sec); + SET_MAX_LENGTH(BYTES_MAX_LENGTH, total_bytes_sec); + SET_MAX_LENGTH(BYTES_MAX_LENGTH, read_bytes_sec); + SET_MAX_LENGTH(BYTES_MAX_LENGTH, write_bytes_sec); + SET_MAX_LENGTH(IOPS_MAX_LENGTH, total_iops_sec); + SET_MAX_LENGTH(IOPS_MAX_LENGTH, read_iops_sec); + SET_MAX_LENGTH(IOPS_MAX_LENGTH, write_iops_sec); #undef SET_MAX_LENGTH @@ -17418,13 +17423,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, size_t i; virDomainDiskDefPtr conf_disk = NULL; virDomainDiskDefPtr disk; - bool set_bytes = false; - bool set_iops = false; - 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; + qemuBlockIoTuneSetFlags set_flag = 0; bool supportMaxOptions = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; @@ -17502,7 +17501,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ info.FIELD = param->value.ul; \ - BOOL = true; \ + set_flag |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ @@ -17521,38 +17520,38 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - SET_IOTUNE_FIELD(total_bytes_sec, set_bytes, TOTAL_BYTES_SEC); - SET_IOTUNE_FIELD(read_bytes_sec, set_bytes, READ_BYTES_SEC); - SET_IOTUNE_FIELD(write_bytes_sec, set_bytes, WRITE_BYTES_SEC); - SET_IOTUNE_FIELD(total_iops_sec, set_iops, TOTAL_IOPS_SEC); - SET_IOTUNE_FIELD(read_iops_sec, set_iops, READ_IOPS_SEC); - SET_IOTUNE_FIELD(write_iops_sec, set_iops, WRITE_IOPS_SEC); + 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); - SET_IOTUNE_FIELD(total_bytes_sec_max, set_bytes_max, + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(read_bytes_sec_max, set_bytes_max, + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(write_bytes_sec_max, set_bytes_max, + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(total_iops_sec_max, set_iops_max, + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(read_iops_sec_max, set_iops_max, + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(write_iops_sec_max, set_iops_max, + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); - SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length, + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH, TOTAL_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_bytes_sec_max_length, set_bytes_max_length, + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH, READ_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_bytes_sec_max_length, set_bytes_max_length, + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH, WRITE_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(total_iops_sec_max_length, set_iops_max_length, + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH, TOTAL_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_iops_sec_max_length, set_iops_max_length, + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH, READ_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_iops_sec_max_length, set_iops_max_length, + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH, WRITE_IOPS_SEC_MAX_LENGTH); } @@ -17604,7 +17603,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!supportMaxOptions && - (set_iops_max || set_bytes_max || set_size_iops)) { + (set_flag & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX | + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX | + QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("a block I/O throttling parameter is not " "supported with this QEMU binary")); @@ -17612,7 +17613,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!supportMaxLengthOptions && - (set_iops_max_length || set_bytes_max_length)) { + (set_flag & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH | + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("a block I/O throttling length parameter is not " "supported with this QEMU binary")); @@ -17625,11 +17627,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (!(device = qemuAliasFromDisk(disk))) goto endjob; - qemuDomainSetBlockIoTuneDefaults(&info, &disk->blkdeviotune, - set_bytes, set_iops, set_bytes_max, - set_iops_max, set_size_iops, - set_bytes_max_length, - set_iops_max_length); + qemuDomainSetBlockIoTuneDefaults(&info, &disk->blkdeviotune, set_flag); #define CHECK_MAX(val) \ do { \ @@ -17683,10 +17681,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, - set_bytes, set_iops, set_bytes_max, - set_iops_max, set_size_iops, - set_bytes_max_length, - set_iops_max_length); + set_flag); conf_disk->blkdeviotune = info; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) -- 2.7.4
On Mon, Nov 21, 2016 at 06:35:50PM -0500, John Ferlan wrote:
Rather than have multiple bool values, create a single enum with bits representing what can be set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 59 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87d219f..73b58d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17338,34 +17338,38 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, return ret; }
+typedef enum { + QEMU_BLOCK_IOTUNE_SET_BYTES = 1 << 0, + QEMU_BLOCK_IOTUNE_SET_IOPS = 1 << 1, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, + QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, +} qemuBlockIoTuneSetFlags; +
/* If the user didn't specify bytes limits, inherit previous values; * likewise if the user didn't specify iops limits. */ static void qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, virDomainBlockIoTuneInfoPtr oldinfo, - bool set_bytes, - bool set_iops, - bool set_bytes_max, - bool set_iops_max, - bool set_size_iops, - bool set_bytes_max_length, - bool set_iops_max_length) + qemuBlockIoTuneSetFlags set_flag)
Just a cosmetic "nit", I spent a few seconds looking at the name "set_flag" confusingly (probably 'cause it's Friday). Maybe something like set_map|set_mask|mask|bitmap or something alike would sound better, but then, who am I to judge with my history of 'brilliant' function naming :D. Patch looks good though, I'll leave it to you. Erik
On 11/25/2016 10:53 AM, Erik Skultety wrote:
On Mon, Nov 21, 2016 at 06:35:50PM -0500, John Ferlan wrote:
Rather than have multiple bool values, create a single enum with bits representing what can be set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 59 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87d219f..73b58d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17338,34 +17338,38 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, return ret; }
+typedef enum { + QEMU_BLOCK_IOTUNE_SET_BYTES = 1 << 0, + QEMU_BLOCK_IOTUNE_SET_IOPS = 1 << 1, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, + QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, +} qemuBlockIoTuneSetFlags; +
/* If the user didn't specify bytes limits, inherit previous values; * likewise if the user didn't specify iops limits. */ static void qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, virDomainBlockIoTuneInfoPtr oldinfo, - bool set_bytes, - bool set_iops, - bool set_bytes_max, - bool set_iops_max, - bool set_size_iops, - bool set_bytes_max_length, - bool set_iops_max_length) + qemuBlockIoTuneSetFlags set_flag)
Just a cosmetic "nit", I spent a few seconds looking at the name "set_flag" confusingly (probably 'cause it's Friday). Maybe something like set_map|set_mask|mask|bitmap or something alike would sound better, but then, who am I to judge with my history of 'brilliant' function naming :D.
I could go with set_mask, but mask has a different connotation for some. It's not a bitmap and there are those that would say that a bitmap should use the Bitmap functions (which would be overkill at this point). Maybe a 'set_fields_flag' would be clearer? It's a flag to indicate a grouping of (usually) 3 elements ({read|write|total}) were set, where size_iops and group_name become the exception. Unless someone else has a brilliant suggestion... John
Patch looks good though, I'll leave it to you.
Erik
On Mon, Nov 28, 2016 at 07:08:42AM -0500, John Ferlan wrote:
On 11/25/2016 10:53 AM, Erik Skultety wrote:
On Mon, Nov 21, 2016 at 06:35:50PM -0500, John Ferlan wrote:
Rather than have multiple bool values, create a single enum with bits representing what can be set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 113 +++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 59 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87d219f..73b58d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17338,34 +17338,38 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, return ret; }
+typedef enum { + QEMU_BLOCK_IOTUNE_SET_BYTES = 1 << 0, + QEMU_BLOCK_IOTUNE_SET_IOPS = 1 << 1, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, + QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, +} qemuBlockIoTuneSetFlags; +
/* If the user didn't specify bytes limits, inherit previous values; * likewise if the user didn't specify iops limits. */ static void qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, virDomainBlockIoTuneInfoPtr oldinfo, - bool set_bytes, - bool set_iops, - bool set_bytes_max, - bool set_iops_max, - bool set_size_iops, - bool set_bytes_max_length, - bool set_iops_max_length) + qemuBlockIoTuneSetFlags set_flag)
Just a cosmetic "nit", I spent a few seconds looking at the name "set_flag" confusingly (probably 'cause it's Friday). Maybe something like set_map|set_mask|mask|bitmap or something alike would sound better, but then, who am I to judge with my history of 'brilliant' function naming :D.
I could go with set_mask, but mask has a different connotation for some. It's not a bitmap and there are those that would say that a bitmap should use the Bitmap functions (which would be overkill at this point).
Maybe a 'set_fields_flag' would be clearer? It's a flag to indicate a
the typedef does already have 'flag' in it...how about plain set_fields then? Erik
grouping of (usually) 3 elements ({read|write|total}) were set, where size_iops and group_name become the exception.
Unless someone else has a brilliant suggestion...
John
Patch looks good though, I'll leave it to you.
Erik
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 | 45 +++++++++++++++++++- src/qemu/qemu_monitor.c | 2 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 14 ++++++- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 88 ++++++++++++++++++++++++++++++++-------- 9 files changed, 140 insertions(+), 21 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 6e008e2..88bd098 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1649,6 +1649,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 541b600..152a6a8 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 73b58d0..7f17975 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -115,8 +115,10 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 #define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 #define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS 1 #define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS) #define QEMU_NB_NUMA_PARAM 2 @@ -17344,8 +17346,9 @@ typedef enum { QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, - QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, - QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_GROUP_NAME = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 7, } qemuBlockIoTuneSetFlags; @@ -17371,6 +17374,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!(set_flag & QEMU_BLOCK_IOTUNE_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 @@ -17425,6 +17430,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virDomainDiskDefPtr disk; qemuBlockIoTuneSetFlags set_flag = 0; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17461,6 +17467,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, @@ -17540,6 +17548,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); SET_IOTUNE_FIELD(size_iops_sec, 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_flag |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; + 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, BYTES_MAX_LENGTH, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17592,6 +17610,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); @@ -17612,6 +17632,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + if (!supportGroupNameOption && + (set_flag & QEMU_BLOCK_IOTUNE_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_flag & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH | QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH))) { @@ -17655,12 +17683,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) goto endjob; disk->blkdeviotune = info; + info.group_name = NULL; ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17683,6 +17713,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_flag); conf_disk->blkdeviotune = info; + info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17693,6 +17724,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: + VIR_FREE(info.group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams) @@ -17754,6 +17786,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; } else { @@ -17821,6 +17855,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 3ff31e4..42c9dd2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3468,6 +3468,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3477,6 +3478,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 100730b..a9da71d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -880,6 +880,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 c2b81b7..aa6ded7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4496,6 +4496,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", @@ -4521,7 +4522,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); @@ -4535,6 +4535,11 @@ 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")) && + 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); @@ -4563,6 +4568,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; @@ -4596,6 +4602,12 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, NULL) < 0) goto cleanup; + if (supportGroupNameOption && + virJSONValueObjectAdd(args, + "s:group", info->group_name, + NULL) < 0) + goto cleanup; + if (supportMaxLengthOptions && virJSONValueObjectAdd(args, "P:bps_max_length", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4780d53..adff0c3 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 ed4190b..c6bcf80 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" " }," @@ -1991,6 +1992,55 @@ testQemuMonitorJSONqemuMonitorJSONGetChardevInfo(const void *data) return ret; } + +static int +testValidateGetBlockIoThrottle(virDomainBlockIoTuneInfo info, + virDomainBlockIoTuneInfo expectedInfo) +{ +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + return -1; \ + } \ + 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); \ + return -1; \ + } \ + 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); \ + return -1; \ + } + 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); + return -1; + } + 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); + return -1; + } +#undef VALIDATE_IOTUNE + + return 0; +} + + static int testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) { @@ -2002,7 +2052,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 +2066,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 +2080,18 @@ 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"); + if (testValidateGetBlockIoThrottle(info, expectedInfo) < 0) goto cleanup; - } 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, Nov 21, 2016 at 06:35:51PM -0500, 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 | 45 +++++++++++++++++++- src/qemu/qemu_monitor.c | 2 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 14 ++++++- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 88 ++++++++++++++++++++++++++++++++-------- 9 files changed, 140 insertions(+), 21 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 6e008e2..88bd098 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1649,6 +1649,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 541b600..152a6a8 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 73b58d0..7f17975 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -115,8 +115,10 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 #define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 #define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS 1 #define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
#define QEMU_NB_NUMA_PARAM 2 @@ -17344,8 +17346,9 @@ typedef enum { QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, - QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, - QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_GROUP_NAME = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 7, } qemuBlockIoTuneSetFlags;
@@ -17371,6 +17374,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo,
if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!(set_flag & QEMU_BLOCK_IOTUNE_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 @@ -17425,6 +17430,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virDomainDiskDefPtr disk; qemuBlockIoTuneSetFlags set_flag = 0; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17461,6 +17467,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, @@ -17540,6 +17548,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); SET_IOTUNE_FIELD(size_iops_sec, 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_flag |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; + 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, BYTES_MAX_LENGTH, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17592,6 +17610,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);
@@ -17612,6 +17632,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
+ if (!supportGroupNameOption && + (set_flag & QEMU_BLOCK_IOTUNE_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_flag & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH | QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH))) { @@ -17655,12 +17683,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) goto endjob; disk->blkdeviotune = info; + info.group_name = NULL;
ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17683,6 +17713,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_flag); conf_disk->blkdeviotune = info; + info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17693,6 +17724,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm);
cleanup: + VIR_FREE(info.group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams) @@ -17754,6 +17786,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; } else { @@ -17821,6 +17855,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 Martin pointed out in v1 review , this could still become BLOCK_IOTUNE_ASSIGN, this way it just sort of bothers the OCD inside me. Would you mind adjusting it? :) Also, this^^ hunk above will later (during serialization) crash the daemon, since reply is uninitialized in this function so that needs to be adjusted as well. Erik
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 3ff31e4..42c9dd2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3468,6 +3468,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3477,6 +3478,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 100730b..a9da71d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -880,6 +880,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 c2b81b7..aa6ded7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4496,6 +4496,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", @@ -4521,7 +4522,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); @@ -4535,6 +4535,11 @@ 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")) && + 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); @@ -4563,6 +4568,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; @@ -4596,6 +4602,12 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, NULL) < 0) goto cleanup;
+ if (supportGroupNameOption && + virJSONValueObjectAdd(args, + "s:group", info->group_name, + NULL) < 0) + goto cleanup; + if (supportMaxLengthOptions && virJSONValueObjectAdd(args, "P:bps_max_length", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4780d53..adff0c3 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 ed4190b..c6bcf80 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" " }," @@ -1991,6 +1992,55 @@ testQemuMonitorJSONqemuMonitorJSONGetChardevInfo(const void *data) return ret; }
+ +static int +testValidateGetBlockIoThrottle(virDomainBlockIoTuneInfo info, + virDomainBlockIoTuneInfo expectedInfo) +{ +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + return -1; \ + } \ + 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); \ + return -1; \ + } \ + 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); \ + return -1; \ + } + 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); + return -1; + } + 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); + return -1; + } +#undef VALIDATE_IOTUNE + + return 0; +} + + static int testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) { @@ -2002,7 +2052,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 +2066,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 +2080,18 @@ 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"); + if (testValidateGetBlockIoThrottle(info, expectedInfo) < 0) goto cleanup; - }
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
[...]
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 Martin pointed out in v1 review , this could still become BLOCK_IOTUNE_ASSIGN, this way it just sort of bothers the OCD inside me. Would you mind adjusting it? :)
I get the OCD thing; however, as I pointed out in my response to Martin the macro uses VIR_TYPED_PARAM_ULLONG while this particular field is a VIR_TYPED_PARAM_STRING
Also, this^^ hunk above will later (during serialization) crash the daemon, since reply is uninitialized in this function so that needs to be adjusted as well.
I'm not getting a libvirtd crash... Do you have a trace? John [...]
On Mon, Nov 28, 2016 at 11:28:10AM -0500, John Ferlan wrote:
[...]
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 Martin pointed out in v1 review , this could still become BLOCK_IOTUNE_ASSIGN, this way it just sort of bothers the OCD inside me. Would you mind adjusting it? :)
I get the OCD thing; however, as I pointed out in my response to Martin the macro uses VIR_TYPED_PARAM_ULLONG while this particular field is a VIR_TYPED_PARAM_STRING
Also, this^^ hunk above will later (during serialization) crash the daemon, since reply is uninitialized in this function so that needs to be adjusted as well.
I'm not getting a libvirtd crash... Do you have a trace?
#0 0x00007ffff35b1a28 in raise () from /lib64/libc.so.6 #1 0x00007ffff35b362a in abort () from /lib64/libc.so.6 #2 0x00007ffff35f4dea in __libc_message () from /lib64/libc.so.6 #3 0x00007ffff35fd22a in _int_free () from /lib64/libc.so.6 #4 0x00007ffff360078c in free () from /lib64/libc.so.6 #5 0x00007ffff71fafd5 in virFree (ptrptr=0x7fffd0306de8) at util/viralloc.c:582 #6 0x00007ffff7287f40 in virTypedParamsClear (params=0x7fffd03068b0, nparams=20) at util/virtypedparam.c:1298 #7 0x00007ffff7287f75 in virTypedParamsFree (params=0x7fffd03068b0, nparams=20) at util/virtypedparam.c:1317 #8 0x000055555559b01d in remoteDispatchDomainGetBlockIoTune (server=0x555555814300, client=0x55555584cad0, hdr=0x55555584d5e0, rerr=0x7fffe41d8ac0, args=0x7fffd000a840, ret=0x7fffd0000bf0) at remote.c:3071 #9 0x0000555555579c32 in remoteDispatchDomainGetBlockIoTuneHelper (server=0x555555814300, client=0x55555584cad0, msg=0x55555584d5e0, rerr=0x7fffe41d8ac0, args=0x7fffd000a840, ret=0x7fffd0000bf0) at remote_dispatch.h:5091 #10 0x00007ffff73ed63b in virNetServerProgramDispatchCall (prog=0x55555580b7a0, server=0x555555814300, client=0x55555584cad0, msg=0x55555584d5e0) at rpc/virnetserverprogram.c:437 #11 0x00007ffff73ed1a1 in virNetServerProgramDispatch (prog=0x55555580b7a0, server=0x555555814300, client=0x55555584cad0, msg=0x55555584d5e0) at rpc/virnetserverprogram.c:307 #12 0x00005555555b8e66 in virNetServerProcessMsg (srv=0x555555814300, client=0x55555584cad0, prog=0x55555580b7a0, msg=0x55555584d5e0) at rpc/virnetserver.c:148 #13 0x00005555555b8f2a in virNetServerHandleJob (jobOpaque=0x55555584b850, opaque=0x555555814300) at rpc/virnetserver.c:169 #14 0x00007ffff72832c4 in virThreadPoolWorker (opaque=0x5555558204c0) at util/virthreadpool.c:167 #15 0x00007ffff7282853 in virThreadHelper (data=0x555555822060) at util/virthread.c:206 #16 0x00007ffff394561a in start_thread () from /lib64/libpthread.so.0 #17 0x00007ffff367f5fd in clone () from /lib64/libc.so.6
John
[...]
On Mon, Nov 21, 2016 at 06:35:51PM -0500, 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 | 45 +++++++++++++++++++- src/qemu/qemu_monitor.c | 2 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 14 ++++++- src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 88 ++++++++++++++++++++++++++++++++-------- 9 files changed, 140 insertions(+), 21 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 6e008e2..88bd098 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1649,6 +1649,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 541b600..152a6a8 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 73b58d0..7f17975 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -115,8 +115,10 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS 6 #define QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS 7 #define QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS 6 +#define QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS 1 #define QEMU_NB_BLOCK_IO_TUNE_ALL_PARAMS (QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS + \ + QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS + \ QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS)
#define QEMU_NB_NUMA_PARAM 2 @@ -17344,8 +17346,9 @@ typedef enum { QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, - QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 5, - QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_GROUP_NAME = 1 << 5, + QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 6, + QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 7, } qemuBlockIoTuneSetFlags;
@@ -17371,6 +17374,8 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo,
if (!(set_flag & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; + if (!(set_flag & QEMU_BLOCK_IOTUNE_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 @@ -17425,6 +17430,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virDomainDiskDefPtr disk; qemuBlockIoTuneSetFlags set_flag = 0; bool supportMaxOptions = true; + bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; virQEMUDriverConfigPtr cfg = NULL; virObjectEventPtr event = NULL; @@ -17461,6 +17467,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, @@ -17540,6 +17548,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); SET_IOTUNE_FIELD(size_iops_sec, 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_flag |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; + 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, BYTES_MAX_LENGTH, TOTAL_BYTES_SEC_MAX_LENGTH); @@ -17592,6 +17610,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);
@@ -17612,6 +17632,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
+ if (!supportGroupNameOption && + (set_flag & QEMU_BLOCK_IOTUNE_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_flag & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH | QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH))) { @@ -17655,12 +17683,14 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, supportMaxOptions, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) goto endjob; disk->blkdeviotune = info; + info.group_name = NULL;
ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17683,6 +17713,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, set_flag); conf_disk->blkdeviotune = info; + info.group_name = NULL; ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef); if (ret < 0) goto endjob; @@ -17693,6 +17724,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEndJob(driver, vm);
cleanup: + VIR_FREE(info.group_name); VIR_FREE(device); virDomainObjEndAPI(&vm); if (eventNparams) @@ -17754,6 +17786,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, maxparams = QEMU_NB_BLOCK_IO_TUNE_BASE_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) maxparams += QEMU_NB_BLOCK_IO_TUNE_MAX_PARAMS; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) + maxparams += QEMU_NB_BLOCK_IO_TUNE_GROUP_PARAMS; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) maxparams += QEMU_NB_BLOCK_IO_TUNE_LENGTH_PARAMS; } else { @@ -17821,6 +17855,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 3ff31e4..42c9dd2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3468,6 +3468,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { VIR_DEBUG("device=%p, info=%p", device, info); @@ -3477,6 +3478,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 100730b..a9da71d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -880,6 +880,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 c2b81b7..aa6ded7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4496,6 +4496,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", @@ -4521,7 +4522,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); @@ -4535,6 +4535,11 @@ 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")) && + VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; +
One more thing, you can make the GetString call directly from VIR_STRDUP and then get rid of the group_name variable. Erik
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); @@ -4563,6 +4568,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, virDomainBlockIoTuneInfoPtr info, bool supportMaxOptions, + bool supportGroupNameOption, bool supportMaxLengthOptions) { int ret = -1; @@ -4596,6 +4602,12 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, NULL) < 0) goto cleanup;
+ if (supportGroupNameOption && + virJSONValueObjectAdd(args, + "s:group", info->group_name, + NULL) < 0) + goto cleanup; + if (supportMaxLengthOptions && virJSONValueObjectAdd(args, "P:bps_max_length", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4780d53..adff0c3 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 ed4190b..c6bcf80 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" " }," @@ -1991,6 +1992,55 @@ testQemuMonitorJSONqemuMonitorJSONGetChardevInfo(const void *data) return ret; }
+ +static int +testValidateGetBlockIoThrottle(virDomainBlockIoTuneInfo info, + virDomainBlockIoTuneInfo expectedInfo) +{ +#define VALIDATE_IOTUNE(field) \ + if (info.field != expectedInfo.field) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + "info.%s=%llu != expected=%llu", \ + #field, info.field, expectedInfo.field); \ + return -1; \ + } \ + 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); \ + return -1; \ + } \ + 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); \ + return -1; \ + } + 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); + return -1; + } + 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); + return -1; + } +#undef VALIDATE_IOTUNE + + return 0; +} + + static int testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) { @@ -2002,7 +2052,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 +2066,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 +2080,18 @@ 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"); + if (testValidateGetBlockIoThrottle(info, expectedInfo) < 0) goto cleanup; - }
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
[...]
@@ -4535,6 +4535,11 @@ 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")) && + VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; +
One more thing, you can make the GetString call directly from VIR_STRDUP and then get rid of the group_name variable.
Erik
Ewww.... too overloaded for my taste... I'm sure behind the scenes the compiler will optimize anyway. FWIW: Doing so results in compiler errors: if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group") < 0) : qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:67: error: ordered comparison of pointer with integer zero [-Werror=extra] virJSONValueObjectGetString(inserted, "group") < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~ qemu/qemu_monitor_json.c:4539:20: error: passing argument 2 of 'virStrdup' makes pointer from integer without a cast [-Werror=int-conversion] OR: if (VIR_STRDUP(reply->group_name, (virJSONValueObjectGetString(inserted, "group")) < 0) In file included from qemu/qemu_monitor_json.c:46:0: qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:73: error: ordered comparison of pointer with integer zero [-Werror=extra] (virJSONValueObjectGetString(inserted, "group")) < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~ John
On Mon, Nov 28, 2016 at 05:14:54PM -0500, John Ferlan wrote:
[...]
@@ -4535,6 +4535,11 @@ 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")) && + VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; +
One more thing, you can make the GetString call directly from VIR_STRDUP and then get rid of the group_name variable.
Erik
Ewww.... too overloaded for my taste... I'm sure behind the scenes the compiler will optimize anyway.
FWIW: Doing so results in compiler errors:
if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group") < 0) :
qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:67: error: ordered comparison of pointer with integer zero [-Werror=extra] virJSONValueObjectGetString(inserted, "group") < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~ qemu/qemu_monitor_json.c:4539:20: error: passing argument 2 of 'virStrdup' makes pointer from integer without a cast [-Werror=int-conversion]
OR:
if (VIR_STRDUP(reply->group_name, (virJSONValueObjectGetString(inserted, "group")) < 0)
In file included from qemu/qemu_monitor_json.c:46:0: qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:73: error: ordered comparison of pointer with integer zero [-Werror=extra] (virJSONValueObjectGetString(inserted, "group")) < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~
You've got the parentheses wrong...in both cases... Also, once you do what I'm suggesting, the crash from 6/9 will be resolved, however I still would like to see the @reply structure in qemuDomainGetBlockIoTune initialized properly on the stack. Erik
John
On 11/29/2016 03:50 AM, Erik Skultety wrote:
On Mon, Nov 28, 2016 at 05:14:54PM -0500, John Ferlan wrote:
[...]
@@ -4535,6 +4535,11 @@ 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")) && + VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; +
One more thing, you can make the GetString call directly from VIR_STRDUP and then get rid of the group_name variable.
Erik
Ewww.... too overloaded for my taste... I'm sure behind the scenes the compiler will optimize anyway.
FWIW: Doing so results in compiler errors:
if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group") < 0) :
qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:67: error: ordered comparison of pointer with integer zero [-Werror=extra] virJSONValueObjectGetString(inserted, "group") < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~ qemu/qemu_monitor_json.c:4539:20: error: passing argument 2 of 'virStrdup' makes pointer from integer without a cast [-Werror=int-conversion]
OR:
if (VIR_STRDUP(reply->group_name, (virJSONValueObjectGetString(inserted, "group")) < 0)
In file included from qemu/qemu_monitor_json.c:46:0: qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:73: error: ordered comparison of pointer with integer zero [-Werror=extra] (virJSONValueObjectGetString(inserted, "group")) < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~
You've got the parentheses wrong...in both cases...
ah, right - my eyes just weren't seeing it, I changed to the following. if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group")) < 0) goto cleanup;
Also, once you do what I'm suggesting, the crash from 6/9 will be resolved, however I still would like to see the @reply structure in qemuDomainGetBlockIoTune initialized properly on the stack.
Weird how that crash happens for you, but not me especially since you show 20 nparams which would seem to imply you're getting something back from qemu. Even though it's not set by libvirt, qemu does default to something (the id/alias for the object - in my case "drive-virtio-disk0" for my qemu 2.6.2). What do you get from a command: virsh qemu-monitor-command $domain '{"execute":"query-block"}' ? In any case, I'll add a memset(&reply, 0, sizeof(reply)) in the qemu path from the caller, which I believe should suffice the review comment. The other path is covered with the copy from persistent XML. Still since we're in freeze, I'll wait until after the release in order to push. That'll mean an adjustment to the version in formatdomain from 2.5.0 to 3.0.0 since the "next" release won't be until January (http://libvirt.org/downloads.html#schedule) Thanks for the review - John
On Tue, Nov 29, 2016 at 07:28:51AM -0500, John Ferlan wrote:
On 11/29/2016 03:50 AM, Erik Skultety wrote:
On Mon, Nov 28, 2016 at 05:14:54PM -0500, John Ferlan wrote:
[...]
@@ -4535,6 +4535,11 @@ 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")) && + VIR_STRDUP(reply->group_name, group_name) < 0) + goto cleanup; +
One more thing, you can make the GetString call directly from VIR_STRDUP and then get rid of the group_name variable.
Erik
Ewww.... too overloaded for my taste... I'm sure behind the scenes the compiler will optimize anyway.
FWIW: Doing so results in compiler errors:
if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group") < 0) :
qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:67: error: ordered comparison of pointer with integer zero [-Werror=extra] virJSONValueObjectGetString(inserted, "group") < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~ qemu/qemu_monitor_json.c:4539:20: error: passing argument 2 of 'virStrdup' makes pointer from integer without a cast [-Werror=int-conversion]
OR:
if (VIR_STRDUP(reply->group_name, (virJSONValueObjectGetString(inserted, "group")) < 0)
In file included from qemu/qemu_monitor_json.c:46:0: qemu/qemu_monitor_json.c: In function 'qemuMonitorJSONBlockIoThrottleInfo': qemu/qemu_monitor_json.c:4539:73: error: ordered comparison of pointer with integer zero [-Werror=extra] (virJSONValueObjectGetString(inserted, "group")) < 0) ^ ./util/virstring.h:152:49: note: in definition of macro 'VIR_STRDUP' # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ ^~~
You've got the parentheses wrong...in both cases...
ah, right - my eyes just weren't seeing it, I changed to the following.
if (VIR_STRDUP(reply->group_name, virJSONValueObjectGetString(inserted, "group")) < 0) goto cleanup;
Also, once you do what I'm suggesting, the crash from 6/9 will be resolved, however I still would like to see the @reply structure in qemuDomainGetBlockIoTune initialized properly on the stack.
Weird how that crash happens for you, but not me especially since you show 20 nparams which would seem to imply you're getting something back from qemu. Even though it's not set by libvirt, qemu does default to something (the id/alias for the object - in my case "drive-virtio-disk0" for my qemu 2.6.2). What do you get from a command:
virsh qemu-monitor-command $domain '{"execute":"query-block"}'
?
The returned JSON doesn't indicate any default on my qemu 2.6.0: {"return":[{"io-status":"ok","device":"drive-virtio-disk0","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":21474836480,"filename":"/var/lib/libvirt/images/f23-a.qcow2","cluster-size":65536,"format":"qcow2","actual-size":8880848896,"format-specific":{"type":"qcow2","data":{"compat":"1.1","lazy-refcounts":false,"refcount-bits":16,"corrupt":false}},"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block190","backing_file_depth":0,"drv":"qcow2","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/f23-a.qcow2","encryption_key_missing":false},"type":"unknown"},{"io-status":"ok","device":"drive-virtio-disk1","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":41943040000,"filename":"/var/lib/libvirt/images/dummy.img","format":"raw","actual-size":41943048192,"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block398","backing_file_depth":0,"drv":"raw","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/dummy.img","encryption_key_missing":false},"type":"unknown"},{"io-status":"ok","device":"drive-virtio-disk2","locked":false,"removable":false,"inserted":{"iops_rd":0,"detect_zeroes":"off","image":{"virtual-size":52428800,"filename":"/var/lib/libvirt/images/vol-qcow2.img","cluster-size":65536,"format":"qcow2","actual-size":274432,"format-specific":{"type":"qcow2","data":{"compat":"1.1","lazy-refcounts":false,"refcount-bits":16,"corrupt":false}},"dirty-flag":false},"iops_wr":0,"ro":false,"node-name":"#block549","backing_file_depth":0,"drv":"qcow2","iops":0,"bps_wr":0,"write_threshold":0,"encrypted":false,"bps":0,"bps_rd":0,"cache":{"no-flush":false,"direct":false,"writeback":true},"file":"/var/lib/libvirt/images/vol-qcow2.img","encryption_key_missing":false},"type":"unknown"}],"id":"libvirt-9"} What I did was just to start a domain that had no block iotuning turned on whatsoever and when I saw the code I was like, wait, I need to try that as it looked suspicious at first glance (since at that moment I'd already known about the missing initialization) so I tried it and it crashed.
In any case, I'll add a memset(&reply, 0, sizeof(reply)) in the qemu
Yeah, that would do, sure, but is there a problem with a plain static initialization at the function entry point? I mean that's where we (most of the time) put all of our initializations and also where everyone would actually expect such an assignment to take place... Erik
path from the caller, which I believe should suffice the review comment. The other path is covered with the copy from persistent XML.
Still since we're in freeze, I'll wait until after the release in order to push. That'll mean an adjustment to the version in formatdomain from 2.5.0 to 3.0.0 since the "next" release won't be until January (http://libvirt.org/downloads.html#schedule)
Thanks for the review -
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...]
Also, once you do what I'm suggesting, the crash from 6/9 will be resolved, however I still would like to see the @reply structure in qemuDomainGetBlockIoTune initialized properly on the stack.
Weird how that crash happens for you, but not me especially since you show 20 nparams which would seem to imply you're getting something back from qemu. Even though it's not set by libvirt, qemu does default to something (the id/alias for the object - in my case "drive-virtio-disk0" for my qemu 2.6.2). What do you get from a command:
virsh qemu-monitor-command $domain '{"execute":"query-block"}'
?
The returned JSON doesn't indicate any default on my qemu 2.6.0:
[...] Ahhh... Right a domain without iotune adjustments... I think I checked that at one time, but that was before I changed my mind in the implementation from having "group" be some number to allowing a string for group. With the number implementation I was just prepending some constant string. So I guess I had it in my mind that I had already tested that non iotune path.
What I did was just to start a domain that had no block iotuning turned on whatsoever and when I saw the code I was like, wait, I need to try that as it looked suspicious at first glance (since at that moment I'd already known about the missing initialization) so I tried it and it crashed.
In any case, I'll add a memset(&reply, 0, sizeof(reply)) in the qemu
Yeah, that would do, sure, but is there a problem with a plain static initialization at the function entry point? I mean that's where we (most of the time) put all of our initializations and also where everyone would actually expect such an assignment to take place...
Erik
OK, I'll just change the definition: virDomainBlockIoTuneInfo reply = {0}; 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 4e40aa1..227e6f8 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 19d45fd..c44b414 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4974,6 +4974,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 88bd098..714582b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7217,6 +7217,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 && @@ -20426,6 +20429,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 || @@ -20454,6 +20458,11 @@ virDomainDiskDefFormat(virBufferPtr buf, def->blkdeviotune.size_iops_sec); } + if (def->blkdeviotune.group_name) { + virBufferEscapeString(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 e22b63f..e338ad0 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -611,6 +611,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
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 4a5fce3..971e271 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1790,6 +1790,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 || @@ -1844,6 +1853,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) { + virBufferEscapeString(&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 985f45d..7ce0b4e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1540,6 +1540,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
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 a133c81..4be0b95 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 = vshCommandOptStringReq(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, Nov 21, 2016 at 06:35:45PM -0500, John Ferlan wrote:
This is just a REPOST of the v2 series:
http://www.redhat.com/archives/libvir-list/2016-November/msg00363.html
The only difference being updating to the current top of tree of commit id '0b4c3bd30'.
I did *not* add the NEWS change yet as that's newer than this, but will update NEWS with this once/if this is ACK'd using whatever format becomes agreed upon for the file contents/formatting options.
ACK series, provided that you fix the daemon crash properly and address the minor nitpicks I had. Other than that, all the notes raised during v1 have been resolved, so no problem there. Erik
On 11/28/2016 09:54 AM, Erik Skultety wrote:
On Mon, Nov 21, 2016 at 06:35:45PM -0500, John Ferlan wrote:
This is just a REPOST of the v2 series:
http://www.redhat.com/archives/libvir-list/2016-November/msg00363.html
The only difference being updating to the current top of tree of commit id '0b4c3bd30'.
I did *not* add the NEWS change yet as that's newer than this, but will update NEWS with this once/if this is ACK'd using whatever format becomes agreed upon for the file contents/formatting options.
ACK series, provided that you fix the daemon crash properly and address the minor nitpicks I had. Other than that, all the notes raised during v1 have been resolved, so no problem there.
Erik
This is now pushed for 3.0.0, news entry as added as well Tks - John
participants (2)
-
Erik Skultety -
John Ferlan