1:54 a.m.
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
1:54 a.m.
New subject: [PATCH v2 1/7] qemu: Move qemuDiskConfigBlkdeviotuneHas* to conf
From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
And introduce virDomainBlockIoTuneInfoHasAny.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1:54 a.m.
New subject: [PATCH v2 2/7] conf: expand iotune params if only group name is given
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
2:37 a.m.
New subject: [PATCH v2 2/7] conf: expand iotune params if only group name is given
On 29.01.2020 10:54, Michal Privoznik wrote:
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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;
}
1:54 a.m.
New subject: [PATCH v2 3/7] qemu: propagate iotune settings to all disks in the group
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1:54 a.m.
New subject: [PATCH v2 4/7] qemu: check iotune params same for all disk in group
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1:54 a.m.
New subject: [PATCH v2 5/7] qemu: fix using defaults when setting persistent iotune params
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1:54 a.m.
New subject: [PATCH v2 6/7] qemu: get defaults from iotune group we move disk into
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1:54 a.m.
New subject: [PATCH v2 7/7] qemu: when leaving iotune group update xml properly
From: Nikolay Shirokovskiy <nshirokovskiy(a)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(a)virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
1759
days inactive
1759
days old
9 comments
2 participants
participants (2)
-
Michal Privoznik -
Nikolay Shirokovskiy