[libvirt] [PATCH 0/3] Fixes for issues caused by IOThreads
Well I guess it happens to everyone - hopefully only once though... The IOThreads code really messed up a few things - git bisection for the build is broken as of 5f6ad32c733a3bd158938aecabb0508a434ece95, but that's resolved by 938fb12fad6d15c9fdb73f998c4e0ec1e278721f which adds the 'niothreadspin && iothreadspin' definitions. Secondarily it seems things got worse, because guests weren't able to be started because of a very bad logic error (<= 0 vs. < 0 and specific check that 0 is "OK" (meaning no IOThreads). How that got by own self testing I'm still not quite sure, but it did and it's a mea culpa for that. Anyway, patch 1 fixes the running issue... Patch 2 fixes some spacing issues that Eric pointed out privately (prefer "i + 1" vs. "i+1"). Patch 3 just makes sure to use the right cgroup setup/init parameters since there is no "iothread0" John Ferlan (3): qemu: Fix iothreads issue qemu_cgroup: Adjust spacing around incrementor qemu: Fix call in qemuDomainSetNumaParamsLive for virCgroupNewIOThread src/qemu/qemu_cgroup.c | 7 ++++--- src/qemu/qemu_driver.c | 3 ++- src/qemu/qemu_process.c | 6 +++++- 3 files changed, 11 insertions(+), 5 deletions(-) -- 1.9.3
If there are no iothreads, then return from qemuProcessDetectIOThreadPIDs without error; otherwise, the following occurs: error: Failed to start domain $dom error: An error occurred, but the cause is unknown Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6c412db..a2efdca 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2110,9 +2110,13 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, goto cleanup; niothreads = qemuMonitorGetIOThreads(priv->mon, &iothreads); qemuDomainObjExitMonitor(driver, vm); - if (niothreads <= 0) + if (niothreads < 0) goto cleanup; + /* Nothing to do */ + if (niothreads == 0) + return 0; + if (niothreads != vm->def->iothreads) { virReportError(VIR_ERR_INTERNAL_ERROR, _("got wrong number of IOThread pids from QEMU monitor. " -- 1.9.3
Change "i+1" to "i + 1" Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index c1d89bb..9d39370 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1140,7 +1140,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* IOThreads are numbered 1..n, although the array is 0..n-1, * so we will account for that here */ - if (virCgroupNewIOThread(priv->cgroup, i+1, true, &cgroup_iothread) < 0) + if (virCgroupNewIOThread(priv->cgroup, i + 1, true, + &cgroup_iothread) < 0) goto cleanup; /* move the thread for iothread to sub dir */ @@ -1159,13 +1160,13 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) * qemuSetupCgroupIOThreadsPin will fail. */ for (j = 0; j < def->cputune.niothreadspin; j++) { /* IOThreads are numbered/named 1..n */ - if (def->cputune.iothreadspin[j]->vcpuid != i+1) + if (def->cputune.iothreadspin[j]->vcpuid != i + 1) continue; if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, def->cputune.iothreadspin, def->cputune.niothreadspin, - i+1) < 0) + i + 1) < 0) goto cleanup; break; -- 1.9.3
On 09/15/2014 08:13 PM, John Ferlan wrote:
Change "i+1" to "i + 1"
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index c1d89bb..9d39370 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1140,7 +1140,8 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) /* IOThreads are numbered 1..n, although the array is 0..n-1, * so we will account for that here */ - if (virCgroupNewIOThread(priv->cgroup, i+1, true, &cgroup_iothread) < 0) + if (virCgroupNewIOThread(priv->cgroup, i + 1, true, + &cgroup_iothread) < 0) goto cleanup;
/* move the thread for iothread to sub dir */ @@ -1159,13 +1160,13 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) * qemuSetupCgroupIOThreadsPin will fail. */ for (j = 0; j < def->cputune.niothreadspin; j++) { /* IOThreads are numbered/named 1..n */ - if (def->cputune.iothreadspin[j]->vcpuid != i+1) + if (def->cputune.iothreadspin[j]->vcpuid != i + 1) continue;
if (qemuSetupCgroupIOThreadsPin(cgroup_iothread, def->cputune.iothreadspin, def->cputune.niothreadspin, - i+1) < 0) + i + 1) < 0) goto cleanup;
break;
And another one to squash in - hurrying too much for the late hour: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a2efdca..daca7c1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2358,7 +2358,7 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) /* set affinity only for existing vcpus */ if (!(pininfo = virDomainVcpuPinFindByVcpu(def->cputune.iothreadspin, def->cputune.niothreadspin, - i+1))) + i + 1))) continue; if (virProcessSetAffinity(priv->iothreadpids[i], pininfo->cpumask) < 0)
Found by inspection of the "i+1" change. IOThreads are numbered 1..n thus the virCgroupNewIOThread needs to create a 1..n value not 0 based. Signed-off-by: John Ferlan <jferlan@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 bf78f8d..6008aeb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8760,7 +8760,8 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, goto cleanup; for (i = 0; i < priv->niothreadpids; i++) { - if (virCgroupNewIOThread(priv->cgroup, i, false, &cgroup_temp) < 0 || + if (virCgroupNewIOThread(priv->cgroup, i + 1, false, + &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; virCgroupFree(&cgroup_temp); -- 1.9.3
On 09/15/2014 06:13 PM, John Ferlan wrote:
Well I guess it happens to everyone - hopefully only once though...
Yep, the brown bag of shame lets you learn from mistakes rather quickly :)
The IOThreads code really messed up a few things - git bisection for the build is broken as of 5f6ad32c733a3bd158938aecabb0508a434ece95, but that's resolved by 938fb12fad6d15c9fdb73f998c4e0ec1e278721f which adds the 'niothreadspin && iothreadspin' definitions.
Secondarily it seems things got worse, because guests weren't able to be started because of a very bad logic error (<= 0 vs. < 0 and specific check that 0 is "OK" (meaning no IOThreads).
How that got by own self testing I'm still not quite sure, but it did and it's a mea culpa for that.
Anyway, patch 1 fixes the running issue... Patch 2 fixes some spacing issues that Eric pointed out privately (prefer "i + 1" vs. "i+1"). Patch 3 just makes sure to use the right cgroup setup/init parameters since there is no "iothread0"
John Ferlan (3): qemu: Fix iothreads issue qemu_cgroup: Adjust spacing around incrementor qemu: Fix call in qemuDomainSetNumaParamsLive for virCgroupNewIOThread
Thanks for the fast fixes. I confirm that patch 1 fixes the regression in starting my dummy transient domain. ACK series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/15/2014 08:55 PM, Eric Blake wrote:
On 09/15/2014 06:13 PM, John Ferlan wrote:
Well I guess it happens to everyone - hopefully only once though...
Yep, the brown bag of shame lets you learn from mistakes rather quickly :)
The IOThreads code really messed up a few things - git bisection for the build is broken as of 5f6ad32c733a3bd158938aecabb0508a434ece95, but that's resolved by 938fb12fad6d15c9fdb73f998c4e0ec1e278721f which adds the 'niothreadspin && iothreadspin' definitions.
Secondarily it seems things got worse, because guests weren't able to be started because of a very bad logic error (<= 0 vs. < 0 and specific check that 0 is "OK" (meaning no IOThreads).
How that got by own self testing I'm still not quite sure, but it did and it's a mea culpa for that.
Anyway, patch 1 fixes the running issue... Patch 2 fixes some spacing issues that Eric pointed out privately (prefer "i + 1" vs. "i+1"). Patch 3 just makes sure to use the right cgroup setup/init parameters since there is no "iothread0"
John Ferlan (3): qemu: Fix iothreads issue qemu_cgroup: Adjust spacing around incrementor qemu: Fix call in qemuDomainSetNumaParamsLive for virCgroupNewIOThread
Thanks for the fast fixes. I confirm that patch 1 fixes the regression in starting my dummy transient domain. ACK series.
Now pushed John
Hi John With your patch and latest qemu-kvm, I can't start guest. # rpm -q qemu-kvm qemu-kvm-1.5.3-70.el7.x86_64 # virsh start test error: Failed to start domain test error: internal error: unable to execute QEMU command 'query-iothreads': The command query-iothreads has not been found On 09/16/2014 08:55 AM, Eric Blake wrote:
On 09/15/2014 06:13 PM, John Ferlan wrote:
Well I guess it happens to everyone - hopefully only once though... Yep, the brown bag of shame lets you learn from mistakes rather quickly :)
The IOThreads code really messed up a few things - git bisection for the build is broken as of 5f6ad32c733a3bd158938aecabb0508a434ece95, but that's resolved by 938fb12fad6d15c9fdb73f998c4e0ec1e278721f which adds the 'niothreadspin && iothreadspin' definitions.
Secondarily it seems things got worse, because guests weren't able to be started because of a very bad logic error (<= 0 vs. < 0 and specific check that 0 is "OK" (meaning no IOThreads).
How that got by own self testing I'm still not quite sure, but it did and it's a mea culpa for that.
Anyway, patch 1 fixes the running issue... Patch 2 fixes some spacing issues that Eric pointed out privately (prefer "i + 1" vs. "i+1"). Patch 3 just makes sure to use the right cgroup setup/init parameters since there is no "iothread0"
John Ferlan (3): qemu: Fix iothreads issue qemu_cgroup: Adjust spacing around incrementor qemu: Fix call in qemuDomainSetNumaParamsLive for virCgroupNewIOThread Thanks for the fast fixes. I confirm that patch 1 fixes the regression in starting my dummy transient domain. ACK series.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards shyu
On 09/16/2014 03:44 AM, shyu wrote:
Hi John
With your patch and latest qemu-kvm, I can't start guest.
# rpm -q qemu-kvm qemu-kvm-1.5.3-70.el7.x86_64
# virsh start test error: Failed to start domain test error: internal error: unable to execute QEMU command 'query-iothreads': The command query-iothreads has not been found
This is the gift that keeps on giving... It's a caps thing. I'll send a patch shortly, for a preview... diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0df041a..f02f305 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2105,6 +2105,9 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, int ret = -1; size_t i; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + /* Get the list of IOThreads from qemu */ if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup;
On 09/16/2014 08:55 AM, Eric Blake wrote:
On 09/15/2014 06:13 PM, John Ferlan wrote:
Well I guess it happens to everyone - hopefully only once though... Yep, the brown bag of shame lets you learn from mistakes rather quickly :)
The IOThreads code really messed up a few things - git bisection for the build is broken as of 5f6ad32c733a3bd158938aecabb0508a434ece95, but that's resolved by 938fb12fad6d15c9fdb73f998c4e0ec1e278721f which adds the 'niothreadspin && iothreadspin' definitions.
Secondarily it seems things got worse, because guests weren't able to be started because of a very bad logic error (<= 0 vs. < 0 and specific check that 0 is "OK" (meaning no IOThreads).
How that got by own self testing I'm still not quite sure, but it did and it's a mea culpa for that.
Anyway, patch 1 fixes the running issue... Patch 2 fixes some spacing issues that Eric pointed out privately (prefer "i + 1" vs. "i+1"). Patch 3 just makes sure to use the right cgroup setup/init parameters since there is no "iothread0"
John Ferlan (3): qemu: Fix iothreads issue qemu_cgroup: Adjust spacing around incrementor qemu: Fix call in qemuDomainSetNumaParamsLive for virCgroupNewIOThread Thanks for the fast fixes. I confirm that patch 1 fixes the regression in starting my dummy transient domain. ACK series.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards shyu
Prior to trying the query-iothreads call - check if the qemu has the capability Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index daca7c1..f391743 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2105,6 +2105,9 @@ qemuProcessDetectIOThreadPIDs(virQEMUDriverPtr driver, int ret = -1; size_t i; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + /* Get the list of IOThreads from qemu */ if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; -- 1.9.3
On 09/16/2014 12:10 PM, John Ferlan wrote:
Prior to trying the query-iothreads call - check if the qemu has the capability
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 3 +++ 1 file changed, 3 insertions(+)
ACK Jan
On 09/16/2014 07:16 AM, Ján Tomko wrote:
On 09/16/2014 12:10 PM, John Ferlan wrote:
Prior to trying the query-iothreads call - check if the qemu has the capability
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 3 +++ 1 file changed, 3 insertions(+)
ACK
Jan
pushed thanks... John
participants (4)
-
Eric Blake -
John Ferlan -
Ján Tomko -
shyu