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

[Michal Prívozník]

On 08/20/2018 05:04 PM, Daniel P. Berrangé wrote:
> On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:
>> On Fri, Aug 17, 2018 at 10:49:12AM -0400, 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?
>>>
>>> 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.
>>
>> Yeah, this doesn't feel right to me. I think we need to treat the
>> metadata locking as completely independant of the content locking.
>> This implies we should have a separate configuration for metadata
>> locking, where the only valid options are "lockd" or "nop".
>>
>> eg our available matrix looks like
>>
>>                         METADATA
>>                  | nop   lockd  sanlock
>>         ---------+--------------------	
>>              nop |  Y      Y      N
>> CONTENT    lockd |  Y      Y      N
>>          sanlock |  Y      Y      N

Having some troubles parsing the table. Do you mean:

         | Content | Metadata
---------+-------------------
     nop |    Y    |    Y
   lockd |    Y    |    Y
 sanlock |    Y    |    N

Where Y says the respective type (content/metadata) can/cannot be locked
by lock driver in question? I.e. we would have 'metadata_lock_manager'
config value in qemu.conf and it'd accept only 'nop' and 'lockd'.

Then we can have 'metadata_lockspace_dir' in
/etc/libvirt/qemu-lockd.conf where the lockspace would be created.

Anyway, I'm gonna state the obvious because it might not be that obvious
to somebody else: we can change the RPC between libvirtd and virtlockd
safely, because we require both to be the same version. I'm saying that
just in case I need to alter the RPC a bit (found some unused procs
there, btw but they might come handy someday).

> 
> Oh and even for virtlockd we need to consider the config separately
> for content vs metdata.

Do you mean like completely new config file? qemu-lockd-metadata.conf?
Okay. So far we only need one config option (metadata_lockspace_dir) but
it might turn out we need more and it would make sens to keep options
separated from content locking options.

> 
> For content we have the possibility to lock out of band files as a
> proxy for the main file. This is primarily done for protecting
> shared blocks devices as locking the device node doesn't apply
> across hosts.
> 
> For metadata locking, we only ever care about the local device node
> as changes to labelling/ownership don't propagate across hosts. IOW,
> we should always directly lock the file in question for metadata
> locking, and never use an out of band proxy for locking.

Oh right. From this POV locking from secdriver seems like even better
idea.

Michal