[v2] Introduce metadata locking

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

Posted by Michal Privoznik 6 years, 3 months ago

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;
+
     switch (type) {
     case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
         if (driver->autoDiskLease) {
@@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
     virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
                   VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);
 
-    if (priv->res_count == 0 &&
-        priv->hasRWDisks &&
-        driver->requireLeaseForDisks) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("Read/write, exclusive access, disks were present, but no leases specified"));
-        return -1;
+    if (priv->res_count == 0) {
+        if (priv->hasRWDisks &&
+            driver->requireLeaseForDisks) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Read/write, exclusive access, disks were present, but no leases specified"));
+            return -1;
+        }
+
+        /* We are not handling METADATA flag yet. So no resources
+         * case is no-op for now. */
+        return 0;
     }
 
     /* We only initialize 'sock' if we are in the real
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 6 years, 3 months ago


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.

>      switch (type) {
>      case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>          if (driver->autoDiskLease) {
> @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
>      virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
>                    VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);
>  
> -    if (priv->res_count == 0 &&
> -        priv->hasRWDisks &&
> -        driver->requireLeaseForDisks) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                       _("Read/write, exclusive access, disks were present, but no leases specified"));
> -        return -1;
> +    if (priv->res_count == 0) {
> +        if (priv->hasRWDisks &&
> +            driver->requireLeaseForDisks) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Read/write, exclusive access, disks were present, but no leases specified"));
> +            return -1;
> +        }
> +
> +        /* We are not handling METADATA flag yet. So no resources
> +         * case is no-op for now. */
> +        return 0;

Similar comment to patch4 w/r/t resource type (e.g. disk or lease), but
now at least it's more obvious to me that hasRWDisks means lock for disk
vs. not.

Still it's odd to think that returning 0, but not actually getting the
lock is the "right" thing to do. "Theoretically", if AddResource failed,
then would Acquire ever be called w/ this flag?

>      }
>  
>      /* We only initialize 'sock' if we are in the real
> 

BTW:

Now that I think about them together...

  - lock_driver_lockd.h - extract from patch4 and merge to patch3. I
suppose it could be separate too to keep mgmt vs. space separate, but
they're related enough to keep them together. Perhaps this is where a
few more "words" about usage expectations as part of the commit message.

  - lock_daemon_dispatch.c - extract from patch4 and whether it goes
before or after is one of those chicken/egg type quandaries. Keeping it
with the fcntl/lockd as opposed to the sanlock implementation doesn't
feel right (even though sanlock doesn't use any LOCK_SPACE symbols).

I can be swayed otherwise... maybe someone else will pipe in.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Prívozník 6 years, 3 months ago

On 08/17/2018 04:49 PM, 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?

Okay, I'll provide some implementation. Even though sanlock is not
really my cup of coffee :-)

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

No we must not do this. Metadata locking is not something users can turn
on or off. Okay, they can turn it off when they disable all the locking
(i.e. comment out lock_manager in qemu.conf). But if they would have
sanlock turned on and wanted to start a domain the virCheckFlags() would
be triggered right away and starting a domain would be denied.

> 
>>      switch (type) {
>>      case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>>          if (driver->autoDiskLease) {
>> @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,
>>      virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
>>                    VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);
>>  
>> -    if (priv->res_count == 0 &&
>> -        priv->hasRWDisks &&
>> -        driver->requireLeaseForDisks) {
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                       _("Read/write, exclusive access, disks were present, but no leases specified"));
>> -        return -1;
>> +    if (priv->res_count == 0) {
>> +        if (priv->hasRWDisks &&
>> +            driver->requireLeaseForDisks) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                           _("Read/write, exclusive access, disks were present, but no leases specified"));
>> +            return -1;
>> +        }
>> +
>> +        /* We are not handling METADATA flag yet. So no resources
>> +         * case is no-op for now. */
>> +        return 0;
> 
> Similar comment to patch4 w/r/t resource type (e.g. disk or lease), but
> now at least it's more obvious to me that hasRWDisks means lock for disk
> vs. not.
> 
> Still it's odd to think that returning 0, but not actually getting the
> lock is the "right" thing to do. "Theoretically", if AddResource failed,
> then would Acquire ever be called w/ this flag?

Sure it would. Look at virDomainLockProcessStart() for instance. It
calls virDomainLockManagerNew() which adds all the resources via
AddResource() and then calls Acquire().

> 
>>      }
>>  
>>      /* We only initialize 'sock' if we are in the real
>>
> 
> BTW:
> 
> Now that I think about them together...
> 
>   - lock_driver_lockd.h - extract from patch4 and merge to patch3. I
> suppose it could be separate too to keep mgmt vs. space separate, but
> they're related enough to keep them together. Perhaps this is where a
> few more "words" about usage expectations as part of the commit message.>
>   - lock_daemon_dispatch.c - extract from patch4 and whether it goes
> before or after is one of those chicken/egg type quandaries. Keeping it
> with the fcntl/lockd as opposed to the sanlock implementation doesn't
> feel right (even though sanlock doesn't use any LOCK_SPACE symbols).

I rather keep the patches as they are now. It makes more sense to me
this way.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Prívozník 6 years, 3 months ago

On 08/20/2018 09:25 AM, Michal Prívozník wrote:
> On 08/17/2018 04:49 PM, 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?
> 
> Okay, I'll provide some implementation. Even though sanlock is not
> really my cup of coffee :-)

Actually, sanlock is doomed. It allows us to set the initial offset
(with posing some weird restrictions on it) and it doesn't allow us to
specify length we want to lock (it looks like it locks whole disk
sector). Therefore we can't lock offsets 0 and 1 independently. D'oh!
Maybe we could lock offsets 0 and 1MB but I'm unsure how sanlock behaves
when offset is outside of the file (we can't safely assume that every
file we will try to lock is at least 1MB big, can we? No we can't!
OVMF_VARS is only 128KB long).

https://linux.die.net/man/8/sanlock   and scroll down to offsets.

Therefore I rather stick with my dummy implementation and have somebody
else look at this. Somebody who understands how sanlock works.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrangé 6 years, 3 months ago

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


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

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

Posted by Daniel P. Berrangé 6 years, 3 months ago

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

Oh and even for virtlockd we need to consider the config separately
for content vs metdata.

For content we have the possibility to lock out of band files as a
proxy for the main file. This is primarily done for protecting
shared blocks devices as locking the device node doesn't apply
across hosts.

For metadata locking, we only ever care about the local device node
as changes to labelling/ownership don't propagate across hosts. IOW,
we should always directly lock the file in question for metadata
locking, and never use an out of band proxy for locking.

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

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

Posted by Michal Prívozník 6 years, 3 months ago

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

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

Anyway, I'm gonna state the obvious because it might not be that obvious
to somebody else: we can change the RPC between libvirtd and virtlockd
safely, because we require both to be the same version. I'm saying that
just in case I need to alter the RPC a bit (found some unused procs
there, btw but they might come handy someday).

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

> 
> For content we have the possibility to lock out of band files as a
> proxy for the main file. This is primarily done for protecting
> shared blocks devices as locking the device node doesn't apply
> across hosts.
> 
> For metadata locking, we only ever care about the local device node
> as changes to labelling/ownership don't propagate across hosts. IOW,
> we should always directly lock the file in question for metadata
> locking, and never use an out of band proxy for locking.

Oh right. From this POV locking from secdriver seems like even better
idea.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrangé 6 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/7] virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource
[libvirt] [PATCH v2 2/7] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_WAIT
[libvirt] [PATCH v2 3/7] lock_driver.h: Introduce metadata flag
[libvirt] [PATCH v2 4/7] lockd_driver_lockd: Implement metadata flag
[libvirt] [PATCH v2 5/7] lock_driver_sanlock: Handle metadata flag gracefully
[libvirt] [PATCH v2 6/7] domain_lock: Implement metadata locking
[libvirt] [PATCH v2 7/7] qemu_security: Lock metadata while relabelling