[v1] Introduce metadata locking

[libvirt] [PATCH v1 1/6] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_METADATA flag

Posted by Michal Privoznik 7 years, 6 months ago

This flag is going to be used to alter default behaviour of the
lock. Firstly, it means we will lock different offset in the file
(offset 1 instead of 0). Secondly, it means the lock acquiring
will actually wait for the lock to be set (compared to default
behaviour which is set or error out).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virlockspace.c | 40 +++++++++++++++++++++++++++++++++-------
 src/util/virlockspace.h |  1 +
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index 3364c843aa..a7ec0c05bf 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -111,6 +111,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res)
     VIR_FREE(res);
 }
 
+#define DEFAULT_OFFSET 0
+#define METADATA_OFFSET 1
 
 static virLockSpaceResourcePtr
 virLockSpaceResourceNew(virLockSpacePtr lockspace,
@@ -120,6 +122,18 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
 {
     virLockSpaceResourcePtr res;
     bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED);
+    bool metadata = !!(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA);
+    off_t start = DEFAULT_OFFSET;
+    bool waitForLock = false;
+
+    if (metadata) {
+        /* We want the metadata lock to act like pthread mutex.
+         * This means waiting for the lock to be acquired. */
+        waitForLock = true;
+
+        /* Also, we are locking different offset. */
+        start = METADATA_OFFSET;
+    }
 
     if (VIR_ALLOC(res) < 0)
         return NULL;
@@ -157,7 +171,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
                 goto error;
             }
 
-            if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
+            if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) {
                 if (errno == EACCES || errno == EAGAIN) {
                     virReportError(VIR_ERR_RESOURCE_BUSY,
                                    _("Lockspace resource '%s' is locked"),
@@ -204,7 +218,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
             goto error;
         }
 
-        if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
+        if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) {
             if (errno == EACCES || errno == EAGAIN) {
                 virReportError(VIR_ERR_RESOURCE_BUSY,
                                _("Lockspace resource '%s' is locked"),
@@ -621,7 +635,15 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
               lockspace, resname, flags, (unsigned long long)owner);
 
     virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED |
-                  VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1);
+                  VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE |
+                  VIR_LOCK_SPACE_ACQUIRE_METADATA, -1);
+
+    if (flags & VIR_LOCK_SPACE_ACQUIRE_METADATA &&
+        flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) {
+        virReportInvalidArg(flags, "%s",
+                            _("metadata and shared are mutually exclusive"));
+        return -1;
+    }
 
     virMutexLock(&lockspace->lock);
 
@@ -635,10 +657,14 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
 
             goto done;
         }
-        virReportError(VIR_ERR_RESOURCE_BUSY,
-                       _("Lockspace resource '%s' is locked"),
-                       resname);
-        goto cleanup;
+
+        if (!(res->flags & VIR_LOCK_SPACE_ACQUIRE_METADATA) ||
+            !(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA)) {
+            virReportError(VIR_ERR_RESOURCE_BUSY,
+                           _("Lockspace resource '%s' is locked"),
+                           resname);
+            goto cleanup;
+        }
     }
 
     if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner)))
diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h
index 041cf20396..a9a3b2e73f 100644
--- a/src/util/virlockspace.h
+++ b/src/util/virlockspace.h
@@ -45,6 +45,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
 typedef enum {
     VIR_LOCK_SPACE_ACQUIRE_SHARED     = (1 << 0),
     VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1),
+    VIR_LOCK_SPACE_ACQUIRE_METADATA   = (1 << 2),
 } virLockSpaceAcquireFlags;
 
 int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
-- 
2.16.4

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

Re: [libvirt] [PATCH v1 1/6] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_METADATA flag

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

On Thu, Aug 09, 2018 at 03:34:39PM +0200, Michal Privoznik wrote:
> This flag is going to be used to alter default behaviour of the
> lock. Firstly, it means we will lock different offset in the file
> (offset 1 instead of 0). Secondly, it means the lock acquiring
> will actually wait for the lock to be set (compared to default
> behaviour which is set or error out).
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virlockspace.c | 40 +++++++++++++++++++++++++++++++++-------
>  src/util/virlockspace.h |  1 +
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
> index 3364c843aa..a7ec0c05bf 100644
> --- a/src/util/virlockspace.c
> +++ b/src/util/virlockspace.c
> @@ -111,6 +111,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res)
>      VIR_FREE(res);
>  }
>  
> +#define DEFAULT_OFFSET 0
> +#define METADATA_OFFSET 1
>  
>  static virLockSpaceResourcePtr
>  virLockSpaceResourceNew(virLockSpacePtr lockspace,
> @@ -120,6 +122,18 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
>  {
>      virLockSpaceResourcePtr res;
>      bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED);
> +    bool metadata = !!(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA);
> +    off_t start = DEFAULT_OFFSET;
> +    bool waitForLock = false;
> +
> +    if (metadata) {
> +        /* We want the metadata lock to act like pthread mutex.
> +         * This means waiting for the lock to be acquired. */
> +        waitForLock = true;
> +
> +        /* Also, we are locking different offset. */
> +        start = METADATA_OFFSET;
> +    }
>  
>      if (VIR_ALLOC(res) < 0)
>          return NULL;
> @@ -157,7 +171,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
>                  goto error;
>              }
>  
> -            if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
> +            if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) {
>                  if (errno == EACCES || errno == EAGAIN) {
>                      virReportError(VIR_ERR_RESOURCE_BUSY,
>                                     _("Lockspace resource '%s' is locked"),
> @@ -204,7 +218,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
>              goto error;
>          }
>  
> -        if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
> +        if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) {
>              if (errno == EACCES || errno == EAGAIN) {
>                  virReportError(VIR_ERR_RESOURCE_BUSY,
>                                 _("Lockspace resource '%s' is locked"),
> @@ -621,7 +635,15 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
>                lockspace, resname, flags, (unsigned long long)owner);
>  
>      virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED |
> -                  VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1);
> +                  VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE |
> +                  VIR_LOCK_SPACE_ACQUIRE_METADATA, -1);
> +
> +    if (flags & VIR_LOCK_SPACE_ACQUIRE_METADATA &&
> +        flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) {
> +        virReportInvalidArg(flags, "%s",
> +                            _("metadata and shared are mutually exclusive"));
> +        return -1;
> +    }
>  
>      virMutexLock(&lockspace->lock);
>  
> @@ -635,10 +657,14 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
>  
>              goto done;
>          }
> -        virReportError(VIR_ERR_RESOURCE_BUSY,
> -                       _("Lockspace resource '%s' is locked"),
> -                       resname);
> -        goto cleanup;
> +
> +        if (!(res->flags & VIR_LOCK_SPACE_ACQUIRE_METADATA) ||
> +            !(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA)) {
> +            virReportError(VIR_ERR_RESOURCE_BUSY,
> +                           _("Lockspace resource '%s' is locked"),
> +                           resname);
> +            goto cleanup;
> +        }
>      }
>  
>      if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner)))
> diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h
> index 041cf20396..a9a3b2e73f 100644
> --- a/src/util/virlockspace.h
> +++ b/src/util/virlockspace.h
> @@ -45,6 +45,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
>  typedef enum {
>      VIR_LOCK_SPACE_ACQUIRE_SHARED     = (1 << 0),
>      VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1),
> +    VIR_LOCK_SPACE_ACQUIRE_METADATA   = (1 << 2),
>  } virLockSpaceAcquireFlags;
>  
>  int virLockSpaceAcquireResource(virLockSpacePtr lockspace,

The virLockSpace code is intended to be a bit more generic than the
locking protocol.

So I'd have a preference for having a VIR_LOCK_SPACE_ACQUIRE_WAIT
flag, and passing in the offset + length to the APIs as parameters.

Just have the "METADATA" concept in the lock driver only, not this
general purpose code.


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 v1 1/6] virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_METADATA flag
[libvirt] [PATCH v1 2/6] lock_driver.h: Introduce metadata flag
[libvirt] [PATCH v1 3/6] lockd_driver_lockd: Implement metadata flag
[libvirt] [PATCH v1 4/6] lock_driver_sanlock: Handle metadata flag gracefully
[libvirt] [PATCH v1 5/6] domain_lock: Implement metadata locking
[libvirt] [PATCH v1 6/6] qemu_security: Lock metadata while relabelling