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

[Daniel P. Berrangé]

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


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|