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

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