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

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(a)redhat.com&gt; > --- > 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.