[libvirt] [PATCH v3 17/28] lock_manager: Introduce virLockManagerCloseConn

[John Ferlan]

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

[...]