On 08/27/2018 04:08 AM, Michal Privoznik wrote:
After the previous commit we have VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN flag. This is not enough because it will keep connection open for only one instance of drvAcquire + drvRelease call. And when starting up a domain there will be a lot of such calls as there will be a lot of paths to relabel and thus lock. Therfore, VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN flag was introduced which allows us to keep connection open even after
s/was/will be/ (or is) s/which allows us to keep/to allow keeping the/
the drvAcquire + drvRelease pair. In order to close the connection after all locking has been done virLockManagerCloseConn is introduced.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/locking/lock_driver.h | 22 ++++++++++++++++++++++ src/locking/lock_driver_lockd.c | 24 ++++++++++++++++++++++++ src/locking/lock_driver_nop.c | 8 ++++++++ src/locking/lock_manager.c | 11 +++++++++++ src/locking/lock_manager.h | 4 ++++ 6 files changed, 70 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 42f15f117e..bca5a51ba0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1294,6 +1294,7 @@ virDomainLockProcessStart; virLockManagerAcquire; virLockManagerAddResource; virLockManagerClearResources; +virLockManagerCloseConn; virLockManagerFree; virLockManagerInquire; virLockManagerNew; diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 7e3ffc58b5..d81767707b 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -282,6 +282,27 @@ typedef int (*virLockDriverRelease)(virLockManagerPtr man, char **state, unsigned int flags);
+/** + * virLockDriverCloseConn: + * @man: the lock manager context + * @flags: optional flags, currently unused + * + * Close any connection that was saved via + * VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN or + * VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN flags.
Hmm, a client that forgets to provide KEEP_OPEN on one or the other would wreak havoc with the algorithm. Upon further thought, the KEEP_OPEN really only matters for Acquire and it should be saved in @priv. That way the @priv would manage it not whether the flag was provided on release.
+ * However, if there is still a resource locked, do not actually + * close the connection as it would result in killing the + * resource owner. This is similar to refcounting when all + * threads call virLockDriverCloseConn() but only the last one + * actually closes the connection. + *
NB: virObject{Ref|Unlock} uses atomic incr/decr for ref counting.
+ * Returns: 0 on success and connection not actually closed, + * 1 on success and connection closed, + * -1 otherwise + */ +typedef int (*virLockDriverCloseConn)(virLockManagerPtr man, + unsigned int flags); + /** * virLockDriverInquire: * @manager: the lock manager context @@ -328,6 +349,7 @@ struct _virLockDriver {
virLockDriverAcquire drvAcquire; virLockDriverRelease drvRelease; + virLockDriverCloseConn drvCloseConn; virLockDriverInquire drvInquire; };
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 14f9eae760..aec768b0df 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -937,6 +937,28 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, }
+static int virLockManagerLockDaemonCloseConn(virLockManagerPtr lock, + unsigned int flags)
static int virLock...
+{ + virLockManagerLockDaemonPrivatePtr priv = lock->privateData; + + virCheckFlags(0, -1); + + if (priv->clientRefs) + return 0; + + virNetClientClose(priv->client); + virObjectUnref(priv->client); + virObjectUnref(priv->program); + + priv->client = NULL; + priv->program = NULL; + priv->counter = 0; + + return 1;
The helper concept from the previous patch still could apply here. I would say I'm surprised this did anything in testing since clientRefs wouldn't be 0 if acquire, then release is called w/ the KEEP_OPEN flag. It'd be -1, I believe. The rest seems fine. John [...]