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

[Daniel P. Berrange]

On Thu, Jan 24, 2013 at 02:59:49PM -0500, Laine Stump wrote:
> On 01/24/2013 01:34 PM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> >
> > This previous commit
> >
> >   commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9
> >   Author: Viktor Mihajlovski <mihajlov at 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 at 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)

No, it should have been removed.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|