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&gt; >>>> --- >>>> 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.

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