[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Fix performance & reliabilty of QMP probing



On 01/24/2013 01:34 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
>
> This previous commit
>
>   commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9
>   Author: Viktor Mihajlovski <mihajlov linux vnet ibm com>
>   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 redhat com>
> ---
>  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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]