[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Mon, Aug 20, 2018 at 07:29:30PM +0200, Michal Prívozník wrote:
> 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 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'.

Heh, ok, yes, that is a simpler table :-)

> Then we can have 'metadata_lockspace_dir' in
> /etc/libvirt/qemu-lockd.conf where the lockspace would be created.

No need for a metadata_lockspace_dir, since we should always be locking
the resource itself, never an out of band proxy file.

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

I don't think we need a config file for now.


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]