gfapi: deliver fdclose before dropping an fd in glfs_fd_destroy()

api/src/glfs.c

@@ -674,16 +674,38 @@ pub_glfs_from_glfd(struct glfs_fd *glfd)
 static void
 glfs_fd_destroy(struct glfs_fd *glfd)
 {
+    struct glfs_fd *other = NULL;
+    gf_boolean_t shared = _gf_false;
+
     if (!glfd)
         return;
 
     glfs_lock(glfd->fs, _gf_true);
     {
         list_del_init(&glfd->openfds);
+
+        /* glfs_dup() makes several glfds share one fd_t; fdclose must be
+         * delivered once, when the last of them goes away. */
+        if (glfd->fd) {
+            list_for_each_entry(other, &glfd->fs->openfds, openfds)
+            {
+                if (other->fd == glfd->fd) {
+                    shared = _gf_true;
+                    break;
+                }
+            }
+        }
     }
     glfs_unlock(glfd->fs);
 
     if (glfd->fd) {
+        /* The application's handle is closing: tell the xlators of the
+         * graph the fd belongs to before dropping the last gfapi
+         * reference, exactly as fuse_release() does. open-behind relies
+         * on this to cancel a still deferred open and release the fd and
+         * stub references it holds for it. */
+        if (!shared)
+            fd_close(glfd->fd);
         fd_unref(glfd->fd);
         glfd->fd = NULL;
     }

tests/basic/gfapi/open-behind-close-leak.c

@@ -0,0 +1,136 @@
+/*
+ * Open N existing files (/f1../fN) read-only through gfapi and close them
+ * without any I/O; then, on N further files (/f(N+1)../f2N), open, glfs_dup()
+ * the handle and close both, alternating the order and reading through the
+ * surviving dup after the original was closed. Finally take a statedump of the
+ * process. With performance.open-behind on the deferred opens must have been
+ * cancelled exactly once, by the last handle: the gfapi xlator's live fd count
+ * in the dump must be 0 and every read through a dup must succeed.
+ *
+ * usage: open-behind-close-leak <host> <volume> <N> <logfile> <pidfile>
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <glusterfs/api/glfs.h>
+
+int
+main(int argc, char *argv[])
+{
+    glfs_t *fs = NULL;
+    glfs_fd_t *fd = NULL;
+    glfs_fd_t *dupfd = NULL;
+    FILE *pf = NULL;
+    char path[64];
+    char buf[1];
+    int n = 0, i = 0, ret = 0;
+
+    if (argc != 6) {
+        fprintf(stderr, "usage: %s <host> <volume> <N> <logfile> <pidfile>\n",
+                argv[0]);
+        return 1;
+    }
+    n = atoi(argv[3]);
+
+    fs = glfs_new(argv[2]);
+    if (!fs) {
+        fprintf(stderr, "glfs_new: %s\n", strerror(errno));
+        return 1;
+    }
+    ret = glfs_set_volfile_server(fs, "tcp", argv[1], 24007);
+    if (ret) {
+        fprintf(stderr, "glfs_set_volfile_server: %s\n", strerror(errno));
+        return 1;
+    }
+    ret = glfs_set_logging(fs, argv[4], 7);
+    if (ret) {
+        fprintf(stderr, "glfs_set_logging: %s\n", strerror(errno));
+        return 1;
+    }
+    ret = glfs_init(fs);
+    if (ret) {
+        fprintf(stderr, "glfs_init: %s\n", strerror(errno));
+        return 1;
+    }
+
+    for (i = 1; i <= n; i++) {
+        snprintf(path, sizeof(path), "/f%d", i);
+        fd = glfs_open(fs, path, O_RDONLY);
+        if (!fd) {
+            fprintf(stderr, "glfs_open(%s): %s\n", path, strerror(errno));
+            return 1;
+        }
+        ret = glfs_close(fd);
+        if (ret) {
+            fprintf(stderr, "glfs_close(%s): %s\n", path, strerror(errno));
+            return 1;
+        }
+    }
+
+    /* glfs_dup(): the two handles share one fd_t; the fdclose must be
+     * delivered once, by whichever handle goes last. */
+    for (i = n + 1; i <= 2 * n; i++) {
+        snprintf(path, sizeof(path), "/f%d", i);
+        fd = glfs_open(fs, path, O_RDONLY);
+        if (!fd) {
+            fprintf(stderr, "glfs_open(%s): %s\n", path, strerror(errno));
+            return 1;
+        }
+        dupfd = glfs_dup(fd);
+        if (!dupfd) {
+            fprintf(stderr, "glfs_dup(%s): %s\n", path, strerror(errno));
+            return 1;
+        }
+        if (i % 2) {
+            /* original first, then read through the dup: a premature
+             * fdclose would leave open-behind unable to serve the read */
+            ret = glfs_close(fd);
+            if (ret) {
+                fprintf(stderr, "glfs_close(%s): %s\n", path, strerror(errno));
+                return 1;
+            }
+            if (glfs_pread(dupfd, buf, 1, 0, 0, NULL) != 1) {
+                fprintf(stderr,
+                        "glfs_pread through dup of %s after closing "
+                        "the original: %s\n",
+                        path, strerror(errno));
+                return 1;
+            }
+            ret = glfs_close(dupfd);
+        } else {
+            ret = glfs_close(dupfd);
+            if (ret) {
+                fprintf(stderr, "glfs_close(dup %s): %s\n", path,
+                        strerror(errno));
+                return 1;
+            }
+            ret = glfs_close(fd);
+        }
+        if (ret) {
+            fprintf(stderr, "glfs_close(%s): %s\n", path, strerror(errno));
+            return 1;
+        }
+    }
+
+    pf = fopen(argv[5], "w");
+    if (!pf) {
+        fprintf(stderr, "fopen(%s): %s\n", argv[5], strerror(errno));
+        return 1;
+    }
+    fprintf(pf, "%d\n", (int)getpid());
+    fclose(pf);
+
+    ret = glfs_sysrq(fs, GLFS_SYSRQ_STATEDUMP);
+    if (ret) {
+        fprintf(stderr, "glfs_sysrq: %s\n", strerror(errno));
+        return 1;
+    }
+
+    glfs_fini(fs);
+    return 0;
+}

tests/basic/gfapi/open-behind-close-leak.t

@@ -0,0 +1,59 @@
+#!/bin/bash
+#
+# gfapi released its fds without delivering fdclose to the graph, so with
+# performance.open-behind on a glfs_open() followed by glfs_close() with no
+# I/O in between left the fd_t alive (open-behind's deferred-open references)
+# and pinned its inode for the life of the process. glfs_dup()ed handles share
+# the fd_t, so the fdclose must come from the last handle only: the tester
+# also opens, dups and closes N more files in both orders, reading through the
+# dup after the original was closed.
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+cleanup;
+
+N=3
+tester=$(dirname $0)/open-behind-close-leak
+logfile=$LOGDIR/open-behind-close-leak.log
+pidfile=$B0/open-behind-close-leak.pid
+
+# Live fd_t objects created by the gfapi process: every fd_t owns exactly one
+# gf_common_mt_fd_ctx array, accounted to the gfapi master xlator. This works on
+# every build; the fd_t mem-pool count does not exist on tcmalloc builds.
+function gfapi_live_fd_count {
+        if [ ! -f "$1" ]; then
+                echo "no-statedump"
+                return
+        fi
+        awk -F= '/^\[mount\/api\.gfapi - usage-type gf_common_mt_fd_ctx memusage\]$/ {f=1}
+                 f && /^num_allocs=/ {print $2; f=0; e=1} END {if (!e) print 0}' "$1"
+}
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume create $V0 $H0:$B0/${V0}0
+TEST $CLI volume set $V0 performance.open-behind on
+TEST $CLI volume start $V0
+
+# the files must exist before the gfapi process first sees their inodes;
+# f1..fN for the plain pass, f(N+1)..f2N for the dup pass
+TEST $GFS --volfile-id=/$V0 --volfile-server=$H0 $M0
+for i in $(seq 1 $((2 * N))); do
+        echo data > $M0/f$i
+done
+EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
+
+TEST build_tester $tester.c -lgfapi
+rm -f $pidfile
+TEST $tester $H0 $V0 $N $logfile $pidfile
+pid=$(cat $pidfile 2>/dev/null)
+TEST [ -n "$pid" ]
+dump=$(ls $statedumpdir/glusterdump.$pid.dump.* 2>/dev/null | head -1)
+TEST [ -n "$dump" ]
+
+EXPECT "0" gfapi_live_fd_count "$dump"
+
+rm -f $dump $pidfile
+cleanup_tester $tester
+cleanup;