From 3f6ff474db3934f43d9963dfe4dda7d201211e75 Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@redhat.com>

Date: Fri, 12 Jun 2020 00:06:36 +0200

Subject: [PATCH 455/456] 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.

>Upstream patch - https://review.gluster.org/#/c/glusterfs/+/20025/

>Fixes: #990

Change-Id: Ic8ba23d9480f80c7f74e7a310bf8a15922320fd5

BUG: 1812789

Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/206442

Tested-by: RHGS Build Bot <nigelb@redhat.com>

---

 xlators/cluster/ec/src/ec-locks.c    |  69 ++++++--

 xlators/features/locks/src/common.c  | 316 ++++++++++++++++++++++++++++++++++-

 xlators/features/locks/src/common.h  |  43 +++++

 xlators/features/locks/src/entrylk.c |  19 +--

 xlators/features/locks/src/inodelk.c | 150 ++++++++++-------

 xlators/features/locks/src/locks.h   |  23 ++-

 xlators/features/locks/src/posix.c   | 183 ++++++++++++++++++--

 7 files changed, 689 insertions(+), 114 deletions(-)

diff --git a/xlators/cluster/ec/src/ec-locks.c b/xlators/cluster/ec/src/ec-locks.c

index ffcac07..db86296 100644

--- a/xlators/cluster/ec/src/ec-locks.c

+++ b/xlators/cluster/ec/src/ec-locks.c

@@ -28,9 +28,36 @@ ec_lock_check(ec_fop_data_t *fop, uintptr_t *mask)

     ec_t *ec = fop->xl->private;

     ec_cbk_data_t *ans = NULL;

     ec_cbk_data_t *cbk = NULL;

-    uintptr_t locked = 0, notlocked = 0;

+    uintptr_t locked = 0;

+    int32_t good = 0;

+    int32_t eagain = 0;

+    int32_t estale = 0;

     int32_t error = -1;

+    /* There are some errors that we'll handle in an special way while trying

+     * to acquire a lock.

+     *

+     *   EAGAIN:  If it's found during a parallel non-blocking lock request, we

+     *            consider that there's contention on the inode, so we consider

+     *            the acquisition a failure and try again with a sequential

+     *            blocking lock request. This will ensure that we get a lock on

+     *            as many bricks as possible (ignoring EAGAIN here would cause

+     *            unnecessary triggers of self-healing).

+     *

+     *            If it's found during a sequential blocking lock request, it's

+     *            considered an error. Lock will only succeed if there are

+     *            enough other bricks locked.

+     *

+     *   ESTALE:  This can appear during parallel or sequential lock request if

+     *            the inode has just been unlinked. We consider this error is

+     *            not recoverable, but we also don't consider it as fatal. So,

+     *            if it happens during parallel lock, we won't attempt a

+     *            sequential one unless there are EAGAIN errors on other

+     *            bricks (and are enough to form a quorum), but if we reach

+     *            quorum counting the ESTALE bricks, we consider the whole

+     *            result of the operation is ESTALE instead of EIO.

+     */

+

     list_for_each_entry(ans, &fop->cbk_list, list)

     {

         if (ans->op_ret >= 0) {

@@ -38,24 +65,23 @@ ec_lock_check(ec_fop_data_t *fop, uintptr_t *mask)

                 error = EIO;

             }

             locked |= ans->mask;

+            good = ans->count;

             cbk = ans;

-        } else {

-            if (ans->op_errno == EAGAIN) {

-                switch (fop->uint32) {

-                    case EC_LOCK_MODE_NONE:

-                    case EC_LOCK_MODE_ALL:

-                        /* Goal is to treat non-blocking lock as failure

-                         * even if there is a single EAGAIN*/

-                        notlocked |= ans->mask;

-                        break;

-                }

-            }

+        } else if (ans->op_errno == ESTALE) {

+            estale += ans->count;

+        } else if ((ans->op_errno == EAGAIN) &&

+                   (fop->uint32 != EC_LOCK_MODE_INC)) {

+            eagain += ans->count;

         }

     }

     if (error == -1) {

-        if (gf_bits_count(locked | notlocked) >= ec->fragments) {

-            if (notlocked == 0) {

+        /* If we have enough quorum with succeeded and EAGAIN answers, we

+         * ignore for now any ESTALE answer. If there are EAGAIN answers,

+         * we retry with a sequential blocking lock request if needed.

+         * Otherwise we succeed. */

+        if ((good + eagain) >= ec->fragments) {

+            if (eagain == 0) {

                 if (fop->answer == NULL) {

                     fop->answer = cbk;

                 }

@@ -68,21 +94,28 @@ ec_lock_check(ec_fop_data_t *fop, uintptr_t *mask)

                     case EC_LOCK_MODE_NONE:

                         error = EAGAIN;

                         break;

-

                     case EC_LOCK_MODE_ALL:

                         fop->uint32 = EC_LOCK_MODE_INC;

                         break;

-

                     default:

+                        /* This shouldn't happen because eagain cannot be > 0

+                         * when fop->uint32 is EC_LOCK_MODE_INC. */

                         error = EIO;

                         break;

                 }

             }

         } else {

-            if (fop->answer && fop->answer->op_ret < 0)

+            /* We have been unable to find enough candidates that will be able

+             * to take the lock. If we have quorum on some answer, we return

+             * it. Otherwise we check if ESTALE answers allow us to reach

+             * quorum. If so, we return ESTALE. */

+            if (fop->answer && fop->answer->op_ret < 0) {

                 error = fop->answer->op_errno;

-            else

+            } else if ((good + eagain + estale) >= ec->fragments) {

+                error = ESTALE;

+            } else {

                 error = EIO;

+            }

         }

     }

diff --git a/xlators/features/locks/src/common.c b/xlators/features/locks/src/common.c

index 1406e70..0c52853 100644

--- a/xlators/features/locks/src/common.c

+++ b/xlators/features/locks/src/common.c

@@ -462,11 +462,16 @@ pl_inode_get(xlator_t *this, inode_t *inode, pl_local_t *local)

         INIT_LIST_HEAD(&pl_inode->blocked_calls);

         INIT_LIST_HEAD(&pl_inode->metalk_list);

         INIT_LIST_HEAD(&pl_inode->queued_locks);

+        INIT_LIST_HEAD(&pl_inode->waiting);

         gf_uuid_copy(pl_inode->gfid, inode->gfid);

         pl_inode->check_mlock_info = _gf_true;

         pl_inode->mlock_enforced = _gf_false;

+        /* -2 means never looked up. -1 means something went wrong and link

+         * tracking is disabled. */

+        pl_inode->links = -2;

+

         ret = __inode_ctx_put(inode, this, (uint64_t)(long)(pl_inode));

         if (ret) {

             pthread_mutex_destroy(&pl_inode->mutex);

@@ -1276,4 +1281,313 @@ pl_local_init(call_frame_t *frame, xlator_t *this, loc_t *loc, fd_t *fd)

     }

     return 0;

-}

\ No newline at end of file

+}

+

+gf_boolean_t

+pl_is_lk_owner_valid(gf_lkowner_t *owner, client_t *client)

+{

+    if (client && (client->opversion < GD_OP_VERSION_7_0)) {

+        return _gf_true;

+    }

+

+    if (is_lk_owner_null(owner)) {

+        return _gf_false;

+    }

+    return _gf_true;

+}

+

+static int32_t

+pl_inode_from_loc(loc_t *loc, inode_t **pinode)

+{

+    inode_t *inode = NULL;

+    int32_t error = 0;

+

+    if (loc->inode != NULL) {

+        inode = inode_ref(loc->inode);

+        goto done;

+    }

+

+    if (loc->parent == NULL) {

+        error = EINVAL;

+        goto done;

+    }

+

+    if (!gf_uuid_is_null(loc->gfid)) {

+        inode = inode_find(loc->parent->table, loc->gfid);

+        if (inode != NULL) {

+            goto done;

+        }

+    }

+

+    if (loc->name == NULL) {

+        error = EINVAL;

+        goto done;

+    }

+

+    inode = inode_grep(loc->parent->table, loc->parent, loc->name);

+    if (inode == NULL) {

+        /* We haven't found any inode. This means that the file doesn't exist

+         * or that even if it exists, we don't have any knowledge about it, so

+         * we don't have locks on it either, which is fine for our purposes. */

+        goto done;

+    }

+

+done:

+    *pinode = inode;

+

+    return error;

+}

+

+static gf_boolean_t

+pl_inode_has_owners(xlator_t *xl, client_t *client, pl_inode_t *pl_inode,

+                    struct timespec *now, struct list_head *contend)

+{

+    pl_dom_list_t *dom;

+    pl_inode_lock_t *lock;

+    gf_boolean_t has_owners = _gf_false;

+

+    list_for_each_entry(dom, &pl_inode->dom_list, inode_list)

+    {

+        list_for_each_entry(lock, &dom->inodelk_list, list)

+        {

+            /* If the lock belongs to the same client, we assume it's related

+             * to the same operation, so we allow the removal to continue. */

+            if (lock->client == client) {

+                continue;

+            }

+            /* If the lock belongs to an internal process, we don't block the

+             * removal. */

+            if (lock->client_pid < 0) {

+                continue;

+            }

+            if (contend == NULL) {

+                return _gf_true;

+            }

+            has_owners = _gf_true;

+            inodelk_contention_notify_check(xl, lock, now, contend);

+        }

+    }

+

+    return has_owners;

+}

+

+int32_t

+pl_inode_remove_prepare(xlator_t *xl, call_frame_t *frame, loc_t *loc,

+                        pl_inode_t **ppl_inode, struct list_head *contend)

+{

+    struct timespec now;

+    inode_t *inode;

+    pl_inode_t *pl_inode;

+    int32_t error;

+

+    pl_inode = NULL;

+

+    error = pl_inode_from_loc(loc, &inode;;

+    if ((error != 0) || (inode == NULL)) {

+        goto done;

+    }

+

+    pl_inode = pl_inode_get(xl, inode, NULL);

+    if (pl_inode == NULL) {

+        inode_unref(inode);

+        error = ENOMEM;

+        goto done;

+    }

+

+    /* pl_inode_from_loc() already increments ref count for inode, so

+     * we only assign here our reference. */

+    pl_inode->inode = inode;

+

+    timespec_now(&now;;

+

+    pthread_mutex_lock(&pl_inode->mutex);

+

+    if (pl_inode->removed) {

+        error = ESTALE;

+        goto unlock;

+    }

+

+    if (pl_inode_has_owners(xl, frame->root->client, pl_inode, &now, contend)) {

+        error = -1;

+        /* We skip the unlock here because the caller must create a stub when

+         * we return -1 and do a call to pl_inode_remove_complete(), which

+         * assumes the lock is still acquired and will release it once

+         * everything else is prepared. */

+        goto done;

+    }

+

+    pl_inode->is_locked = _gf_true;

+    pl_inode->remove_running++;

+

+unlock:

+    pthread_mutex_unlock(&pl_inode->mutex);

+

+done:

+    *ppl_inode = pl_inode;

+

+    return error;

+}

+

+int32_t

+pl_inode_remove_complete(xlator_t *xl, pl_inode_t *pl_inode, call_stub_t *stub,

+                         struct list_head *contend)

+{

+    pl_inode_lock_t *lock;

+    int32_t error = -1;

+

+    if (stub != NULL) {

+        list_add_tail(&stub->list, &pl_inode->waiting);

+        pl_inode->is_locked = _gf_true;

+    } else {

+        error = ENOMEM;

+

+        while (!list_empty(contend)) {

+            lock = list_first_entry(contend, pl_inode_lock_t, list);

+            list_del_init(&lock->list);

+            __pl_inodelk_unref(lock);

+        }

+    }

+

+    pthread_mutex_unlock(&pl_inode->mutex);

+

+    if (error < 0) {

+        inodelk_contention_notify(xl, contend);

+    }

+

+    inode_unref(pl_inode->inode);

+

+    return error;

+}

+

+void

+pl_inode_remove_wake(struct list_head *list)

+{

+    call_stub_t *stub;

+

+    while (!list_empty(list)) {

+        stub = list_first_entry(list, call_stub_t, list);

+        list_del_init(&stub->list);

+

+        call_resume(stub);

+    }

+}

+

+void

+pl_inode_remove_cbk(xlator_t *xl, pl_inode_t *pl_inode, int32_t error)

+{

+    struct list_head contend, granted;

+    struct timespec now;

+    pl_dom_list_t *dom;

+

+    if (pl_inode == NULL) {

+        return;

+    }

+

+    INIT_LIST_HEAD(&contend);

+    INIT_LIST_HEAD(&granted);

+    timespec_now(&now;;

+

+    pthread_mutex_lock(&pl_inode->mutex);

+

+    if (error == 0) {

+        if (pl_inode->links >= 0) {

+            pl_inode->links--;

+        }

+        if (pl_inode->links == 0) {

+            pl_inode->removed = _gf_true;

+        }

+    }

+

+    pl_inode->remove_running--;

+

+    if ((pl_inode->remove_running == 0) && list_empty(&pl_inode->waiting)) {

+        pl_inode->is_locked = _gf_false;

+

+        list_for_each_entry(dom, &pl_inode->dom_list, inode_list)

+        {

+            __grant_blocked_inode_locks(xl, pl_inode, &granted, dom, &now,

+                                        &contend);

+        }

+    }

+

+    pthread_mutex_unlock(&pl_inode->mutex);

+

+    unwind_granted_inodes(xl, pl_inode, &granted);

+

+    inodelk_contention_notify(xl, &contend);

+

+    inode_unref(pl_inode->inode);

+}

+

+void

+pl_inode_remove_unlocked(xlator_t *xl, pl_inode_t *pl_inode,

+                         struct list_head *list)

+{

+    call_stub_t *stub, *tmp;

+

+    if (!pl_inode->is_locked) {

+        return;

+    }

+

+    list_for_each_entry_safe(stub, tmp, &pl_inode->waiting, list)

+    {

+        if (!pl_inode_has_owners(xl, stub->frame->root->client, pl_inode, NULL,

+                                 NULL)) {

+            list_move_tail(&stub->list, list);

+        }

+    }

+}

+

+/* This function determines if an inodelk attempt can be done now or it needs

+ * to wait.

+ *

+ * Possible return values:

+ *   < 0: An error occurred. Currently only -ESTALE can be returned if the

+ *        inode has been deleted previously by unlink/rmdir/rename

+ *   = 0: The lock can be attempted.

+ *   > 0: The lock needs to wait because a conflicting remove operation is

+ *        ongoing.

+ */

+int32_t

+pl_inode_remove_inodelk(pl_inode_t *pl_inode, pl_inode_lock_t *lock)

+{

+    pl_dom_list_t *dom;

+    pl_inode_lock_t *ilock;

+

+    /* If the inode has been deleted, we won't allow any lock. */

+    if (pl_inode->removed) {

+        return -ESTALE;

+    }

+

+    /* We only synchronize with locks made for regular operations coming from

+     * the user. Locks done for internal purposes are hard to control and could

+     * lead to long delays or deadlocks quite easily. */

+    if (lock->client_pid < 0) {

+        return 0;

+    }

+    if (!pl_inode->is_locked) {

+        return 0;

+    }

+    if (pl_inode->remove_running > 0) {

+        return 1;

+    }

+

+    list_for_each_entry(dom, &pl_inode->dom_list, inode_list)

+    {

+        list_for_each_entry(ilock, &dom->inodelk_list, list)

+        {

+            /* If a lock from the same client is already granted, we allow this

+             * one to continue. This is necessary to prevent deadlocks when

+             * multiple locks are taken for the same operation.

+             *

+             * On the other side it's unlikely that the same client sends

+             * completely unrelated locks for the same inode.

+             */

+            if (ilock->client == lock->client) {

+                return 0;

+            }

+        }

+    }

+

+    return 1;

+}

diff --git a/xlators/features/locks/src/common.h b/xlators/features/locks/src/common.h

index ea86b96..6c81ac3 100644

--- a/xlators/features/locks/src/common.h

+++ b/xlators/features/locks/src/common.h

@@ -105,6 +105,15 @@ void

 __pl_inodelk_unref(pl_inode_lock_t *lock);

 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);

+

+void

+unwind_granted_inodes(xlator_t *this, pl_inode_t *pl_inode,

+                      struct list_head *granted);

+

+void

 grant_blocked_entry_locks(xlator_t *this, pl_inode_t *pl_inode,

                           pl_dom_list_t *dom, struct timespec *now,

                           struct list_head *contend);

@@ -204,6 +213,16 @@ pl_metalock_is_active(pl_inode_t *pl_inode);

 void

 __pl_queue_lock(pl_inode_t *pl_inode, posix_lock_t *reqlock);

+void

+inodelk_contention_notify_check(xlator_t *xl, pl_inode_lock_t *lock,

+                                struct timespec *now,

+                                struct list_head *contend);

+

+void

+entrylk_contention_notify_check(xlator_t *xl, pl_entry_lock_t *lock,

+                                struct timespec *now,

+                                struct list_head *contend);

+

 gf_boolean_t

 pl_does_monkey_want_stuck_lock();

@@ -216,4 +235,28 @@ pl_clean_local(pl_local_t *local);

 int

 pl_local_init(call_frame_t *frame, xlator_t *this, loc_t *loc, fd_t *fd);

+gf_boolean_t

+pl_is_lk_owner_valid(gf_lkowner_t *owner, client_t *client);

+

+int32_t

+pl_inode_remove_prepare(xlator_t *xl, call_frame_t *frame, loc_t *loc,

+                        pl_inode_t **ppl_inode, struct list_head *contend);

+

+int32_t

+pl_inode_remove_complete(xlator_t *xl, pl_inode_t *pl_inode, call_stub_t *stub,

+                         struct list_head *contend);

+

+void

+pl_inode_remove_wake(struct list_head *list);

+

+void

+pl_inode_remove_cbk(xlator_t *xl, pl_inode_t *pl_inode, int32_t error);

+

+void

+pl_inode_remove_unlocked(xlator_t *xl, pl_inode_t *pl_inode,

+                         struct list_head *list);

+

+int32_t

+pl_inode_remove_inodelk(pl_inode_t *pl_inode, pl_inode_lock_t *lock);

+

 #endif /* __COMMON_H__ */

diff --git a/xlators/features/locks/src/entrylk.c b/xlators/features/locks/src/entrylk.c

index 93c649c..b97836f 100644

--- a/xlators/features/locks/src/entrylk.c

+++ b/xlators/features/locks/src/entrylk.c

@@ -197,9 +197,9 @@ out:

     return revoke_lock;

 }

-static gf_boolean_t

-__entrylk_needs_contention_notify(xlator_t *this, pl_entry_lock_t *lock,

-                                  struct timespec *now)

+void

+entrylk_contention_notify_check(xlator_t *this, pl_entry_lock_t *lock,

+                                struct timespec *now, struct list_head *contend)

 {

     posix_locks_private_t *priv;

     int64_t elapsed;

@@ -209,7 +209,7 @@ __entrylk_needs_contention_notify(xlator_t *this, pl_entry_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;

@@ -218,7 +218,7 @@ __entrylk_needs_contention_notify(xlator_t *this, pl_entry_lock_t *lock,

         elapsed--;

     }

     if (elapsed < priv->notify_contention_delay) {

-        return _gf_false;

+        return;

     }

     /* All contention notifications will be sent outside of the locked

@@ -231,7 +231,7 @@ __entrylk_needs_contention_notify(xlator_t *this, pl_entry_lock_t *lock,

     lock->contention_time = *now;

-    return _gf_true;

+    list_add_tail(&lock->contend, contend);

 }

 void

@@ -325,9 +325,7 @@ __entrylk_grantable(xlator_t *this, pl_dom_list_t *dom, pl_entry_lock_t *lock,

                     break;

                 }

             }

-            if (__entrylk_needs_contention_notify(this, tmp, now)) {

-                list_add_tail(&tmp->contend, contend);

-            }

+            entrylk_contention_notify_check(this, tmp, now, contend);

         }

     }

@@ -690,10 +688,9 @@ __grant_blocked_entry_locks(xlator_t *this, pl_inode_t *pl_inode,

         bl_ret = __lock_entrylk(bl->this, pl_inode, bl, 0, dom, now, contend);

         if (bl_ret == 0) {

-            list_add(&bl->blocked_locks, granted);

+            list_add_tail(&bl->blocked_locks, granted);

         }

     }

-    return;

 }

 /* Grants locks if possible which are blocked on a lock */

diff --git a/xlators/features/locks/src/inodelk.c b/xlators/features/locks/src/inodelk.c

index 24dee49..1a07243 100644

--- a/xlators/features/locks/src/inodelk.c

+++ b/xlators/features/locks/src/inodelk.c

@@ -231,9 +231,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;

@@ -243,7 +243,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;

@@ -252,7 +252,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

@@ -265,7 +265,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

@@ -353,9 +353,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);

         }

     }

@@ -435,12 +433,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

@@ -462,17 +465,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 */

@@ -529,12 +528,11 @@ out:

     return conf;

 }

-static void

+void