2:55 a.m.
Couple of issues were found with my recent work on IOThread and its pool
size limits. Here are fixes.
Michal Prívozník (5):
domain_conf: Format <defaultiothread/> more often
domain_conf: Format iothread IDs more often
qemu: Make IOThread changing more robust
qemuDomainSetIOThreadParams: Accept VIR_DOMAIN_AFFECT_CONFIG flag
virsh: Implement --config for iothreadset
src/conf/domain_conf.c | 50 ++++++++++----------
src/qemu/qemu_driver.c | 101 +++++++++++++++++++++++------------------
tools/virsh-domain.c | 8 ++++
3 files changed, 90 insertions(+), 69 deletions(-)
--
2.35.1
2:55 a.m.
New subject: [PATCH 1/5] domain_conf: Format <defaultiothread/> more often
The <defaultiothread/> element is formatted inside
virDomainDefaultIOThreadDefFormat() which is called only from
virDomainDefIOThreadsFormat() (so that IOThread related stuff is
formatted calling one function). However, when there are no
<iothreadids/> defined (or only autoallocated ones are present),
then the outer formatting function exits early never calling the
<defaultiothread/> formatter.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 46 ++++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b639022396..24f17a8b91 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26349,40 +26349,38 @@ static void
virDomainDefIOThreadsFormat(virBuffer *buf,
const virDomainDef *def)
{
- g_auto(virBuffer) childrenBuf = VIR_BUFFER_INIT_CHILD(buf);
- size_t i;
-
- if (def->niothreadids == 0)
- return;
+ if (def->niothreadids > 0) {
+ virBufferAsprintf(buf, "<iothreads>%zu</iothreads>\n",
+ def->niothreadids);
+ }
- virBufferAsprintf(buf, "<iothreads>%zu</iothreads>\n",
- def->niothreadids);
+ if (virDomainDefIothreadShouldFormat(def)) {
+ g_auto(virBuffer) childrenBuf = VIR_BUFFER_INIT_CHILD(buf);
+ size_t i;
- if (!virDomainDefIothreadShouldFormat(def))
- return;
+ for (i = 0; i < def->niothreadids; i++) {
+ virDomainIOThreadIDDef *iothread = def->iothreadids[i];
+ g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
- for (i = 0; i < def->niothreadids; i++) {
- virDomainIOThreadIDDef *iothread = def->iothreadids[i];
- g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+ virBufferAsprintf(&attrBuf, " id='%u'",
+ iothread->iothread_id);
- virBufferAsprintf(&attrBuf, " id='%u'",
- iothread->iothread_id);
+ if (iothread->thread_pool_min >= 0) {
+ virBufferAsprintf(&attrBuf, "
thread_pool_min='%d'",
+ iothread->thread_pool_min);
+ }
- if (iothread->thread_pool_min >= 0) {
- virBufferAsprintf(&attrBuf, " thread_pool_min='%d'",
- iothread->thread_pool_min);
- }
+ if (iothread->thread_pool_max >= 0) {
+ virBufferAsprintf(&attrBuf, "
thread_pool_max='%d'",
+ iothread->thread_pool_max);
+ }
- if (iothread->thread_pool_max >= 0) {
- virBufferAsprintf(&attrBuf, " thread_pool_max='%d'",
- iothread->thread_pool_max);
+ virXMLFormatElement(&childrenBuf, "iothread", &attrBuf,
NULL);
}
- virXMLFormatElement(&childrenBuf, "iothread", &attrBuf, NULL);
+ virXMLFormatElement(buf, "iothreadids", NULL, &childrenBuf);
}
- virXMLFormatElement(buf, "iothreadids", NULL, &childrenBuf);
-
virDomainDefaultIOThreadDefFormat(buf, def);
}
--
2.35.1
2:55 a.m.
New subject: [PATCH 2/5] domain_conf: Format iothread IDs more often
When formatting IOThreads (in virDomainDefIOThreadsFormat()), we
may only output the number of IOThreads, or the full list of IOThreads too:
<iothreads>4</iothreads>
<iothreadids>
<iothread id='1' thread_pool_max='10'/>
<iothread id='2' thread_pool_min='2'
thread_pool_max='10'/>
<iothread id='3'/>
<iothread id='4'/>
</iothreadids>
Now, the deciding factor here is whether those individual
IOThreads were so called 'autofill-ed' or user provided. Well, we
need to take another factor in: if an IOThread has pool size
limit set, then we ought to format the full list.
But how can we get into a situation when a thread is autofilled
(i.e. not provided by user in the XML) and yet it has pool size
limit set? virDomainSetIOThreadParams() is the answer.
Sure, we could also unset the autofill flag whenever a pool size
limit is being set. But this approach allows us to not format
anything if the limits are reset (we don't lose the autofill
information).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 24f17a8b91..e80616fe7b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26314,7 +26314,9 @@ virDomainDefIothreadShouldFormat(const virDomainDef *def)
size_t i;
for (i = 0; i < def->niothreadids; i++) {
- if (!def->iothreadids[i]->autofill)
+ if (!def->iothreadids[i]->autofill ||
+ def->iothreadids[i]->thread_pool_min >= 0 ||
+ def->iothreadids[i]->thread_pool_max >= 0)
return true;
}
--
2.35.1
2:55 a.m.
New subject: [PATCH 3/5] qemu: Make IOThread changing more robust
There are three APIs that allow changing IOThreads:
virDomainAddIOThread()
virDomainDelIOThread()
virDomainSetIOThreadParams()
In case of QEMU driver these are handled by
qemuDomainChgIOThread() which attempts to be versatile enough to
work on both inactive and live domain definitions at the same
time. However, it's a bit clumsy - when a change to live
definition succeeds but fails in inactive definition then there's
no rollback. And somewhat rightfully so - changes to live
definition are in general harder to roll back. Therefore, do what
we do elsewhere (qemuDomainAttachDeviceLiveAndConfig(),
qemuDomainDetachDeviceAliasLiveAndConfig(), ...):
1) do the change to inactive XML first,
2) in fact, do the change to a copy of inactive XML,
3) swap inactive XML and its copy only after everything
succeeded.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c4418df1ed..e30b1b8d84 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5595,6 +5595,7 @@ qemuDomainChgIOThread(virQEMUDriver *driver,
{
g_autoptr(virQEMUDriverConfig) cfg = NULL;
qemuDomainObjPrivate *priv;
+ g_autoptr(virDomainDef) defcopy = NULL;
virDomainDef *def;
virDomainDef *persistentDef;
virDomainIOThreadIDDef *iothreaddef = NULL;
@@ -5610,6 +5611,55 @@ qemuDomainChgIOThread(virQEMUDriver *driver,
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
goto endjob;
+ if (persistentDef) {
+ /* Make a copy of persistent definition and do all the changes there.
+ * Swap the definitions only after changes to live definition
+ * succeeded. */
+ if (!(defcopy = virDomainObjCopyPersistentDef(vm, driver->xmlopt,
+ priv->qemuCaps)))
+ return -1;
+
+ switch (action) {
+ case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
+ if (virDomainDriverAddIOThreadCheck(defcopy, iothread.iothread_id) < 0)
+ goto endjob;
+
+ if (!virDomainIOThreadIDAdd(defcopy, iothread.iothread_id))
+ goto endjob;
+
+ break;
+
+ case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
+ if (virDomainDriverDelIOThreadCheck(defcopy, iothread.iothread_id) < 0)
+ goto endjob;
+
+ virDomainIOThreadIDDel(defcopy, iothread.iothread_id);
+
+ break;
+
+ case VIR_DOMAIN_IOTHREAD_ACTION_MOD:
+ iothreaddef = virDomainIOThreadIDFind(defcopy, iothread.iothread_id);
+
+ if (!iothreaddef) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("cannot find IOThread '%u' in
iothreadids"),
+ iothread.iothread_id);
+ goto endjob;
+ }
+
+ if (qemuDomainIOThreadValidate(iothreaddef, iothread, false) < 0)
+ goto endjob;
+
+ if (qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread) < 0) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("configuring persistent polling values is not
supported"));
+ goto endjob;
+ }
+
+ break;
+ }
+ }
+
if (def) {
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -5660,50 +5710,12 @@ qemuDomainChgIOThread(virQEMUDriver *driver,
qemuDomainSaveStatus(vm);
}
- if (persistentDef) {
- switch (action) {
- case VIR_DOMAIN_IOTHREAD_ACTION_ADD:
- if (virDomainDriverAddIOThreadCheck(persistentDef, iothread.iothread_id) <
0)
- goto endjob;
-
- if (!virDomainIOThreadIDAdd(persistentDef, iothread.iothread_id))
- goto endjob;
-
- break;
-
- case VIR_DOMAIN_IOTHREAD_ACTION_DEL:
- if (virDomainDriverDelIOThreadCheck(persistentDef, iothread.iothread_id) <
0)
- goto endjob;
-
- virDomainIOThreadIDDel(persistentDef, iothread.iothread_id);
-
- break;
-
- case VIR_DOMAIN_IOTHREAD_ACTION_MOD:
- iothreaddef = virDomainIOThreadIDFind(persistentDef, iothread.iothread_id);
-
- if (!iothreaddef) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("cannot find IOThread '%u' in
iothreadids"),
- iothread.iothread_id);
- goto endjob;
- }
-
- if (qemuDomainIOThreadValidate(iothreaddef, iothread, false) < 0)
- goto endjob;
-
- if (qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread) < 0) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("configuring persistent polling values is not
supported"));
- goto endjob;
- }
-
- break;
- }
-
- if (virDomainDefSave(persistentDef, driver->xmlopt,
- cfg->configDir) < 0)
+ /* Finally, if no error until here, we can save config. */
+ if (defcopy) {
+ if (virDomainDefSave(defcopy, driver->xmlopt, cfg->configDir) < 0)
goto endjob;
+
+ virDomainObjAssignDef(vm, &defcopy, false, NULL);
}
ret = 0;
--
2.35.1
2:55 a.m.
New subject: [PATCH 4/5] qemuDomainSetIOThreadParams: Accept VIR_DOMAIN_AFFECT_CONFIG flag
It was always possible to modify the inactive XML, because
VIR_DOMAIN_AFFECT_CURRENT (= 0) is accepted implicitly. But now
that the logic when changing both config and live XMLs is more
robust we can accept VIR_DOMAIN_AFFECT_CONFIG flag too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e30b1b8d84..da95f947e7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5819,7 +5819,8 @@ qemuDomainSetIOThreadParams(virDomainPtr dom,
qemuMonitorIOThreadInfo iothread = {0};
int ret = -1;
- virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
+ virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
+ VIR_DOMAIN_AFFECT_CONFIG, -1);
if (iothread_id == 0) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
--
2.35.1
2:55 a.m.
New subject: [PATCH 5/5] virsh: Implement --config for iothreadset
Our man page already documents that iothreadset has --config
argument. Well, it doesn't really. Normally, I'd just fix the man
page, but with recent work on the API it's possible to tweak
values for inactive XML too. Therefore, implement the --config
argument for the command.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-domain.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 43034f2f81..da63cc95ff 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7831,6 +7831,7 @@ static const vshCmdOptDef opts_iothreadset[] = {
.type = VSH_OT_INT,
.help = N_("upper boundary for worker thread pool")
},
+ VIRSH_COMMON_OPT_DOMAIN_CONFIG,
VIRSH_COMMON_OPT_DOMAIN_LIVE,
VIRSH_COMMON_OPT_DOMAIN_CURRENT,
{.name = NULL}
@@ -7842,6 +7843,8 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd)
g_autoptr(virshDomain) dom = NULL;
int id = 0;
bool ret = false;
+ bool current = vshCommandOptBool(cmd, "current");
+ bool config = vshCommandOptBool(cmd, "config");
bool live = vshCommandOptBool(cmd, "live");
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
virTypedParameterPtr params = NULL;
@@ -7852,8 +7855,13 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd)
int thread_val;
int rc;
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
if (live)
flags |= VIR_DOMAIN_AFFECT_LIVE;
+ if (config)
+ flags |= VIR_DOMAIN_AFFECT_CONFIG;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
--
2.35.1
3:53 a.m.
On a Friday in 2022, Michal Privoznik wrote:
Couple of issues were found with my recent work on IOThread and its
pool
size limits. Here are fixes.
Michal Prívozník (5):
domain_conf: Format <defaultiothread/> more often
domain_conf: Format iothread IDs more often
qemu: Make IOThread changing more robust
qemuDomainSetIOThreadParams: Accept VIR_DOMAIN_AFFECT_CONFIG flag
virsh: Implement --config for iothreadset
src/conf/domain_conf.c | 50 ++++++++++----------
src/qemu/qemu_driver.c | 101 +++++++++++++++++++++++------------------
tools/virsh-domain.c | 8 ++++
3 files changed, 90 insertions(+), 69 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
898
days inactive
903
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik