[PATCH v2 0/7] qemu: block iotune group fixes/improvements
This series was originally posted by Nikolay here: https://www.redhat.com/archives/libvir-list/2020-January/msg00283.html I've delayed review too much. Sorry for that. To make it up to you, I've fixed some patches and resending the series. Basically, I want to know whether you agree with my adjustments. I will add Dan's Reviewed-by when merging. Nikolay Shirokovskiy (7): qemu: Move qemuDiskConfigBlkdeviotuneHas* to conf conf: expand iotune params if only group name is given qemu: propagate iotune settings to all disks in the group qemu: check iotune params same for all disk in group qemu: fix using defaults when setting persistent iotune params qemu: get defaults from iotune group we move disk into qemu: when leaving iotune group update xml properly src/conf/domain_conf.c | 108 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 22 ++++++++ src/libvirt_private.syms | 6 +++ src/qemu/qemu_command.c | 91 ++++++++++++++------------------- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_driver.c | 94 ++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 8 ++- 8 files changed, 275 insertions(+), 59 deletions(-) -- 2.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> And introduce virDomainBlockIoTuneInfoHasAny. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 9f7fe2aa78..c77901dfa0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31712,3 +31712,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 5d736e99c0..a037d9c2f2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3698,3 +3698,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 3608f73b4e..eea90ce0dc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -226,6 +226,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 3378413ee1..f4b7251aab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1137,50 +1137,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); } @@ -1198,9 +1159,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")); @@ -1228,7 +1187,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 " @@ -1246,7 +1205,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.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Currently, 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 convenient 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 easily add a 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c77901dfa0..0eeffb6ed0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5110,6 +5110,31 @@ 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]; + char *tmp = disk->blkdeviotune.group_name; + + if (STRNEQ_NULLABLE(disk->blkdeviotune.group_name, d->blkdeviotune.group_name) || + !virDomainBlockIoTuneInfoHasAny(&d->blkdeviotune)) + continue; + + disk->blkdeviotune = d->blkdeviotune; + disk->blkdeviotune.group_name = tmp; + return; + } +} + + static int virDomainDiskDefPostParse(virDomainDiskDefPtr disk, const virDomainDef *def, @@ -5155,6 +5180,8 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, return -1; } + virDomainDiskExpandGroupIoTune(disk, def); + return 0; } -- 2.24.1
On 29.01.2020 10:54, Michal Privoznik wrote:
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Currently, 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 convenient 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 easily add a 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c77901dfa0..0eeffb6ed0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5110,6 +5110,31 @@ 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]; + char *tmp = disk->blkdeviotune.group_name; + + if (STRNEQ_NULLABLE(disk->blkdeviotune.group_name, d->blkdeviotune.group_name) || + !virDomainBlockIoTuneInfoHasAny(&d->blkdeviotune)) + continue; + + disk->blkdeviotune = d->blkdeviotune; + disk->blkdeviotune.group_name = tmp;
We can use virDomainBlockIoTuneInfoCopy here too. And it makes sense to add freeing of group_name in the copy function then I guess. Everything else is fine. Nikolay
+ return; + } +} + + static int virDomainDiskDefPostParse(virDomainDiskDefPtr disk, const virDomainDef *def, @@ -5155,6 +5180,8 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk, return -1; }
+ virDomainDiskExpandGroupIoTune(disk, def); + return 0; }
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 048855b533..dfc7111937 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19117,6 +19117,26 @@ 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)) { + VIR_FREE(d->blkdeviotune.group_name); + virDomainBlockIoTuneInfoCopy(iotune, &d->blkdeviotune); + } + } +} + + static int qemuDomainSetBlockIoTune(virDomainPtr dom, const char *path, @@ -19419,6 +19439,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (virDomainDiskSetBlockIOTune(disk, &info) < 0) goto endjob; + qemuDomainSetGroupBlockIoTune(def, &info); + if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) goto endjob; @@ -19444,6 +19466,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0) goto endjob; + qemuDomainSetGroupBlockIoTune(persistentDef, &conf_info); + if (virDomainDefSave(persistentDef, driver->xmlopt, cfg->configDir) < 0) goto endjob; -- 2.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 26 +++++++++++++++++++++++++ src/conf/domain_conf.h | 5 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 8 ++++++-- 8 files changed, 78 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0eeffb6ed0..89c9ed2834 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31785,3 +31785,29 @@ virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune) virDomainBlockIoTuneInfoHasMax(iotune) || virDomainBlockIoTuneInfoHasMaxLength(iotune); } + + +bool +virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, + const virDomainBlockIoTuneInfo *b) +{ + return 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; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a037d9c2f2..5c3156a0aa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -489,6 +489,7 @@ struct _virDomainBlockIoTuneInfo { unsigned long long total_iops_sec_max_length; unsigned long long read_iops_sec_max_length; unsigned long long write_iops_sec_max_length; + /* Don't forget to update virDomainBlockIoTuneInfoEqual. */ }; @@ -3710,3 +3711,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 eea90ce0dc..5e9eb34459 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -226,6 +226,7 @@ virDomainActualNetDefFree; virDomainActualNetDefValidate; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; +virDomainBlockIoTuneInfoEqual; virDomainBlockIoTuneInfoHasAny; virDomainBlockIoTuneInfoHasBasic; virDomainBlockIoTuneInfoHasMax; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f4b7251aab..1668c85666 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1155,6 +1155,7 @@ qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk) */ static int qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { /* group_name by itself is ignored by qemu */ @@ -1166,6 +1167,28 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk, return -1; } + /* checking def here is only for calling from tests */ + if (disk->blkdeviotune.group_name) { + size_t i; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr d = def->disks[i]; + + if (STREQ(d->dst, disk->dst) || + STRNEQ_NULLABLE(d->blkdeviotune.group_name, + disk->blkdeviotune.group_name)) + continue; + + if (!virDomainBlockIoTuneInfoEqual(&d->blkdeviotune, + &disk->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 || @@ -1228,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) { @@ -1728,6 +1752,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk, static char * qemuBuildDriveStr(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER; @@ -1754,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) return NULL; virBufferAsprintf(&opt, "if=%s", @@ -1896,7 +1921,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, g_autofree char *scsiVPDDeviceId = NULL; int controllerModel; - if (qemuCheckDiskConfig(disk, qemuCaps) < 0) + if (qemuCheckDiskConfig(disk, def, qemuCaps) < 0) return NULL; if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) @@ -2401,6 +2426,7 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommandPtr cmd, static int qemuBuildDiskSourceCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceChainData) data = NULL; @@ -2420,7 +2446,7 @@ qemuBuildDiskSourceCommandLine(virCommandPtr cmd, !(copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk))) return -1; } else { - if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, qemuCaps))) + if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, def, qemuCaps))) return -1; } @@ -2450,7 +2476,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)) { @@ -10164,6 +10190,7 @@ qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) */ qemuBlockStorageSourceAttachDataPtr qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; @@ -10171,7 +10198,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; @@ -10229,6 +10256,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src, */ qemuBlockStorageSourceChainDataPtr qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk, + const virDomainDef *def, virQEMUCapsPtr qemuCaps) { g_autoptr(qemuBlockStorageSourceAttachData) elem = NULL; @@ -10237,7 +10265,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 dfc7111937..255b77cbb4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8136,7 +8136,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 31d455505b..83cc5468c2 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 7ff6a6b17b..3076dc9645 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -185,6 +185,7 @@ static int testQemuDiskXMLToProps(const void *opaque) { struct testQemuDiskXMLToJSONData *data = (void *) opaque; + g_autoptr(virDomainDef) vmdef = NULL; virDomainDiskDefPtr disk = NULL; virStorageSourcePtr n; virJSONValuePtr formatProps = NULL; @@ -204,7 +205,11 @@ testQemuDiskXMLToProps(const void *opaque) VIR_DOMAIN_DEF_PARSE_STATUS))) goto cleanup; - if (qemuCheckDiskConfig(disk, data->qemuCaps) < 0 || + if (!(vmdef = virDomainDefNew()) || + virDomainDiskInsert(vmdef, disk) < 0) + goto cleanup; + + if (qemuCheckDiskConfig(disk, vmdef, data->qemuCaps) < 0 || qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) { VIR_TEST_VERBOSE("invalid configuration for disk"); goto cleanup; @@ -242,7 +247,6 @@ testQemuDiskXMLToProps(const void *opaque) cleanup: virJSONValueFree(formatProps); virJSONValueFree(storageProps); - virDomainDiskDefFree(disk); VIR_FREE(xmlpath); VIR_FREE(xmlstr); return ret; -- 2.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 7 ++++++- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++-- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89c9ed2834..7bbd3fcaaf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31811,3 +31811,12 @@ virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, a->read_iops_sec_max_length == b->read_iops_sec_max_length && a->write_iops_sec_max_length == b->write_iops_sec_max_length; } + + +void +virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src, + virDomainBlockIoTuneInfoPtr dst) +{ + *dst = *src; + dst->group_name = g_strdup(src->group_name); +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5c3156a0aa..0c35c9155d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -489,7 +489,8 @@ struct _virDomainBlockIoTuneInfo { unsigned long long total_iops_sec_max_length; unsigned long long read_iops_sec_max_length; unsigned long long write_iops_sec_max_length; - /* Don't forget to update virDomainBlockIoTuneInfoEqual. */ + /* Don't forget to update virDomainBlockIoTuneInfoEqual + * and virDomainBlockIoTuneInfoCopy. */ }; @@ -3715,3 +3716,7 @@ virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune); bool virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, const virDomainBlockIoTuneInfo *b); + +void +virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src, + virDomainBlockIoTuneInfoPtr dst); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5e9eb34459..970f412527 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -226,6 +226,7 @@ virDomainActualNetDefFree; virDomainActualNetDefValidate; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; +virDomainBlockIoTuneInfoCopy; virDomainBlockIoTuneInfoEqual; virDomainBlockIoTuneInfoHasAny; virDomainBlockIoTuneInfoHasBasic; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 255b77cbb4..eb4b85a20d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19150,6 +19150,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; @@ -19213,6 +19214,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, return -1; memset(&info, 0, sizeof(info)); + memset(&conf_info, 0, sizeof(conf_info)); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -19337,6 +19339,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } + virDomainBlockIoTuneInfoCopy(&info, &conf_info); + if (def) { supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); @@ -19459,11 +19463,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; qemuDomainSetGroupBlockIoTune(persistentDef, &conf_info); @@ -19479,6 +19483,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, cleanup: VIR_FREE(info.group_name); + VIR_FREE(conf_info.group_name); virDomainObjEndAPI(&vm); if (eventNparams) virTypedParamsFree(eventParams, eventNparams); -- 2.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 eb4b85a20d..9bf4d99fff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19137,6 +19137,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, @@ -19166,6 +19188,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); @@ -19386,7 +19411,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; @@ -19463,7 +19490,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.24.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 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@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 9bf4d99fff..3e6d14ead8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19159,6 +19159,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, @@ -19417,6 +19439,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) { \ @@ -19496,6 +19521,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.24.1
participants (2)
-
Michal Privoznik -
Nikolay Shirokovskiy