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@xxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
