And introduce virDomainBlockIoTuneInfoHasAny. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 12 ++++++++++ src/libvirt_private.syms | 4 ++++ src/qemu/qemu_command.c | 49 ++++------------------------------------ 4 files changed, 66 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index afa072e17d..abe21a93fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31621,3 +31621,49 @@ virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics) return true; } + + +bool +virDomainBlockIoTuneInfoHasBasic(const virDomainBlockIoTuneInfo *iotune) +{ + return iotune->total_bytes_sec || + iotune->read_bytes_sec || + iotune->write_bytes_sec || + iotune->total_iops_sec || + iotune->read_iops_sec || + iotune->write_iops_sec; +} + + +bool +virDomainBlockIoTuneInfoHasMax(const virDomainBlockIoTuneInfo *iotune) +{ + return iotune->total_bytes_sec_max || + iotune->read_bytes_sec_max || + iotune->write_bytes_sec_max || + iotune->total_iops_sec_max || + iotune->read_iops_sec_max || + iotune->write_iops_sec_max || + iotune->size_iops_sec; +} + + +bool +virDomainBlockIoTuneInfoHasMaxLength(const virDomainBlockIoTuneInfo *iotune) +{ + return iotune->total_bytes_sec_max_length || + iotune->read_bytes_sec_max_length || + iotune->write_bytes_sec_max_length || + iotune->total_iops_sec_max_length || + iotune->read_iops_sec_max_length || + iotune->write_iops_sec_max_length; +} + + +bool +virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune) +{ + return virDomainBlockIoTuneInfoHasBasic(iotune) || + virDomainBlockIoTuneInfoHasMax(iotune) || + virDomainBlockIoTuneInfoHasMaxLength(iotune); +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e012975fca..8b373184ca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3684,3 +3684,15 @@ virDomainGraphicsGetRenderNode(const virDomainGraphicsDef *graphics); bool virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics); + +bool +virDomainBlockIoTuneInfoHasBasic(const virDomainBlockIoTuneInfo *iotune); + +bool +virDomainBlockIoTuneInfoHasMax(const virDomainBlockIoTuneInfo *iotune); + +bool +virDomainBlockIoTuneInfoHasMaxLength(const virDomainBlockIoTuneInfo *iotune); + +bool +virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7b1ef23bc..a8dc2c30ea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,10 @@ virDomainActualNetDefFree; virDomainActualNetDefValidate; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; +virDomainBlockIoTuneInfoHasAny; +virDomainBlockIoTuneInfoHasBasic; +virDomainBlockIoTuneInfoHasMax; +virDomainBlockIoTuneInfoHasMaxLength; virDomainBootTypeFromString; virDomainBootTypeToString; virDomainCapabilitiesPolicyTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a8137b3a32..29928ac1c8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1138,50 +1138,11 @@ qemuGetDriveSourceString(virStorageSourcePtr src, } -static bool -qemuDiskConfigBlkdeviotuneHasBasic(virDomainDiskDefPtr disk) -{ - return disk->blkdeviotune.total_bytes_sec || - disk->blkdeviotune.read_bytes_sec || - disk->blkdeviotune.write_bytes_sec || - disk->blkdeviotune.total_iops_sec || - disk->blkdeviotune.read_iops_sec || - disk->blkdeviotune.write_iops_sec; -} - - -static bool -qemuDiskConfigBlkdeviotuneHasMax(virDomainDiskDefPtr disk) -{ - return disk->blkdeviotune.total_bytes_sec_max || - disk->blkdeviotune.read_bytes_sec_max || - disk->blkdeviotune.write_bytes_sec_max || - disk->blkdeviotune.total_iops_sec_max || - disk->blkdeviotune.read_iops_sec_max || - disk->blkdeviotune.write_iops_sec_max || - disk->blkdeviotune.size_iops_sec; -} - - -static bool -qemuDiskConfigBlkdeviotuneHasMaxLength(virDomainDiskDefPtr disk) -{ - return disk->blkdeviotune.total_bytes_sec_max_length || - disk->blkdeviotune.read_bytes_sec_max_length || - disk->blkdeviotune.write_bytes_sec_max_length || - disk->blkdeviotune.total_iops_sec_max_length || - disk->blkdeviotune.read_iops_sec_max_length || - disk->blkdeviotune.write_iops_sec_max_length; -} - - bool qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk) { return !!disk->blkdeviotune.group_name || - qemuDiskConfigBlkdeviotuneHasBasic(disk) || - qemuDiskConfigBlkdeviotuneHasMax(disk) || - qemuDiskConfigBlkdeviotuneHasMaxLength(disk); + virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune); } @@ -1199,9 +1160,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, { /* group_name by itself is ignored by qemu */ if (disk->blkdeviotune.group_name && - !qemuDiskConfigBlkdeviotuneHasBasic(disk) && - !qemuDiskConfigBlkdeviotuneHasMax(disk) && - !qemuDiskConfigBlkdeviotuneHasMaxLength(disk)) { + !virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("group_name can be configured only together with " "settings")); @@ -1229,7 +1188,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, if (qemuCaps) { /* block I/O throttling 1.7 */ - if (qemuDiskConfigBlkdeviotuneHasMax(disk) && + if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("there are some block I/O throttling parameters " @@ -1247,7 +1206,7 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, } /* block I/O throttling length 2.6 */ - if (qemuDiskConfigBlkdeviotuneHasMaxLength(disk) && + if (virDomainBlockIoTuneInfoHasMaxLength(&disk->blkdeviotune) && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("there are some block I/O throttling length parameters " -- 2.23.0

Currenly if only iotune group name is given for some disk and no any params then later start of domain will fail. I guess it will be convinient to allow such configuration if there is another disk in the same iotune group with iotune params set. The meaning is that the first disk have same iotunes and the latter. Thus one can easyly add disk to iotune group - just add group name parameter and no need to copy all the params. Also let's expand iotunes params in the described case so we don't need to refer to another disk to know iotunes and this will make logic in many places simple. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index abe21a93fd..f75841c60f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5106,6 +5106,33 @@ virDomainRNGDefPostParse(virDomainRNGDefPtr rng) } +static void +virDomainDiskExpandGroupIoTune(virDomainDiskDefPtr disk, + const virDomainDef *def) +{ + size_t i; + + if (!disk->blkdeviotune.group_name || + virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune)) + return; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr d = def->disks[i]; + + if (d->blkdeviotune.group_name && + STREQ(disk->blkdeviotune.group_name, d->blkdeviotune.group_name) && + virDomainBlockIoTuneInfoHasAny(&d->blkdeviotune)) { + char *tmp = disk->blkdeviotune.group_name; + + disk->blkdeviotune = d->blkdeviotune; + disk->blkdeviotune.group_name = tmp; + + return; + } + } +} + + static int virDomainDiskDefPostParse(virDomainDiskDefPtr disk, const virDomainDef *def, @@ -5151,6 +5178,8 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, return -1; } + virDomainDiskExpandGroupIoTune(disk, def); + return 0; } -- 2.23.0

Currently it is possible to start a domain which have disks in same iotune group and at the same time having different iotune params. Both params set are passed to qemu in command line and the one that is passed later down command line is get actually set. Let's prohibit such configurations. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 29 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 41 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 2 +- 8 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f75841c60f..45002782fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31696,3 +31696,32 @@ virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune) virDomainBlockIoTuneInfoHasMax(iotune) || virDomainBlockIoTuneInfoHasMaxLength(iotune); } + + +bool +virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, + const virDomainBlockIoTuneInfo *b) +{ + if (a->total_bytes_sec == b->total_bytes_sec && + a->read_bytes_sec == b->read_bytes_sec && + a->write_bytes_sec == b->write_bytes_sec && + a->total_iops_sec == b->total_iops_sec && + a->read_iops_sec == b->read_iops_sec && + a->write_iops_sec == b->write_iops_sec && + a->total_bytes_sec_max == b->total_bytes_sec_max && + a->read_bytes_sec_max == b->read_bytes_sec_max && + a->write_bytes_sec_max == b->write_bytes_sec_max && + a->total_iops_sec_max == b->total_iops_sec_max && + a->read_iops_sec_max == b->read_iops_sec_max && + a->write_iops_sec_max == b->write_iops_sec_max && + a->size_iops_sec == b->size_iops_sec && + a->total_bytes_sec_max_length == b->total_bytes_sec_max_length && + a->read_bytes_sec_max_length == b->read_bytes_sec_max_length && + a->write_bytes_sec_max_length == b->write_bytes_sec_max_length && + a->total_iops_sec_max_length == b->total_iops_sec_max_length && + a->read_iops_sec_max_length == b->read_iops_sec_max_length && + a->write_iops_sec_max_length == b->write_iops_sec_max_length) + return true; + + return false; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b373184ca..33d7ab97bd 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3696,3 +3696,7 @@ virDomainBlockIoTuneInfoHasMaxLength(const virDomainBlockIoTuneInfo *iotune); bool virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune); + +bool +virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, + const virDomainBlockIoTuneInfo *b); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a8dc2c30ea..fddf715220 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,7 @@ virDomainActualNetDefFree; virDomainActualNetDefValidate; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; +virDomainBlockIoTuneInfoEqual; virDomainBlockIoTuneInfoHasAny; virDomainBlockIoTuneInfoHasBasic; virDomainBlockIoTuneInfoHasMax; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 29928ac1c8..94a49b7e4f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1156,6 +1156,7 @@ qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk) */ static int qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { /* group_name by itself is ignored by qemu */ @@ -1167,6 +1168,27 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, return -1; } + /* checking def here is only for calling from tests */ + if (disk->blkdeviotune.group_name && def) { + size_t i; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr d = def->disks[i]; + + if (!d->blkdeviotune.group_name || + STRNEQ(disk->blkdeviotune.group_name, d->blkdeviotune.group_name)) + continue; + + if (!virDomainBlockIoTuneInfoEqual(&disk->blkdeviotune, + &d->blkdeviotune)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("different iotunes for disks %s and %s"), + disk->dst, d->dst); + return -1; + } + } + } + if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX || @@ -1229,9 +1251,10 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, */ int qemuCheckDiskConfig(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { - if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0) + if (qemuCheckDiskConfigBlkdeviotune(disk, def, qemuCaps) < 0) return -1; if (disk->wwn) { @@ -1729,6 +1752,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk, static char * qemuBuildDriveStr(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER; @@ -1755,7 +1779,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, } /* if we are using -device this will be checked elsewhere */ - if (qemuCheckDiskConfig(disk, qemuCaps) < 0) + if (qemuCheckDiskConfig(disk, def, qemuCaps) < 0) goto error; virBufferAsprintf(&opt, "if=%s", @@ -1900,7 +1924,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, g_autofree char *scsiVPDDeviceId = NULL; int controllerModel; - if (qemuCheckDiskConfig(disk, qemuCaps) < 0) + if (qemuCheckDiskConfig(disk, def, qemuCaps) < 0) goto error; if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) @@ -2408,6 +2432,7 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommandPtr cmd, static int qemuBuildDiskSourceCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceChainData) data = NULL; @@ -2427,7 +2452,7 @@ qemuBuildDiskSourceCommandLine(virCommandPtr cmd, !(copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk))) return -1; } else { - if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, qemuCaps))) + if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, def, qemuCaps))) return -1; } @@ -2457,7 +2482,7 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, { g_autofree char *optstr = NULL; - if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0) + if (qemuBuildDiskSourceCommandLine(cmd, disk, def, qemuCaps) < 0) return -1; if (!qemuDiskBusNeedsDriveArg(disk->bus)) { @@ -10138,6 +10163,7 @@ qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) */ qemuBlockStorageSourceAttachDataPtr qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; @@ -10145,7 +10171,7 @@ qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk, if (VIR_ALLOC(data) < 0) return NULL; - if (!(data->driveCmd = qemuBuildDriveStr(disk, qemuCaps)) || + if (!(data->driveCmd = qemuBuildDriveStr(disk, def, qemuCaps)) || !(data->driveAlias = qemuAliasDiskDriveFromDisk(disk))) return NULL; @@ -10203,6 +10229,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src, */ qemuBlockStorageSourceChainDataPtr qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceAttachData) elem = NULL; @@ -10211,7 +10238,7 @@ qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, if (VIR_ALLOC(data) < 0) return NULL; - if (!(elem = qemuBuildStorageSourceAttachPrepareDrive(disk, qemuCaps))) + if (!(elem = qemuBuildStorageSourceAttachPrepareDrive(disk, def, qemuCaps))) return NULL; if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, elem, qemuCaps) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 786991fd3d..d7fdba9942 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -105,6 +105,7 @@ bool qemuDiskBusNeedsDriveArg(int bus); qemuBlockStorageSourceAttachDataPtr qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps); int qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src, @@ -114,6 +115,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src, qemuBlockStorageSourceChainDataPtr qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps); @@ -199,6 +201,7 @@ bool qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk); int qemuCheckDiskConfig(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps); bool diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ec8faf384c..c41179edb1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8126,7 +8126,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, } if (virDomainDiskTranslateSourcePool(disk) < 0) return -1; - if (qemuCheckDiskConfig(disk, NULL) < 0) + if (qemuCheckDiskConfig(disk, vmdef, NULL) < 0) return -1; if (qemuCheckDiskConfigAgainstDomain(vmdef, disk) < 0) return -1; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f2a9386433..6a95fd49ac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -700,7 +700,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, priv->qemuCaps))) goto cleanup; } else { - if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, + if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, vm->def, priv->qemuCaps))) goto cleanup; } diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 3e9edb85f0..670e5bfab1 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -204,7 +204,7 @@ testQemuDiskXMLToProps(const void *opaque) VIR_DOMAIN_DEF_PARSE_STATUS))) goto cleanup; - if (qemuCheckDiskConfig(disk, data->qemuCaps) < 0 || + if (qemuCheckDiskConfig(disk, NULL, data->qemuCaps) < 0 || qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) { VIR_TEST_VERBOSE("invalid configuration for disk"); goto cleanup; -- 2.23.0

virDomainSetBlockIoTune not simply sets the iotune params given in API but use current settings for all the omitted params. Unfortunately it uses current settings for active config when setting inactive params. Let's fix it. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c41179edb1..f1ee25aebd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19115,6 +19115,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virDomainDefPtr def = NULL; virDomainDefPtr persistentDef = NULL; virDomainBlockIoTuneInfo info; + virDomainBlockIoTuneInfo conf_info; g_autofree char *drivealias = NULL; const char *qdevid = NULL; int ret = -1; @@ -19178,6 +19179,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, return -1; memset(&info, 0, sizeof(info)); + memset(&conf_info, 0, sizeof(conf_info)); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -19302,6 +19304,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + conf_info = info; + conf_info.group_name = g_strdup(info.group_name); + if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); @@ -19422,11 +19427,11 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (qemuDomainSetBlockIoTuneDefaults(&info, &conf_disk->blkdeviotune, + if (qemuDomainSetBlockIoTuneDefaults(&conf_info, &conf_disk->blkdeviotune, set_fields) < 0) goto endjob; - if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0) + if (virDomainDiskSetBlockIOTune(conf_disk, &conf_info) < 0) goto endjob; if (virDomainDefSave(persistentDef, driver->xmlopt, @@ -19440,6 +19445,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, cleanup: VIR_FREE(info.group_name); + VIR_FREE(conf_info.group_name); virDomainObjEndAPI(&vm); if (eventNparams) virTypedParamsFree(eventParams, eventNparams); -- 2.23.0

Currently upon successfull call to qemu's implementation of virDomainSetBlockIoTune iotune settings are changed only for the disk given in API if the disk is in iotune group while we need to change the settings for all disks in the group. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1ee25aebd..5713266329 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19102,6 +19102,29 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, } +static void +qemuDomainSetGroupBlockIoTune(virDomainDefPtr def, + virDomainBlockIoTuneInfoPtr iotune) +{ + size_t i; + + if (!iotune->group_name) + return; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr d = def->disks[i]; + + if (STREQ_NULLABLE(d->blkdeviotune.group_name, iotune->group_name)) { + char *tmp; + + tmp = d->blkdeviotune.group_name; + d->blkdeviotune = *iotune; + d->blkdeviotune.group_name = tmp; + } + } +} + + static int qemuDomainSetBlockIoTune(virDomainPtr dom, const char *path, @@ -19409,6 +19432,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (virDomainDiskSetBlockIOTune(disk, &info) < 0) goto endjob; + qemuDomainSetGroupBlockIoTune(def, &info); + if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) goto endjob; @@ -19434,6 +19459,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (virDomainDiskSetBlockIOTune(conf_disk, &conf_info) < 0) goto endjob; + qemuDomainSetGroupBlockIoTune(persistentDef, &conf_info); + if (virDomainDefSave(persistentDef, driver->xmlopt, cfg->configDir) < 0) goto endjob; -- 2.23.0

For example if disk is not in the group and we want to move it there then it makes sense to specify only the group name in API call. Currently the destination group iotune settings will be overwritten with the disk settings which I would say is not what one would expect. Thus let's get defaults from the group we are moving to. And if we are moving the brand new group then is makes sense to copy the current disk iotune settings to the group. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5713266329..5da3748dc7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19125,6 +19125,28 @@ qemuDomainSetGroupBlockIoTune(virDomainDefPtr def, } +static virDomainBlockIoTuneInfoPtr +qemuDomainFindGroupBlockIoTune(virDomainDefPtr def, + virDomainDiskDefPtr disk, + virDomainBlockIoTuneInfoPtr newiotune) +{ + size_t i; + + if (!newiotune->group_name || + STREQ_NULLABLE(disk->blkdeviotune.group_name, newiotune->group_name)) + return &disk->blkdeviotune; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr d = def->disks[i]; + + if (STREQ_NULLABLE(newiotune->group_name, d->blkdeviotune.group_name)) + return &d->blkdeviotune; + } + + return &disk->blkdeviotune; +} + + static int qemuDomainSetBlockIoTune(virDomainPtr dom, const char *path, @@ -19154,6 +19176,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; + virDomainBlockIoTuneInfoPtr cur_info; + virDomainBlockIoTuneInfoPtr conf_cur_info; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -19375,7 +19400,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (qemuDomainSetBlockIoTuneDefaults(&info, &disk->blkdeviotune, + cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); + + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) < 0) goto endjob; @@ -19452,7 +19479,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (qemuDomainSetBlockIoTuneDefaults(&conf_info, &conf_disk->blkdeviotune, + conf_cur_info = qemuDomainFindGroupBlockIoTune(persistentDef, conf_disk, &info); + + if (qemuDomainSetBlockIoTuneDefaults(&conf_info, conf_cur_info, set_fields) < 0) goto endjob; -- 2.23.0

Currently when disk is removed from iotune group (by setting all tunables to zero) group name is leaved in config. Let's fix it. Given iotune defaults are taken from the destination group setting tunables to zero may require different set of zero settings in API call. Let's prohibit removing from group while specifying different group name then current for the sanity sake. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5da3748dc7..7c75d502d5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19147,6 +19147,28 @@ qemuDomainFindGroupBlockIoTune(virDomainDefPtr def, } +static int +qemuDomainCheckBlockIoTuneReset(virDomainDiskDefPtr disk, + virDomainBlockIoTuneInfoPtr newiotune) +{ + if (virDomainBlockIoTuneInfoHasAny(newiotune)) + return 0; + + if (newiotune->group_name && + STRNEQ_NULLABLE(newiotune->group_name, disk->blkdeviotune.group_name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("creating a new group/updating existing with all" + " tune parameters zero is not supported")); + return -1; + } + + /* all zero means remove any throttling and remove from group for qemu */ + VIR_FREE(newiotune->group_name); + + return 0; +} + + static int qemuDomainSetBlockIoTune(virDomainPtr dom, const char *path, @@ -19406,6 +19428,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_fields) < 0) goto endjob; + if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) + goto endjob; + #define CHECK_MAX(val, _bool) \ do { \ if (info.val##_max) { \ @@ -19485,6 +19510,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, set_fields) < 0) goto endjob; + if (qemuDomainCheckBlockIoTuneReset(conf_disk, &conf_info) < 0) + goto endjob; + if (virDomainDiskSetBlockIOTune(conf_disk, &conf_info) < 0) goto endjob; -- 2.23.0