[libvirt] [PATCH v3 16/28] lock_driver: Introduce KEEP_OPEN flags
Michal Privoznik
mprivozn at redhat.com
Mon Sep 3 15:13:42 UTC 2018
On 08/31/2018 02:32 PM, John Ferlan wrote:
>
>
> 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.
I'm not sure we can do this. There are places (e.g. regular disk content
locking code) where Acquire() and Release() are called separately and on
behalf of other process (domain is the owner of the lock). Definitely we
do not want any connection sharing there. Therefore I'm failing to see
how would Acquire() determine if it should save the connection for later
use or not.
>
>> 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.
The code needs to handle both cases, if the flag is set and if it isn't.
I don't want to write the code twice with the only difference being in
the variable accessed (priv->client vs client).
>
>> 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?
Yes. The client socket is closed by code below. There's not much point
in keeping stale pointer to what used to be connection.
> Or am I reading too much into this? Why is clientRefs not
> auto-incremented here, but auto-decremented in Release?
Oooops. Yes. Somehow the line that increments the clientRefs counter
when grabbing the client got lost. Damn you small dwarfs who make some
lines disappear! :-D
>
> 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?
Yes. We don't want to free client data at this point.
>
>> + }
>> +
>> 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.
Maybe it is too drastic. But rather to be safe then sorry. If something
goes wrong, we can't be sure what state the connection is in and it's
better for the caller to open new connection.
>
> 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.
Well, do we want each caller to re-implement this semantic? I say it's
better to have it at one place.
Michal
More information about the libvir-list
mailing list