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