[libvirt] [PATCH v3 16/28] lock_driver: Introduce KEEP_OPEN flags

[John Ferlan]

On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> This flag causes connection to be opened when needed (e.g. when
> calling virLockManagerLockDaemonAcquire for the first time) and
> instead of closing it at the end of such API store it in
> privateData so that it can be reused by later calls.
> 
> This is needed because if a resource is acquired and connection
> is closed then virtlockd kills the registered PID (that's what
> virtlockd is designed to do). Therefore we will need the
> connection to open at drvAcquire and close not any sooner than
> drvRelease. However, as we will be locking files step-by-step we
> want to avoid opening new connection for every drvAcquire +
> drvRelease pair, so the connection is going to be shared even
> more than that. But more on that in next commit.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/locking/lock_driver.h       |  7 +++++
>  src/locking/lock_driver_lockd.c | 68 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index 59c4c3aac7..7e3ffc58b5 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -67,8 +67,15 @@ typedef enum {
>      VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0),
>      /* Prevent further lock/unlock calls from this process */
>      VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1),
> +    /* Causes driver to keep connection open and reuse it for further use. */
> +    VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN = (1 << 2),
>  } virLockManagerAcquireFlags;
>  
> +typedef enum {
> +    /* Reuse previously saved connection. */
> +    VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN = (1 << 0),
> +} virLockManagerReleaseFlags;
> +
>  typedef enum {
>      /* virLockManagerNew called for a freshly started domain */
>      VIR_LOCK_MANAGER_NEW_STARTED = (1 << 0),
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 4883e89ac6..14f9eae760 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -76,6 +76,11 @@ struct _virLockManagerLockDaemonPrivate {
>  
>      size_t nresources;
>      virLockManagerLockDaemonResourcePtr resources;
> +
> +    int clientRefs;
> +    virNetClientPtr client;
> +    virNetClientProgramPtr program;
> +    int counter;
>  };
>  
>  
> @@ -440,6 +445,13 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv)
>      default:
>          break;
>      }
> +
> +    if (priv->client) {
> +        virNetClientClose(priv->client);
> +        virObjectUnref(priv->client);
> +        virObjectUnref(priv->program);
> +    }
> +

How about a helper method that would do these?

I wonder now if the @priv should keep track of flags as well?

That way you could "always" use @priv to store the client, program, and
counter and then use helper methods to determine at close whether @flags
had the KEEP_OPEN or not instead of having KEEP_OPEN in various places.

>      VIR_FREE(priv);
>  }
>  
> @@ -770,7 +782,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>      virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>  
>      virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
> -                  VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
> +                  VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
> +                  VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN, -1);
>  
>      if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
>          priv->nresources == 0 &&
> @@ -781,7 +794,14 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>          return -1;
>      }
>  
> -    if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> +    if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> +        client = priv->client;
> +        program = priv->program;
> +        counter = priv->counter;
> +    }
> +
> +    if (!client &&
> +        !(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))

If "always" storing in @priv this alters to if !priv->client &&
!(priv->client ==...

>          goto cleanup;
>  
>      if (fd &&
> @@ -814,11 +834,25 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>          virLockManagerLockDaemonConnectionRestrict(lock, client, program, &counter) < 0)
>          goto cleanup;
>  
> +    if (flags & VIR_LOCK_MANAGER_ACQUIRE_KEEP_OPEN) {
> +        VIR_STEAL_PTR(priv->client, client);
> +        VIR_STEAL_PTR(priv->program, program);
> +        priv->counter = counter;
> +    }
> +

If "always" using @priv means this isn't necessary as long as @flags is
also stored.

>      rv = 0;
>  
>   cleanup:
> -    if (rv != 0 && fd)
> -        VIR_FORCE_CLOSE(*fd);
> +    if (rv < 0) {
> +        if (fd)
> +            VIR_FORCE_CLOSE(*fd);
> +
> +        priv->client = NULL;
> +        priv->program = NULL;
> +        priv->counter = 0;
> +        priv->clientRefs = 0;

So one failure this time causes previously stored successes to be thrown
away? Or am I reading too much into this?  Why is clientRefs not
auto-incremented here, but auto-decremented in Release?

This is where I'd think a helper would be able to know the "last"
referenced was removed and thus be able to perform the close and free of
resources.

Calling *PrivateFree and finding that there's extra clientRefs
would/could mean something is programatically wrong, right?

> +    }
> +
>      virNetClientClose(client);
>      virObjectUnref(client);
>      virObjectUnref(program);


Always storing in @priv, priv->flags, and using @rv I would think could
easily be managed in a helper...

> @@ -837,12 +871,20 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
>      size_t i;
>      virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN, -1);
>  
>      if (state)
>          *state = NULL;
>  
> -    if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> +    if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) {
> +        client = priv->client;
> +        program = priv->program;
> +        counter = priv->counter;
> +        priv->clientRefs--;

This decrements something from 0...

> +    }
> +
> +    if (!client &&
> +        !(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
>          goto cleanup;
>  
>      for (i = 0; i < priv->nresources; i++) {
> @@ -870,9 +912,23 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
>              goto cleanup;
>      }
>  
> +    if (flags & VIR_LOCK_MANAGER_RELEASE_KEEP_OPEN) {
> +        /* Avoid freeing in cleanup. */
> +        client = NULL;
> +        program = NULL;
> +        counter = 0;
> +    }
> +
>      rv = 0;
>  
>   cleanup:
> +    if (rv < 0) {
> +        priv->client = NULL;
> +        priv->program = NULL;
> +        priv->counter = 0;
> +        priv->clientRefs = 0;
> +    }
> +

Again, I'm not clear why when this release fails we go off and clear out
everything?  Including things that may have been connected before?
Certain a properly documented helper would solve this mystery for me.

One that could manage things based on KEEP_OPEN, @priv, and @rv. I would
assume the close open when KEEP_OPEN is true would be where the
auto-decrement would occur and be checked to determine whether the
subsequent closes and unref's happen.

John

>      virNetClientClose(client);
>      virObjectUnref(client);
>      virObjectUnref(program);
>