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

[Michal Privoznik]

On 08/31/2018 03:26 PM, John Ferlan wrote:
> 
> 
> 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.

We have to have the flag for release too. Imagine the following scenario:

ThreadA:
  conn = connect_to_virtlockd();
  lock(conn, pathX);
  chown(pathX);

  /* context switch to threadB */

ThreadB:
  conn = connect_to_virtlockd(); /* This will actually reuse the
connection */
  lock(conn, pathY);
  chown(pathY);
  unlock(conn, pathY);
  close_connection(conn);

  /* context switch back to threaA */

ThreadA:
  unlock(conn, pathX);
  close_connection(conn);

We certainly want the unlock() to NOT close the connection when called
from threadB because that would trigger the killer code on virtlockd
side (as we still have pathX locked from threadA).

Anyway, this is internal implementation and can be changed anytime. If
we find better solution (e.g. opening connection at
virLockManagerLockDaemonNew() time).

> 
>> + * 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...

Actually, the whole file uses it the way I have it. I rather be consistent.

> 
>> +{
>> +    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.

Yeah, the clientRefs increment line got lost somewhere. I could swear I
had it but maybe I just forgot to add it into the commit. Dunno.

I'll do the helper.

Michal