247 lines
6.2 KiB
Diff
247 lines
6.2 KiB
Diff
|
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
|
||
|
|