1 files changed, 102 insertions, 85 deletions

diff --git a/libglusterfs/src/fd.c b/libglusterfs/src/fd.c
index d26b7097fd4..62606e91164 100644
--- a/libglusterfs/src/fd.c
+++ b/libglusterfs/src/fd.c
@@ -8,11 +8,12 @@
   cases as published by the Free Software Foundation.
 */
 
-#include "fd.h"
-#include "glusterfs.h"
-#include "dict.h"
-#include "statedump.h"
-#include "libglusterfs-messages.h"
+#include "glusterfs/fd.h"
+#include <errno.h>     // for EINVAL, errno, ENOMEM
+#include <inttypes.h>  // for PRIu64
+#include <stdint.h>    // for UINT32_MAX
+#include <string.h>    // for NULL, memcpy, memset, size_t
+#include "glusterfs/statedump.h"
 
 static int
 gf_fd_fdtable_expand(fdtable_t *fdtable, uint32_t nr);
@@ -501,6 +502,32 @@ out:
 }
 
 void
+fd_close(fd_t *fd)
+{
+    xlator_t *xl, *old_THIS;
+
+    old_THIS = THIS;
+
+    for (xl = fd->inode->table->xl->graph->first; xl != NULL; xl = xl->next) {
+        if (!xl->call_cleanup) {
+            THIS = xl;
+
+            if (IA_ISDIR(fd->inode->ia_type)) {
+                if (xl->cbks->fdclosedir != NULL) {
+                    xl->cbks->fdclosedir(xl, fd);
+                }
+            } else {
+                if (xl->cbks->fdclose != NULL) {
+                    xl->cbks->fdclose(xl, fd);
+                }
+            }
+        }
+    }
+
+    THIS = old_THIS;
+}
+
+void
 fd_unref(fd_t *fd)
 {
     int32_t refcount = 0;
@@ -532,7 +559,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 +589,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 +600,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 +749,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 +763,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;
-
-    LOCK(&inode->lock);
-    {
-        if (flags & O_DIRECT)
-            flags = GF_ANON_FD_FLAGS | O_DIRECT;
-        else
-            flags = GF_ANON_FD_FLAGS;
-
-        fd = __fd_anonymous(inode, flags);
-    }
-    UNLOCK(&inode->lock);
-
-    return fd;
+    return fd_anonymous_with_flags(inode, 0);
 }
 
 fd_t *
@@ -992,13 +1004,14 @@ fd_dump(fd_t *fd, char *prefix)
     if (!fd)
         return;
 
-    gf_proc_dump_write("pid", "%llu", fd->pid);
-    gf_proc_dump_write("refcount", "%d", fd->refcount);
+    gf_proc_dump_write("pid", "%" PRIu64, fd->pid);
+    gf_proc_dump_write("refcount", "%" GF_PRI_ATOMIC,
+                       GF_ATOMIC_GET(fd->refcount));
     gf_proc_dump_write("flags", "%d", fd->flags);
 
     if (fd->inode) {
         gf_proc_dump_build_key(key, "inode", NULL);
-        gf_proc_dump_add_section(key);
+        gf_proc_dump_add_section("%s", key);
         inode_dump(fd->inode, key);
     }
 }
@@ -1040,7 +1053,7 @@ fdtable_dump(fdtable_t *fdtable, char *prefix)
     for (i = 0; i < fdtable->max_fds; i++) {
         if (GF_FDENTRY_ALLOCATED == fdtable->fdentries[i].next_free) {
             gf_proc_dump_build_key(key, prefix, "fdentry[%d]", i);
-            gf_proc_dump_add_section(key);
+            gf_proc_dump_add_section("%s", key);
             fdentry_dump(&fdtable->fdentries[i], key);
         }
     }
@@ -1130,6 +1143,8 @@ fdentry_dump_to_dict(fdentry_t *fdentry, char *prefix, dict_t *dict,
 
         snprintf(key, sizeof(key), "%s.flags", prefix);
         ret = dict_set_int32(dict, key, fdentry->fd->flags);
+        if (ret)
+            return;
 
         (*openfds)++;
     }
@@ -1179,6 +1194,8 @@ fdtable_dump_to_dict(fdtable_t *fdtable, char *prefix, dict_t *dict)
 
     snprintf(key, sizeof(key), "%s.fdtable.openfds", prefix);
     ret = dict_set_int32(dict, key, openfds);
+    if (ret)
+        goto out;
 
 out:
     pthread_rwlock_unlock(&fdtable->lock);