summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorXavi Hernandez <xhernandez@redhat.com>2019-07-04 13:21:33 +0200
committerRinku Kothiya <rkothiya@redhat.com>2019-07-20 07:42:59 +0000
commite69bc0a183d9cbbace0bc36de87707fc059f2c35 (patch)
treedff23cc1f20765bbbc85e4544523f62e682af482
parent4ad6dd3ca8cd558b853c7df0d6cd34dd22dff7d3 (diff)
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. Backport of: > Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7 > BUG: 1727068 > Signed-off-by: Xavi Hernandez <xhernandez@redhat.com> Change-Id: I24355447f0ea1b39e2546782ad07f0512cc381e7 Fixes: bz#1729950 Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>
-rw-r--r--libglusterfs/src/fd.c137
1 files changed, 61 insertions, 76 deletions
diff --git a/libglusterfs/src/fd.c b/libglusterfs/src/fd.c
index bceb77d6600..e0767a7e61f 100644
--- a/libglusterfs/src/fd.c
+++ b/libglusterfs/src/fd.c
@@ -533,7 +533,7 @@ fd_unref(fd_t *fd)
return;
}
-fd_t *
+static fd_t *
__fd_bind(fd_t *fd)
{
list_del_init(&fd->inode_list);
@@ -563,9 +563,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,
@@ -574,64 +574,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 *
@@ -720,10 +723,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);
@@ -731,54 +737,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 *