[libvirt] [PATCH 08/10] Replace polling for active VMs with signalling by drivers

[Daniel P. Berrange]

On Tue, Nov 27, 2012 at 03:53:48PM -0500, Eric Blake wrote:
> > Currently to deal with auto-shutdown libvirtd must periodically
> > poll all stateful drivers. Thus sucks because it requires
> > acquiring both the driver lock and locks on every single virtual
> > machine. Instead pass in a "inhibit" callback to virStateInitialize
> > which drivers can invoke whenever they want to inhibit shutdown
> > due to existance of active VMs.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> 
> > @@ -772,6 +762,18 @@ static int daemonSetupSignals(virNetServerPtr
> > srv)
> >      return 0;
> >  }
> >  
> > +
> > +static void daemonInhibitCallback(bool inhibit, void *opaque)
> > +{
> > +    virNetServerPtr srv = opaque;
> > +
> > +    if (inhibit)
> > +        virNetServerAddShutdownInhibition(srv);
> > +    else
> > +        virNetServerRemoveShutdownInhibition(srv);
> > +}
> 
> Nice.  Is the intent that drivers call this once on the first
> guest to start, and again on the last guest stopped, or once
> on every single guest start/stop action?  Either way, it seems
> like the inhibition has to be reference counted to deal with
> more than one driver having a reason for inhibition among a
> single libvirtd service.

The only real requirement is that the drivers have a matching
number of calls to turn it on/off. The way I've coded things
though, the drivers only call it in first guest start, and last
guest stop.  virNetServer does indeed do reference counting.

> 
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -165,7 +165,9 @@ nwfilterDriverInstallDBusMatches(DBusConnection
> > *sysbus ATTRIBUTE_UNUSED)
> >   * Initialization function for the QEmu daemon
> >   */
> >  static int
> > -nwfilterDriverStartup(bool privileged)
> > +nwfilterDriverStartup(bool privileged ATTRIBUTE_UNUSED,
> > +                      virStateInhibitCallback callback
> > ATTRIBUTE_UNUSED,
> > +                      void *opaque ATTRIBUTE_UNUSED)
> >  {
> 
> Here, you aren't remembering the callback...

Yes, this is technically a semantic change, I could have pulled
into a separate patch.

Basically there is no compelling reason for the nwfilter driver
to inhibit shutdown. Active NWfilters are all associated with
active domains, which will already be inhibiting shutdown.

> > +
> > +void virNetServerAddShutdownInhibition(virNetServerPtr srv)
> > +{
> > +    virNetServerLock(srv);
> > +    srv->autoShutdownInhibitions++;
> > +    virNetServerUnlock(srv);
> > +}
> > +
> > +
> > +void virNetServerRemoveShutdownInhibition(virNetServerPtr srv)
> > +{
> > +    virNetServerLock(srv);
> > +    srv->autoShutdownInhibitions--;
> > +    virNetServerUnlock(srv);
> > +}
> 
> Do these have to obtain full-blown locks, or can you use atomic
> increments outside of any other locking?

In the next method, these functions grow more code, so the locks
are actually protecting something reasonable

> 
> >  static int
> > -storageDriverStartup(bool privileged)
> > +storageDriverStartup(bool privileged,
> > +                     virStateInhibitCallback callback
> > ATTRIBUTE_UNUSED,
> > +                     void *opaque ATTRIBUTE_UNUSED)
> 
> Another case of ignoring the callback...
> 
> >  {
> >      char *base = NULL;
> >  
> > @@ -202,33 +204,6 @@ storageDriverReload(void) {
> >      return 0;
> >  }
> >  
> > -/**
> > - * virStorageActive:
> > - *
> > - * Checks if the storage driver is active, i.e. has an active pool
> > - *
> > - * Returns 1 if active, 0 otherwise
> > - */
> > -static int
> > -storageDriverActive(void) {
> > -    unsigned int i;
> > -    int active = 0;
> > -
> > -    if (!driverState)
> > -        return 0;
> > -
> > -    storageDriverLock(driverState);
> > -
> > -    for (i = 0 ; i < driverState->pools.count ; i++) {
> > -        virStoragePoolObjLock(driverState->pools.objs[i]);
> > -        if (virStoragePoolObjIsActive(driverState->pools.objs[i]))
> > -            active = 1;
> > -        virStoragePoolObjUnlock(driverState->pools.objs[i]);
> > -    }
> > -
> > -    storageDriverUnlock(driverState);
> > -    return active;
> 
> ...where the old code could inhibit shutdown.  Intentional?

Again this is because, IMHO, there is no compelling reason for active
storage pools to inhibit libvirtd shutdown. Indeed inhibiting it makes
auto-shutdown pretty much useless, since you'll almost always have an
active directory based storage pool.

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 :|