50dc83
From ea7f11b989896d76b8d091d26bc0241bce9413f8 Mon Sep 17 00:00:00 2001
50dc83
From: Xavi Hernandez <xhernandez@redhat.com>
50dc83
Date: Thu, 4 Jul 2019 13:21:33 +0200
50dc83
Subject: [PATCH 253/255] core: fix deadlock between statedump and
50dc83
 fd_anonymous()
50dc83
50dc83
There exists a deadlock between statedump generation and fd_anonymous()
50dc83
function because they are acquiring inode table lock and inode lock in
50dc83
reverse order.
50dc83
50dc83
This patch modifies fd_anonymous() so that it takes inode lock only when
50dc83
it's really necessary, avoiding the deadlock.
50dc83
50dc83
Upstream patch:
50dc83
> Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7
50dc83
> Upstream patch link: https://review.gluster.org/c/glusterfs/+/22995
50dc83
> BUG: 1727068
50dc83
> Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>
50dc83
50dc83
Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7
50dc83
Fixes: bz#1722209
50dc83
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>
50dc83
Reviewed-on: https://code.engineering.redhat.com/gerrit/176096
50dc83
Tested-by: RHGS Build Bot <nigelb@redhat.com>
50dc83
Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>
50dc83
---
50dc83
 libglusterfs/src/fd.c | 137 ++++++++++++++++++++++----------------------------
50dc83
 1 file changed, 61 insertions(+), 76 deletions(-)
50dc83
50dc83
diff --git a/libglusterfs/src/fd.c b/libglusterfs/src/fd.c
50dc83
index b8aac72..314546a 100644
50dc83
--- a/libglusterfs/src/fd.c
50dc83
+++ b/libglusterfs/src/fd.c
50dc83
@@ -532,7 +532,7 @@ fd_unref(fd_t *fd)
50dc83
     return;
50dc83
 }
50dc83
 
50dc83
-fd_t *
50dc83
+static fd_t *
50dc83
 __fd_bind(fd_t *fd)
50dc83
 {
50dc83
     list_del_init(&fd->inode_list);
50dc83
@@ -562,9 +562,9 @@ fd_bind(fd_t *fd)
50dc83
 }
50dc83
 
50dc83
 static fd_t *
50dc83
-__fd_create(inode_t *inode, uint64_t pid)
50dc83
+fd_allocate(inode_t *inode, uint64_t pid)
50dc83
 {
50dc83
-    fd_t *fd = NULL;
50dc83
+    fd_t *fd;
50dc83
 
50dc83
     if (inode == NULL) {
50dc83
         gf_msg_callingfn("fd", GF_LOG_ERROR, EINVAL, LG_MSG_INVALID_ARG,
50dc83
@@ -573,64 +573,67 @@ __fd_create(inode_t *inode, uint64_t pid)
50dc83
     }
50dc83
 
50dc83
     fd = mem_get0(inode->table->fd_mem_pool);
50dc83
-    if (!fd)
50dc83
-        goto out;
50dc83
+    if (fd == NULL) {
50dc83
+        return NULL;
50dc83
+    }
50dc83
 
50dc83
     fd->xl_count = inode->table->xl->graph->xl_count + 1;
50dc83
 
50dc83
     fd->_ctx = GF_CALLOC(1, (sizeof(struct _fd_ctx) * fd->xl_count),
50dc83
                          gf_common_mt_fd_ctx);
50dc83
-    if (!fd->_ctx)
50dc83
-        goto free_fd;
50dc83
+    if (fd->_ctx == NULL) {
50dc83
+        goto failed;
50dc83
+    }
50dc83
 
50dc83
     fd->lk_ctx = fd_lk_ctx_create();
50dc83
-    if (!fd->lk_ctx)
50dc83
-        goto free_fd_ctx;
50dc83
-
50dc83
-    fd->inode = inode_ref(inode);
50dc83
-    fd->pid = pid;
50dc83
-    INIT_LIST_HEAD(&fd->inode_list);
50dc83
-
50dc83
-    LOCK_INIT(&fd->lock);
50dc83
-out:
50dc83
-    return fd;
50dc83
+    if (fd->lk_ctx != NULL) {
50dc83
+        /* We need to take a reference from the inode, but we cannot do it
50dc83
+         * here because this function can be called with the inode lock taken
50dc83
+         * and inode_ref() takes the inode's table lock. This is the reverse
50dc83
+         * of the logical lock acquisition order and can cause a deadlock. So
50dc83
+         * we simply assign the inode here and we delefate the inode reference
50dc83
+         * responsibility to the caller (when this function succeeds and the
50dc83
+         * inode lock is released). This is safe because the caller must hold
50dc83
+         * a reference of the inode to use it, so it's guaranteed that the
50dc83
+         * number of references won't reach 0 before the caller finishes.
50dc83
+         *
50dc83
+         * TODO: minimize use of locks in favor of atomic operations to avoid
50dc83
+         *       these dependencies. */
50dc83
+        fd->inode = inode;
50dc83
+        fd->pid = pid;
50dc83
+        INIT_LIST_HEAD(&fd->inode_list);
50dc83
+        LOCK_INIT(&fd->lock);
50dc83
+        GF_ATOMIC_INIT(fd->refcount, 1);
50dc83
+        return fd;
50dc83
+    }
50dc83
 
50dc83
-free_fd_ctx:
50dc83
     GF_FREE(fd->_ctx);
50dc83
-free_fd:
50dc83
+
50dc83
+failed:
50dc83
     mem_put(fd);
50dc83
 
50dc83
     return NULL;
50dc83
 }
50dc83
 
50dc83
 fd_t *
50dc83
-fd_create(inode_t *inode, pid_t pid)
50dc83
+fd_create_uint64(inode_t *inode, uint64_t pid)
50dc83
 {
50dc83
-    fd_t *fd = NULL;
50dc83
-
50dc83
-    fd = __fd_create(inode, (uint64_t)pid);
50dc83
-    if (!fd)
50dc83
-        goto out;
50dc83
+    fd_t *fd;
50dc83
 
50dc83
-    fd = fd_ref(fd);
50dc83
+    fd = fd_allocate(inode, pid);
50dc83
+    if (fd != NULL) {
50dc83
+        /* fd_allocate() doesn't get a reference from the inode. We need to
50dc83
+         * take it here in case of success. */
50dc83
+        inode_ref(inode);
50dc83
+    }
50dc83
 
50dc83
-out:
50dc83
     return fd;
50dc83
 }
50dc83
 
50dc83
 fd_t *
50dc83
-fd_create_uint64(inode_t *inode, uint64_t pid)
50dc83
+fd_create(inode_t *inode, pid_t pid)
50dc83
 {
50dc83
-    fd_t *fd = NULL;
50dc83
-
50dc83
-    fd = __fd_create(inode, pid);
50dc83
-    if (!fd)
50dc83
-        goto out;
50dc83
-
50dc83
-    fd = fd_ref(fd);
50dc83
-
50dc83
-out:
50dc83
-    return fd;
50dc83
+    return fd_create_uint64(inode, (uint64_t)pid);
50dc83
 }
50dc83
 
50dc83
 static fd_t *
50dc83
@@ -719,10 +722,13 @@ __fd_lookup_anonymous(inode_t *inode, int32_t flags)
50dc83
     return fd;
50dc83
 }
50dc83
 
50dc83
-static fd_t *
50dc83
-__fd_anonymous(inode_t *inode, int32_t flags)
50dc83
+fd_t *
50dc83
+fd_anonymous_with_flags(inode_t *inode, int32_t flags)
50dc83
 {
50dc83
     fd_t *fd = NULL;
50dc83
+    bool ref = false;
50dc83
+
50dc83
+    LOCK(&inode->lock);
50dc83
 
50dc83
     fd = __fd_lookup_anonymous(inode, flags);
50dc83
 
50dc83
@@ -730,54 +736,33 @@ __fd_anonymous(inode_t *inode, int32_t flags)
50dc83
        __fd_lookup_anonymous(), so no need of one more fd_ref().
50dc83
        if (!fd); then both create and bind won't bump up the ref
50dc83
        count, so we have to call fd_ref() after bind. */
50dc83
-    if (!fd) {
50dc83
-        fd = __fd_create(inode, 0);
50dc83
-
50dc83
-        if (!fd)
50dc83
-            return NULL;
50dc83
-
50dc83
-        fd->anonymous = _gf_true;
50dc83
-        fd->flags = GF_ANON_FD_FLAGS | flags;
50dc83
+    if (fd == NULL) {
50dc83
+        fd = fd_allocate(inode, 0);
50dc83
+        if (fd != NULL) {
50dc83
+            fd->anonymous = _gf_true;
50dc83
+            fd->flags = GF_ANON_FD_FLAGS | (flags & O_DIRECT);
50dc83
 
50dc83
-        __fd_bind(fd);
50dc83
+            __fd_bind(fd);
50dc83
 
50dc83
-        __fd_ref(fd);
50dc83
+            ref = true;
50dc83
+        }
50dc83
     }
50dc83
 
50dc83
-    return fd;
50dc83
-}
50dc83
-
50dc83
-fd_t *
50dc83
-fd_anonymous(inode_t *inode)
50dc83
-{
50dc83
-    fd_t *fd = NULL;
50dc83
+    UNLOCK(&inode->lock);
50dc83
 
50dc83
-    LOCK(&inode->lock);
50dc83
-    {
50dc83
-        fd = __fd_anonymous(inode, GF_ANON_FD_FLAGS);
50dc83
+    if (ref) {
50dc83
+        /* fd_allocate() doesn't get a reference from the inode. We need to
50dc83
+         * take it here in case of success. */
50dc83
+        inode_ref(inode);
50dc83
     }
50dc83
-    UNLOCK(&inode->lock);
50dc83
 
50dc83
     return fd;
50dc83
 }
50dc83
 
50dc83
 fd_t *
50dc83
-fd_anonymous_with_flags(inode_t *inode, int32_t flags)
50dc83
+fd_anonymous(inode_t *inode)
50dc83
 {
50dc83
-    fd_t *fd = NULL;
50dc83
-
50dc83
-    if (flags & O_DIRECT)
50dc83
-        flags = GF_ANON_FD_FLAGS | O_DIRECT;
50dc83
-    else
50dc83
-        flags = GF_ANON_FD_FLAGS;
50dc83
-
50dc83
-    LOCK(&inode->lock);
50dc83
-    {
50dc83
-        fd = __fd_anonymous(inode, flags);
50dc83
-    }
50dc83
-    UNLOCK(&inode->lock);
50dc83
-
50dc83
-    return fd;
50dc83
+    return fd_anonymous_with_flags(inode, 0);
50dc83
 }
50dc83
 
50dc83
 fd_t *
50dc83
-- 
50dc83
1.8.3.1
50dc83