Re: [libvirt] [PATCH 08/10] Replace polling for active VMs with signalling by drivers
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(a)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.
+++ 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...
char *base = NULL;
DBusConnection *sysbus = NULL;
@@ -305,27 +307,6 @@ nwfilterDriverReload(void) {
return 0;
}
-/**
- * virNWFilterActive:
- *
- * Checks if the nwfilter driver is active, i.e. has an active
nwfilter
- *
- * Returns 1 if active, 0 otherwise
- */
-static int
-nwfilterDriverActive(void) {
- int ret;
-
- if (!driverState)
- return 0;
-
- nwfilterDriverLock(driverState);
- ret = driverState->nwfilters.count ? 1 : 0;
- ret |= driverState->watchingFirewallD;
- nwfilterDriverUnlock(driverState);
-
- return ret;
But the old code could inhibit shutdown if a nwfilter was active.
Is this intentional?
+
+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?
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?
Overall, the idea is nice, but answers to my questions will determine
whether you need a v2.