locks: prevent deletion of locked entries

To keep consistency inside transactions started by locking an entry or
an inode, this change delays the removal of entries that are currently
locked by one or more clients. Once all locks are released, the removal
is processed.

It has also been improved the detection of stale inodes in the locking
code of EC.

Fixes: #990
Change-Id: Ic8ba23d9480f80c7f74e7a310bf8a15922320fd5
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

1 files changed, 87 insertions, 63 deletions

diff --git a/xlators/features/locks/src/inodelk.c b/xlators/features/locks/src/inodelk.c
index e0e3b8f1f2d..d4a24eb44be 100644
--- a/xlators/features/locks/src/inodelk.c
+++ b/xlators/features/locks/src/inodelk.c
@@ -229,9 +229,9 @@ out:
     return revoke_lock;
 }
 
-static gf_boolean_t
-__inodelk_needs_contention_notify(xlator_t *this, pl_inode_lock_t *lock,
-                                  struct timespec *now)
+void
+inodelk_contention_notify_check(xlator_t *this, pl_inode_lock_t *lock,
+                                struct timespec *now, struct list_head *contend)
 {
     posix_locks_private_t *priv;
     int64_t elapsed;
@@ -241,7 +241,7 @@ __inodelk_needs_contention_notify(xlator_t *this, pl_inode_lock_t *lock,
     /* If this lock is in a list, it means that we are about to send a
      * notification for it, so no need to do anything else. */
     if (!list_empty(&lock->contend)) {
-        return _gf_false;
+        return;
     }
 
     elapsed = now->tv_sec;
@@ -250,7 +250,7 @@ __inodelk_needs_contention_notify(xlator_t *this, pl_inode_lock_t *lock,
         elapsed--;
     }
     if (elapsed < priv->notify_contention_delay) {
-        return _gf_false;
+        return;
     }
 
     /* All contention notifications will be sent outside of the locked
@@ -263,7 +263,7 @@ __inodelk_needs_contention_notify(xlator_t *this, pl_inode_lock_t *lock,
 
     lock->contention_time = *now;
 
-    return _gf_true;
+    list_add_tail(&lock->contend, contend);
 }
 
 void
@@ -351,9 +351,7 @@ __inodelk_grantable(xlator_t *this, pl_dom_list_t *dom, pl_inode_lock_t *lock,
                     break;
                 }
             }
-            if (__inodelk_needs_contention_notify(this, l, now)) {
-                list_add_tail(&l->contend, contend);
-            }
+            inodelk_contention_notify_check(this, l, now, contend);
         }
     }
 
@@ -433,12 +431,17 @@ __lock_inodelk(xlator_t *this, pl_inode_t *pl_inode, pl_inode_lock_t *lock,
                struct list_head *contend)
 {
     pl_inode_lock_t *conf = NULL;
-    int ret = -EINVAL;
+    int ret;
 
-    conf = __inodelk_grantable(this, dom, lock, now, contend);
-    if (conf) {
-        ret = __lock_blocked_add(this, dom, lock, can_block);
-        goto out;
+    ret = pl_inode_remove_inodelk(pl_inode, lock);
+    if (ret < 0) {
+        return ret;
+    }
+    if (ret == 0) {
+        conf = __inodelk_grantable(this, dom, lock, now, contend);
+    }
+    if ((ret > 0) || (conf != NULL)) {
+        return __lock_blocked_add(this, dom, lock, can_block);
     }
 
     /* To prevent blocked locks starvation, check if there are any blocked
@@ -460,17 +463,13 @@ __lock_inodelk(xlator_t *this, pl_inode_t *pl_inode, pl_inode_lock_t *lock,
                    "starvation");
         }
 
-        ret = __lock_blocked_add(this, dom, lock, can_block);
-        goto out;
+        return __lock_blocked_add(this, dom, lock, can_block);
     }
     __pl_inodelk_ref(lock);
     gettimeofday(&lock->granted_time, NULL);
     list_add(&lock->list, &dom->inodelk_list);
 
-    ret = 0;
-
-out:
-    return ret;
+    return 0;
 }
 
 /* Return true if the two inodelks have exactly same lock boundaries */
@@ -527,12 +526,11 @@ out:
     return conf;
 }
 
-static void
+void
 __grant_blocked_inode_locks(xlator_t *this, pl_inode_t *pl_inode,
                             struct list_head *granted, pl_dom_list_t *dom,
                             struct timespec *now, struct list_head *contend)
 {
-    int bl_ret = 0;
     pl_inode_lock_t *bl = NULL;
     pl_inode_lock_t *tmp = NULL;
 
@@ -545,52 +543,48 @@ __grant_blocked_inode_locks(xlator_t *this, pl_inode_t *pl_inode,
     {
         list_del_init(&bl->blocked_locks);
 
-        bl_ret = __lock_inodelk(this, pl_inode, bl, 1, dom, now, contend);
+        bl->status = __lock_inodelk(this, pl_inode, bl, 1, dom, now, contend);
 
-        if (bl_ret == 0) {
-            list_add(&bl->blocked_locks, granted);
+        if (bl->status != -EAGAIN) {
+            list_add_tail(&bl->blocked_locks, granted);
         }
     }
-    return;
 }
 
-/* Grant all inodelks blocked on a lock */
 void
-grant_blocked_inode_locks(xlator_t *this, pl_inode_t *pl_inode,
-                          pl_dom_list_t *dom, struct timespec *now,
-                          struct list_head *contend)
+unwind_granted_inodes(xlator_t *this, pl_inode_t *pl_inode,
+                      struct list_head *granted)
 {
-    struct list_head granted;
     pl_inode_lock_t *lock;
     pl_inode_lock_t *tmp;
+    int32_t op_ret;
+    int32_t op_errno;
 
-    INIT_LIST_HEAD(&granted);
-
-    pthread_mutex_lock(&pl_inode->mutex);
-    {
-        __grant_blocked_inode_locks(this, pl_inode, &granted, dom, now,
-                                    contend);
-    }
-    pthread_mutex_unlock(&pl_inode->mutex);
-
-    list_for_each_entry_safe(lock, tmp, &granted, blocked_locks)
+    list_for_each_entry_safe(lock, tmp, granted, blocked_locks)
     {
-        gf_log(this->name, GF_LOG_TRACE,
-               "%s (pid=%d) (lk-owner=%s) %" PRId64 " - %" PRId64 " => Granted",
-               lock->fl_type == F_UNLCK ? "Unlock" : "Lock", lock->client_pid,
-               lkowner_utoa(&lock->owner), lock->user_flock.l_start,
-               lock->user_flock.l_len);
-
+        if (lock->status == 0) {
+            op_ret = 0;
+            op_errno = 0;
+            gf_log(this->name, GF_LOG_TRACE,
+                   "%s (pid=%d) (lk-owner=%s) %" PRId64 " - %" PRId64
+                   " => Granted",
+                   lock->fl_type == F_UNLCK ? "Unlock" : "Lock",
+                   lock->client_pid, lkowner_utoa(&lock->owner),
+                   lock->user_flock.l_start, lock->user_flock.l_len);
+        } else {
+            op_ret = -1;
+            op_errno = -lock->status;
+        }
         pl_trace_out(this, lock->frame, NULL, NULL, F_SETLKW, &lock->user_flock,
-                     0, 0, lock->volume);
+                     op_ret, op_errno, lock->volume);
 
-        STACK_UNWIND_STRICT(inodelk, lock->frame, 0, 0, NULL);
+        STACK_UNWIND_STRICT(inodelk, lock->frame, op_ret, op_errno, NULL);
         lock->frame = NULL;
     }
 
     pthread_mutex_lock(&pl_inode->mutex);
     {
-        list_for_each_entry_safe(lock, tmp, &granted, blocked_locks)
+        list_for_each_entry_safe(lock, tmp, granted, blocked_locks)
         {
             list_del_init(&lock->blocked_locks);
             __pl_inodelk_unref(lock);
@@ -599,6 +593,26 @@ grant_blocked_inode_locks(xlator_t *this, pl_inode_t *pl_inode,
     pthread_mutex_unlock(&pl_inode->mutex);
 }
 
+/* Grant all inodelks blocked on a lock */
+void
+grant_blocked_inode_locks(xlator_t *this, pl_inode_t *pl_inode,
+                          pl_dom_list_t *dom, struct timespec *now,
+                          struct list_head *contend)
+{
+    struct list_head granted;
+
+    INIT_LIST_HEAD(&granted);
+
+    pthread_mutex_lock(&pl_inode->mutex);
+    {
+        __grant_blocked_inode_locks(this, pl_inode, &granted, dom, now,
+                                    contend);
+    }
+    pthread_mutex_unlock(&pl_inode->mutex);
+
+    unwind_granted_inodes(this, pl_inode, &granted);
+}
+
 static void
 pl_inodelk_log_cleanup(pl_inode_lock_t *lock)
 {
@@ -660,7 +674,7 @@ pl_inodelk_client_cleanup(xlator_t *this, pl_ctx_t *ctx)
                  * and blocked lists, then this means that a parallel
                  * unlock on another inodelk (L2 say) may have 'granted'
                  * L1 and added it to 'granted' list in
-                 * __grant_blocked_node_locks() (although using the
+                 * __grant_blocked_inode_locks() (although using the
                  * 'blocked_locks' member). In that case, the cleanup
                  * codepath must try and grant other overlapping
                  * blocked inodelks from other clients, now that L1 is
@@ -745,6 +759,7 @@ pl_inode_setlk(xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
     gf_boolean_t need_inode_unref = _gf_false;
     struct list_head *pcontend = NULL;
     struct list_head contend;
+    struct list_head wake;
     struct timespec now = {};
     short fl_type;
 
@@ -796,6 +811,8 @@ pl_inode_setlk(xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
         timespec_now(&now);
     }
 
+    INIT_LIST_HEAD(&wake);
+
     if (ctx)
         pthread_mutex_lock(&ctx->lock);
     pthread_mutex_lock(&pl_inode->mutex);
@@ -818,18 +835,17 @@ pl_inode_setlk(xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
                        lock->fl_type == F_UNLCK ? "Unlock" : "Lock",
                        lock->client_pid, lkowner_utoa(&lock->owner),
                        lock->user_flock.l_start, lock->user_flock.l_len);
-                if (can_block)
+                if (can_block) {
                     unref = _gf_false;
-                /* For all but the case where a non-blocking
-                 * lock attempt fails, the extra ref taken at
-                 * the start of this function must be negated.
-                 */
-                else
-                    need_inode_unref = _gf_true;
+                }
             }
-
-            if (ctx && (!ret || can_block))
+            /* For all but the case where a non-blocking lock attempt fails
+             * with -EAGAIN, the extra ref taken at the start of this function
+             * must be negated. */
+            need_inode_unref = (ret != 0) && ((ret != -EAGAIN) || !can_block);
+            if (ctx && !need_inode_unref) {
                 list_add_tail(&lock->client_list, &ctx->inodelk_lockers);
+            }
         } else {
             /* Irrespective of whether unlock succeeds or not,
              * the extra inode ref that was done at the start of
@@ -847,6 +863,8 @@ pl_inode_setlk(xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
             list_del_init(&retlock->client_list);
             __pl_inodelk_unref(retlock);
 
+            pl_inode_remove_unlocked(this, pl_inode, &wake);
+
             ret = 0;
         }
     out:
@@ -857,6 +875,8 @@ pl_inode_setlk(xlator_t *this, pl_ctx_t *ctx, pl_inode_t *pl_inode,
     if (ctx)
         pthread_mutex_unlock(&ctx->lock);
 
+    pl_inode_remove_wake(&wake);
+
     /* The following (extra) unref corresponds to the ref that
      * was done at the time the lock was granted.
      */
@@ -1037,10 +1057,14 @@ pl_common_inodelk(call_frame_t *frame, xlator_t *this, const char *volume,
                                  inode);
 
             if (ret < 0) {
-                if ((can_block) && (F_UNLCK != lock_type)) {
-                    goto out;
+                if (ret == -EAGAIN) {
+                    if (can_block && (F_UNLCK != lock_type)) {
+                        goto out;
+                    }
+                    gf_log(this->name, GF_LOG_TRACE, "returning EAGAIN");
+                } else {
+                    gf_log(this->name, GF_LOG_TRACE, "returning %d", ret);
                 }
-                gf_log(this->name, GF_LOG_TRACE, "returning EAGAIN");
                 op_errno = -ret;
                 goto unwind;
             }