[libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully

[Michal Prívozník]

On 08/17/2018 04:49 PM, John Ferlan wrote:
> 
> 
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> No real support implemented here. But hey, at least we will not
>> fail.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
>> index 3e5f0e37b0..c1996fb937 100644
>> --- a/src/locking/lock_driver_sanlock.c
>> +++ b/src/locking/lock_driver_sanlock.c
>> @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>>      virLockManagerSanlockPrivatePtr priv = lock->privateData;
>>  
>>      virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
>> -                  VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
>> +                  VIR_LOCK_MANAGER_RESOURCE_SHARED |
>> +                  VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>>  
>>      if (priv->res_count == SANLK_MAX_RESOURCES) {
>>          virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>>      if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>>          return 0;
>>  
>> +    /* No metadata locking support for now.
>> +     * TODO: implement it. */
>> +    if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
>> +        return 0;
>> +
> 
> Doesn't this give someone the false impression that their resource is
> locked if they choose METADATA?

Okay, I'll provide some implementation. Even though sanlock is not
really my cup of coffee :-)

> 
> Something doesn't feel right about that - giving the impression that
> it's supported and the consumer is protected, but when push comes to
> shove they aren't.
> 
> I'd be inclined to believe that we may want to do nothing with/for
> sanlock allowing the virCheckFlags above take care of the consumer.

No we must not do this. Metadata locking is not something users can turn
on or off. Okay, they can turn it off when they disable all the locking
(i.e. comment out lock_manager in qemu.conf). But if they would have
sanlock turned on and wanted to start a domain the virCheckFlags() would
be triggered right away and starting a domain would be denied.

> 
>>      switch (type) {
>>      case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>>          if (driver->autoDiskLease) {
>> @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
>>      virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
>>                    VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);
>>  
>> -    if (priv->res_count == 0 &&
>> -        priv->hasRWDisks &&
>> -        driver->requireLeaseForDisks) {
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                       _("Read/write, exclusive access, disks were present, but no leases specified"));
>> -        return -1;
>> +    if (priv->res_count == 0) {
>> +        if (priv->hasRWDisks &&
>> +            driver->requireLeaseForDisks) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("Read/write, exclusive access, disks were present, but no leases specified"));
>> +            return -1;
>> +        }
>> +
>> +        /* We are not handling METADATA flag yet. So no resources
>> +         * case is no-op for now. */
>> +        return 0;
> 
> Similar comment to patch4 w/r/t resource type (e.g. disk or lease), but
> now at least it's more obvious to me that hasRWDisks means lock for disk
> vs. not.
> 
> Still it's odd to think that returning 0, but not actually getting the
> lock is the "right" thing to do. "Theoretically", if AddResource failed,
> then would Acquire ever be called w/ this flag?

Sure it would. Look at virDomainLockProcessStart() for instance. It
calls virDomainLockManagerNew() which adds all the resources via
AddResource() and then calls Acquire().

> 
>>      }
>>  
>>      /* We only initialize 'sock' if we are in the real
>>
> 
> BTW:
> 
> Now that I think about them together...
> 
>   - lock_driver_lockd.h - extract from patch4 and merge to patch3. I
> suppose it could be separate too to keep mgmt vs. space separate, but
> they're related enough to keep them together. Perhaps this is where a
> few more "words" about usage expectations as part of the commit message.>
>   - lock_daemon_dispatch.c - extract from patch4 and whether it goes
> before or after is one of those chicken/egg type quandaries. Keeping it
> with the fcntl/lockd as opposed to the sanlock implementation doesn't
> feel right (even though sanlock doesn't use any LOCK_SPACE symbols).

I rather keep the patches as they are now. It makes more sense to me
this way.

Michal