From dd94f36ffbe8ba0fef093d88099be760c87d21a1 Mon Sep 17 00:00:00 2001 From: Nikolay Shirokovskiy Date: Wed, 8 Jan 2020 09:49:27 +0300 Subject: [PATCH] qemu: check iotune params same for all disk in group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Michal Privoznik Reviewed-by: Daniel P. Berrangé Reviewed-by: Michal Privoznik --- src/conf/domain_conf.c | 26 +++++++++++++++++++++++++ src/conf/domain_conf.h | 7 ++++++- 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, 79 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index daa145b493..2443e408ed 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31795,3 +31795,29 @@ virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src, *dst = *src; dst->group_name = g_strdup(src->group_name); } + + +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 adfa043169..5feb70938e 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 virDomainBlockIoTuneInfoCopy. */ + /* Don't forget to update virDomainBlockIoTuneInfoCopy and + * virDomainBlockIoTuneInfoEqual. */ }; @@ -3715,3 +3716,7 @@ virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune); void virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src, virDomainBlockIoTuneInfoPtr dst); + +bool +virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, + const virDomainBlockIoTuneInfo *b); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7158f0959..970f412527 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,7 @@ virDomainActualNetDefValidate; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; virDomainBlockIoTuneInfoCopy; +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 51bdc74e2d..146be25c8b 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;