On 01/24/2013 01:34 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> This previous commit
>
> commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9
> Author: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
> Date: Mon Nov 26 15:17:13 2012 +0100
>
> qemu: Fix QMP Capabability Probing Failure
>
> which attempted to make sure the QEMU process used for probing
> ran as the right user id, caused serious performance regression
> and unreliability in probing. The -daemonize switch in QEMU
> guarantees that the monitor socket is present before the parent
> process exits. This means libvirtd is guaranteed to be able to
> connect immediately. By switching from -daemonize to the
> virCommandDaemonize API libvirtd was no longer synchronized with
> QEMU's startup process. The result was that the QEMU monitor
> failed to open and went into its 200ms sleep loop. This happened
> for all 25 binaries resulting in 5 seconds worth of sleeping
> at libvirtd startup. In addition sometimes when libvirt connected,
> QEMU would be partially initialized and crash causing total
> failure to probe that binary.
>
> This commit reverts the previous change, ensuring we do use the
> -daemonize flag to QEMU. Startup delay is cut from 7 seconds
> to 2 seconds on my machine, which is on a par with what it was
> prior to the capabilities rewrite.
>
> To deal with the fact that QEMU needs to be able to create the
> pidfile, we switch pidfile location fron runDir to libDir, which
> QEMU is guaranteed to be able to write to.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 50 ++++++++++++++++++++++++++++++--------------
> src/qemu/qemu_capabilities.h | 3 +--
> 2 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 95fa3da..703179d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -38,6 +38,7 @@
> #include "virbitmap.h"
> #include "virnodesuspend.h"
> #include "qemu_monitor.h"
> +#include "virtime.h"
>
> #include <sys/stat.h>
> #include <unistd.h>
> @@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache)
> * so just probe for them all - we gracefully fail
> * if a qemu-system-$ARCH binary can't be found
> */
> - for (i = 0 ; i < VIR_ARCH_LAST ; i++)
> + unsigned long long a, b;
> + for (i = 0 ; i < VIR_ARCH_LAST ; i++) {
> + unsigned long long start, end;
> + if (virTimeMillisNow(&start) < 0)
> + goto error;
> if (qemuCapsInitGuest(caps, cache,
> virArchFromHost(),
> i) < 0)
> goto error;
> + if (virTimeMillisNow(&end) < 0)
> + goto error;
> + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start);
> + }
Did you intend to leave in this debug code? (if you did, you need to
move the definition of a & b up to the top of the block, and maybe
rename them to something more descriptive)
>
> /* QEMU Requires an emulator in the XML */
> virCapabilitiesSetEmulatorRequired(caps);
> @@ -2291,7 +2300,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps)
> static int
> qemuCapsInitQMP(qemuCapsPtr caps,
> const char *libDir,
> - const char *runDir,
> uid_t runUid,
> gid_t runGid)
> {
> @@ -2324,8 +2332,11 @@ qemuCapsInitQMP(qemuCapsPtr caps,
>
> /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash
> * with a qemu domain called "capabilities"
> + * Normally we'd use runDir for pid files, but because we're using
> + * -daemonize we need QEMU to be allowed to create them, rather
> + * than libvirtd. So we're using libDir which QEMU can write to
> */
> - if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) {
> + if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) {
> virReportOOMError();
> goto cleanup;
> }
> @@ -2337,6 +2348,13 @@ qemuCapsInitQMP(qemuCapsPtr caps,
>
> VIR_DEBUG("Try to get caps via QMP caps=%p", caps);
>
> + /*
> + * We explicitly need to use -daemonize here, rather than
> + * virCommandDaemonize, because we need to synchronize
> + * with QEMU creating its monitor socket API. Using
> + * daemonize guarnatees control won't return to libvirt
s/guarnatees/guarantees/
> + * until the socket is present.
> + */
> cmd = virCommandNewArgList(caps->binary,
> "-S",
> "-no-user-config",
> @@ -2344,14 +2362,14 @@ qemuCapsInitQMP(qemuCapsPtr caps,
> "-nographic",
> "-M", "none",
> "-qmp", monarg,
> + "-pidfile", pidfile,
> + "-daemonize",
> NULL);
> virCommandAddEnvPassCommon(cmd);
> virCommandClearCaps(cmd);
> hookData.runUid = runUid;
> hookData.runGid = runGid;
> virCommandSetPreExecHook(cmd, qemuCapsHook, &hookData);
> - virCommandSetPidFile(cmd, pidfile);
> - virCommandDaemonize(cmd);
>
> if (virCommandRun(cmd, &status) < 0)
> goto cleanup;
> @@ -2472,7 +2490,6 @@ cleanup:
>
> qemuCapsPtr qemuCapsNewForBinary(const char *binary,
> const char *libDir,
> - const char *runDir,
> uid_t runUid,
> gid_t runGid)
> {
> @@ -2502,12 +2519,14 @@ qemuCapsPtr qemuCapsNewForBinary(const char *binary,
> goto error;
> }
>
> - if ((rv = qemuCapsInitQMP(caps, libDir, runDir, runUid, runGid)) < 0)
> + if ((rv = qemuCapsInitQMP(caps, libDir, runUid, runGid)) < 0)
> goto error;
>
> - if (!caps->usedQMP &&
> - qemuCapsInitHelp(caps, runUid, runGid) < 0)
> - goto error;
> + if (!caps->usedQMP) {
> + VIR_ERROR("Falling back to help");
> + if (qemuCapsInitHelp(caps, runUid, runGid) < 0)
> + goto error;
> + }
>
> return caps;
>
> @@ -2542,8 +2561,9 @@ qemuCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED)
>
>
> qemuCapsCachePtr
> -qemuCapsCacheNew(const char *libDir, const char *runDir,
> - uid_t runUid, gid_t runGid)
> +qemuCapsCacheNew(const char *libDir,
> + uid_t runUid,
> + gid_t runGid)
> {
> qemuCapsCachePtr cache;
>
> @@ -2561,8 +2581,7 @@ qemuCapsCacheNew(const char *libDir, const char *runDir,
>
> if (!(cache->binaries = virHashCreate(10, qemuCapsHashDataFree)))
> goto error;
> - if (!(cache->libDir = strdup(libDir)) ||
> - !(cache->runDir = strdup(runDir))) {
> + if (!(cache->libDir = strdup(libDir))) {
> virReportOOMError();
> goto error;
> }
> @@ -2594,7 +2613,7 @@ qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary)
> if (!ret) {
> VIR_DEBUG("Creating capabilities for %s",
> binary);
> - ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runDir,
> + ret = qemuCapsNewForBinary(binary, cache->libDir,
> cache->runUid, cache->runGid);
> if (ret) {
> VIR_DEBUG("Caching capabilities %p for %s",
> @@ -2634,7 +2653,6 @@ qemuCapsCacheFree(qemuCapsCachePtr cache)
> return;
>
> VIR_FREE(cache->libDir);
> - VIR_FREE(cache->runDir);
> virHashFree(cache->binaries);
> virMutexDestroy(&cache->lock);
> VIR_FREE(cache);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 089fa30..5279d07 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -179,7 +179,6 @@ qemuCapsPtr qemuCapsNew(void);
> qemuCapsPtr qemuCapsNewCopy(qemuCapsPtr caps);
> qemuCapsPtr qemuCapsNewForBinary(const char *binary,
> const char *libDir,
> - const char *runDir,
> uid_t runUid,
> gid_t runGid);
>
> @@ -219,7 +218,7 @@ int qemuCapsGetMachineTypesCaps(qemuCapsPtr caps,
> bool qemuCapsIsValid(qemuCapsPtr caps);
>
>
> -qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, const char *runDir,
> +qemuCapsCachePtr qemuCapsCacheNew(const char *libDir,
> uid_t uid, gid_t gid);
> qemuCapsPtr qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary);
> qemuCapsPtr qemuCapsCacheLookupCopy(qemuCapsCachePtr cache, const char *binary);
ACK with the above issues fixed.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list