[libvirt] [PATCH 0/7] qemu: block iotune group fixes/improvements
Currently iotune group impl has several bugs/oddities this patchset aims to fix. Also patches (2) and (3) add/change functionality so that group iotune became easier/saner to use. TODO: add docs to API/virsh in respect to using iotune group Nikolay Shirokovskiy (7): conf: refactor virDomainBlockIoTuneInfoHas* conf: expand iotune params if only group name is given (2) qemu: check iotune params same for all disk in group qemu: fix using defaults when setting persistent iotune params qemu: propagate iotune settings to all disks in the group qemu: get defaults from iotune group we move disk into (3) qemu: when leaving iotune group update xml properly src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 16 ++++++ src/libvirt_private.syms | 5 ++ src/qemu/qemu_command.c | 90 ++++++++++++++------------------- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 2 +- 8 files changed, 262 insertions(+), 58 deletions(-) -- 2.23.0
And introduce virDomainBlockIoTuneInfoHasAny. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.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 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
On Wed, Jan 08, 2020 at 09:49:25AM +0300, Nikolay Shirokovskiy wrote:
And introduce virDomainBlockIoTuneInfoHasAny.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@virtuozzo.com> --- 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
On Wed, Jan 08, 2020 at 09:49:26AM +0300, Nikolay Shirokovskiy wrote:
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
convenient
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
"easily" and s/add disk/add a disk/
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> --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- 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
On Wed, Jan 08, 2020 at 09:49:27AM +0300, Nikolay Shirokovskiy wrote:
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> --- 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/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) {
I'm not a fan of special casing for the test suite
+ 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 ||
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 ||
Passing invalid parameters to an API, only in the test suite, has tended to cause pain for us in the long term. Can we create a valid virDomainDef to pass in - it doens't have to contain a full VM really - just enough functionality to satisify the method
qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) { VIR_TEST_VERBOSE("invalid configuration for disk"); goto cleanup;
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- 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
On Wed, Jan 08, 2020 at 09:49:28AM +0300, Nikolay Shirokovskiy wrote:
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> --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- 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
On Wed, Jan 08, 2020 at 09:49:29AM +0300, Nikolay Shirokovskiy wrote:
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> --- src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
I'd suggest re-ordering so that this patch is *before* the patch 3 which forbids the inconsistent disk settings.
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;
nitpick - assign tmp at time of declaration.
+ d->blkdeviotune = *iotune; + d->blkdeviotune.group_name = tmp; + } + } +}
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- 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
On Wed, Jan 08, 2020 at 09:49:30AM +0300, Nikolay Shirokovskiy wrote:
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> --- src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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> --- 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
On Wed, Jan 08, 2020 at 09:49:31AM +0300, Nikolay Shirokovskiy wrote:
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> --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
ping On 08.01.2020 09:49, Nikolay Shirokovskiy wrote:
Currently iotune group impl has several bugs/oddities this patchset aims to fix. Also patches (2) and (3) add/change functionality so that group iotune became easier/saner to use.
TODO: add docs to API/virsh in respect to using iotune group
Nikolay Shirokovskiy (7): conf: refactor virDomainBlockIoTuneInfoHas* conf: expand iotune params if only group name is given (2) qemu: check iotune params same for all disk in group qemu: fix using defaults when setting persistent iotune params qemu: propagate iotune settings to all disks in the group qemu: get defaults from iotune group we move disk into (3) qemu: when leaving iotune group update xml properly
src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 16 ++++++ src/libvirt_private.syms | 5 ++ src/qemu/qemu_command.c | 90 ++++++++++++++------------------- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 2 +- 8 files changed, 262 insertions(+), 58 deletions(-)
On 1/8/20 7:49 AM, Nikolay Shirokovskiy wrote:
Currently iotune group impl has several bugs/oddities this patchset aims to fix. Also patches (2) and (3) add/change functionality so that group iotune became easier/saner to use.
TODO: add docs to API/virsh in respect to using iotune group
Nikolay Shirokovskiy (7): conf: refactor virDomainBlockIoTuneInfoHas* conf: expand iotune params if only group name is given (2) qemu: check iotune params same for all disk in group qemu: fix using defaults when setting persistent iotune params qemu: propagate iotune settings to all disks in the group qemu: get defaults from iotune group we move disk into (3) qemu: when leaving iotune group update xml properly
src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 16 ++++++ src/libvirt_private.syms | 5 ++ src/qemu/qemu_command.c | 90 ++++++++++++++------------------- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 2 +- 8 files changed, 262 insertions(+), 58 deletions(-)
I will repost your patches with the fixes I think should be merged into individual patches, if that is okay with you. Michal
On 29.01.2020 10:14, Michal Privoznik wrote:
On 1/8/20 7:49 AM, Nikolay Shirokovskiy wrote:
Currently iotune group impl has several bugs/oddities this patchset aims to fix. Also patches (2) and (3) add/change functionality so that group iotune became easier/saner to use.
TODO: add docs to API/virsh in respect to using iotune group
Nikolay Shirokovskiy (7): conf: refactor virDomainBlockIoTuneInfoHas* conf: expand iotune params if only group name is given (2) qemu: check iotune params same for all disk in group qemu: fix using defaults when setting persistent iotune params qemu: propagate iotune settings to all disks in the group qemu: get defaults from iotune group we move disk into (3) qemu: when leaving iotune group update xml properly
src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 16 ++++++ src/libvirt_private.syms | 5 ++ src/qemu/qemu_command.c | 90 ++++++++++++++------------------- src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- tests/qemublocktest.c | 2 +- 8 files changed, 262 insertions(+), 58 deletions(-)
I will repost your patches with the fixes I think should be merged into individual patches, if that is okay with you.
Sure.
participants (3)
-
Daniel P. Berrangé -
Michal Privoznik -
Nikolay Shirokovskiy