[PATCH 0/5] Follow up fixes for IOThreads
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
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@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
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@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
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@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
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@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
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@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
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@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik