Re: [libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
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(a)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