From ea7f11b989896d76b8d091d26bc0241bce9413f8 Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@redhat.com>

Date: Thu, 4 Jul 2019 13:21:33 +0200

Subject: [PATCH 253/255] core: fix deadlock between statedump and

 fd_anonymous()

There exists a deadlock between statedump generation and fd_anonymous()

function because they are acquiring inode table lock and inode lock in

reverse order.

This patch modifies fd_anonymous() so that it takes inode lock only when

it's really necessary, avoiding the deadlock.

Upstream patch:

> Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7

> Upstream patch link: https://review.gluster.org/c/glusterfs/+/22995

> BUG: 1727068

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

Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7

Fixes: bz#1722209

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

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

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

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 libglusterfs/src/fd.c | 137 ++++++++++++++++++++++----------------------------

 1 file changed, 61 insertions(+), 76 deletions(-)

diff --git a/libglusterfs/src/fd.c b/libglusterfs/src/fd.c

index b8aac72..314546a 100644

--- a/libglusterfs/src/fd.c

+++ b/libglusterfs/src/fd.c

@@ -532,7 +532,7 @@ fd_unref(fd_t *fd)

     return;

 }

-fd_t *

+static fd_t *

 __fd_bind(fd_t *fd)

 {

     list_del_init(&fd->inode_list);

@@ -562,9 +562,9 @@ fd_bind(fd_t *fd)

 }

 static fd_t *

-__fd_create(inode_t *inode, uint64_t pid)

+fd_allocate(inode_t *inode, uint64_t pid)

 {

-    fd_t *fd = NULL;

+    fd_t *fd;

     if (inode == NULL) {

         gf_msg_callingfn("fd", GF_LOG_ERROR, EINVAL, LG_MSG_INVALID_ARG,

@@ -573,64 +573,67 @@ __fd_create(inode_t *inode, uint64_t pid)

     }

     fd = mem_get0(inode->table->fd_mem_pool);

-    if (!fd)

-        goto out;

+    if (fd == NULL) {

+        return NULL;

+    }

     fd->xl_count = inode->table->xl->graph->xl_count + 1;

     fd->_ctx = GF_CALLOC(1, (sizeof(struct _fd_ctx) * fd->xl_count),

                          gf_common_mt_fd_ctx);

-    if (!fd->_ctx)

-        goto free_fd;

+    if (fd->_ctx == NULL) {

+        goto failed;

+    }

     fd->lk_ctx = fd_lk_ctx_create();

-    if (!fd->lk_ctx)

-        goto free_fd_ctx;

-

-    fd->inode = inode_ref(inode);

-    fd->pid = pid;

-    INIT_LIST_HEAD(&fd->inode_list);

-

-    LOCK_INIT(&fd->lock);

-out:

-    return fd;

+    if (fd->lk_ctx != NULL) {

+        /* We need to take a reference from the inode, but we cannot do it

+         * here because this function can be called with the inode lock taken

+         * and inode_ref() takes the inode's table lock. This is the reverse

+         * of the logical lock acquisition order and can cause a deadlock. So

+         * we simply assign the inode here and we delefate the inode reference

+         * responsibility to the caller (when this function succeeds and the

+         * inode lock is released). This is safe because the caller must hold

+         * a reference of the inode to use it, so it's guaranteed that the

+         * number of references won't reach 0 before the caller finishes.

+         *

+         * TODO: minimize use of locks in favor of atomic operations to avoid

+         *       these dependencies. */

+        fd->inode = inode;

+        fd->pid = pid;

+        INIT_LIST_HEAD(&fd->inode_list);

+        LOCK_INIT(&fd->lock);

+        GF_ATOMIC_INIT(fd->refcount, 1);

+        return fd;

+    }

-free_fd_ctx:

     GF_FREE(fd->_ctx);

-free_fd:

+

+failed:

     mem_put(fd);

     return NULL;

 }

 fd_t *

-fd_create(inode_t *inode, pid_t pid)

+fd_create_uint64(inode_t *inode, uint64_t pid)

 {

-    fd_t *fd = NULL;

-

-    fd = __fd_create(inode, (uint64_t)pid);

-    if (!fd)

-        goto out;

+    fd_t *fd;

-    fd = fd_ref(fd);

+    fd = fd_allocate(inode, pid);

+    if (fd != NULL) {

+        /* fd_allocate() doesn't get a reference from the inode. We need to

+         * take it here in case of success. */

+        inode_ref(inode);

+    }

-out:

     return fd;

 }

 fd_t *

-fd_create_uint64(inode_t *inode, uint64_t pid)

+fd_create(inode_t *inode, pid_t pid)

 {

-    fd_t *fd = NULL;

-

-    fd = __fd_create(inode, pid);

-    if (!fd)

-        goto out;

-

-    fd = fd_ref(fd);

-

-out:

-    return fd;

+    return fd_create_uint64(inode, (uint64_t)pid);

 }

 static fd_t *

@@ -719,10 +722,13 @@ __fd_lookup_anonymous(inode_t *inode, int32_t flags)

     return fd;

 }

-static fd_t *

-__fd_anonymous(inode_t *inode, int32_t flags)

+fd_t *

+fd_anonymous_with_flags(inode_t *inode, int32_t flags)

 {

     fd_t *fd = NULL;

+    bool ref = false;

+

+    LOCK(&inode->lock);

     fd = __fd_lookup_anonymous(inode, flags);

@@ -730,54 +736,33 @@ __fd_anonymous(inode_t *inode, int32_t flags)

        __fd_lookup_anonymous(), so no need of one more fd_ref().

        if (!fd); then both create and bind won't bump up the ref

        count, so we have to call fd_ref() after bind. */

-    if (!fd) {

-        fd = __fd_create(inode, 0);

-

-        if (!fd)

-            return NULL;

-

-        fd->anonymous = _gf_true;

-        fd->flags = GF_ANON_FD_FLAGS | flags;

+    if (fd == NULL) {

+        fd = fd_allocate(inode, 0);

+        if (fd != NULL) {

+            fd->anonymous = _gf_true;

+            fd->flags = GF_ANON_FD_FLAGS | (flags & O_DIRECT);

-        __fd_bind(fd);

+            __fd_bind(fd);

-        __fd_ref(fd);

+            ref = true;

+        }

     }

-    return fd;

-}

-

-fd_t *

-fd_anonymous(inode_t *inode)

-{

-    fd_t *fd = NULL;

+    UNLOCK(&inode->lock);

-    LOCK(&inode->lock);

-    {

-        fd = __fd_anonymous(inode, GF_ANON_FD_FLAGS);

+    if (ref) {

+        /* fd_allocate() doesn't get a reference from the inode. We need to

+         * take it here in case of success. */

+        inode_ref(inode);

     }

-    UNLOCK(&inode->lock);

     return fd;

 }

 fd_t *

-fd_anonymous_with_flags(inode_t *inode, int32_t flags)

+fd_anonymous(inode_t *inode)

 {

-    fd_t *fd = NULL;

-

-    if (flags & O_DIRECT)

-        flags = GF_ANON_FD_FLAGS | O_DIRECT;

-    else

-        flags = GF_ANON_FD_FLAGS;

-

-    LOCK(&inode->lock);

-    {

-        fd = __fd_anonymous(inode, flags);

-    }

-    UNLOCK(&inode->lock);

-

-    return fd;

+    return fd_anonymous_with_flags(inode, 0);

 }

 fd_t *

-- 

1.8.3.1