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