From 175c99dccc47d2b4267a8819404e5cbeb8cfba11 Mon Sep 17 00:00:00 2001

From: Mohit Agrawal <moagrawal@redhat.com>

Date: Thu, 12 Mar 2020 21:12:13 +0530

Subject: [PATCH 448/449] Posix: Use simple approach to close fd

Problem: posix_release(dir) functions add the fd's into a ctx->janitor_fds

         and janitor thread closes the fd's.In brick_mux environment it is

         difficult to handle race condition in janitor threads because brick

         spawns a single janitor thread for all bricks.

Solution: Use synctask to execute posix_release(dir) functions instead of

          using background a thread to close fds.

> Credits: Pranith Karampuri <pkarampu@redhat.com>

> Change-Id: Iffb031f0695a7da83d5a2f6bac8863dad225317e

> Fixes: bz#1811631

> Signed-off-by: Mohit Agrawal <moagrawal@redhat.com>

> (Cherry pick from commit fb20713b380e1df8d7f9e9df96563be2f9144fd6)

> (Reviewed on upstream link https://review.gluster.org/#/c/glusterfs/+/24221/)

BUG: 1790336

Change-Id: Iffb031f0695a7da83d5a2f6bac8863dad225317e

Signed-off-by: Mohit Agrawal <moagrawal@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/202791

Tested-by: Mohit Agrawal <moagrawa@redhat.com>

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 libglusterfs/src/glusterfs/glusterfs.h         |  6 +-

 libglusterfs/src/glusterfs/syncop.h            |  7 +-

 rpc/rpc-lib/src/rpcsvc.c                       |  6 ++

 run-tests.sh                                   |  2 +-

 tests/features/ssl-authz.t                     |  7 +-

 xlators/storage/posix/src/posix-common.c       |  4 --

 xlators/storage/posix/src/posix-helpers.c      | 98 --------------------------

 xlators/storage/posix/src/posix-inode-fd-ops.c | 28 ++------

 xlators/storage/posix/src/posix.h              |  3 -

 9 files changed, 20 insertions(+), 141 deletions(-)

diff --git a/libglusterfs/src/glusterfs/glusterfs.h b/libglusterfs/src/glusterfs/glusterfs.h

index 584846e..495a4d7 100644

--- a/libglusterfs/src/glusterfs/glusterfs.h

+++ b/libglusterfs/src/glusterfs/glusterfs.h

@@ -734,11 +734,7 @@ struct _glusterfs_ctx {

     struct list_head volfile_list;

-    /* Add members to manage janitor threads for cleanup fd */

-    struct list_head janitor_fds;

-    pthread_cond_t janitor_cond;

-    pthread_mutex_t janitor_lock;

-    pthread_t janitor;

+    char volume_id[GF_UUID_BUF_SIZE]; /* Used only in protocol/client */

 };

 typedef struct _glusterfs_ctx glusterfs_ctx_t;

diff --git a/libglusterfs/src/glusterfs/syncop.h b/libglusterfs/src/glusterfs/syncop.h

index 3011b4c..1e4c73b 100644

--- a/libglusterfs/src/glusterfs/syncop.h

+++ b/libglusterfs/src/glusterfs/syncop.h

@@ -254,7 +254,7 @@ struct syncopctx {

         task = synctask_get();                                                 \

         stb->task = task;                                                      \

         if (task)                                                              \

-            frame = task->opframe;                                             \

+            frame = copy_frame(task->opframe);                                 \

         else                                                                   \

             frame = syncop_create_frame(THIS);                                 \

                                                                                \

@@ -269,10 +269,7 @@ struct syncopctx {

         STACK_WIND_COOKIE(frame, cbk, (void *)stb, subvol, fn_op, params);     \

                                                                                \

         __yield(stb);                                                          \

-        if (task)                                                              \

-            STACK_RESET(frame->root);                                          \

-        else                                                                   \

-            STACK_DESTROY(frame->root);                                        \

+        STACK_DESTROY(frame->root);                                            \

     } while (0)

 /*

diff --git a/rpc/rpc-lib/src/rpcsvc.c b/rpc/rpc-lib/src/rpcsvc.c

index 3f184bf..23ca1fd 100644

--- a/rpc/rpc-lib/src/rpcsvc.c

+++ b/rpc/rpc-lib/src/rpcsvc.c

@@ -375,6 +375,12 @@ rpcsvc_program_actor(rpcsvc_request_t *req)

     req->ownthread = program->ownthread;

     req->synctask = program->synctask;

+    if (((req->procnum == GFS3_OP_RELEASE) ||

+         (req->procnum == GFS3_OP_RELEASEDIR)) &&

+        (program->prognum == GLUSTER_FOP_PROGRAM)) {

+        req->ownthread = _gf_false;

+        req->synctask = _gf_true;

+    }

     err = SUCCESS;

     gf_log(GF_RPCSVC, GF_LOG_TRACE, "Actor found: %s - %s for %s",

diff --git a/run-tests.sh b/run-tests.sh

index 5683b21..c835d93 100755

--- a/run-tests.sh

+++ b/run-tests.sh

@@ -356,7 +356,7 @@ function run_tests()

             selected_tests=$((selected_tests+1))

             echo

             echo $section_separator$section_separator

-            if [[ $(get_test_status $t) == "BAD_TEST" ]] && \

+            if [[ $(get_test_status $t) =~ "BAD_TEST" ]] && \

                [[ $skip_bad_tests == "yes" ]]

             then

                 skipped_bad_tests=$((skipped_bad_tests+1))

diff --git a/tests/features/ssl-authz.t b/tests/features/ssl-authz.t

index 132b598..497083e 100755

--- a/tests/features/ssl-authz.t

+++ b/tests/features/ssl-authz.t

@@ -67,13 +67,14 @@ echo "Memory consumption for glusterfsd process"

 for i in $(seq 1 100); do

         gluster v heal $V0 info >/dev/null

 done

-

+#Wait to cleanup memory

+sleep 10

 end=`pmap -x $glusterfsd_pid | grep total | awk -F " " '{print $4}'`

 diff=$((end-start))

-# If memory consumption is more than 5M some leak in SSL code path

+# If memory consumption is more than 15M some leak in SSL code path

-TEST [ $diff -lt 5000 ]

+TEST [ $diff -lt 15000 ]

 # Set ssl-allow to a wildcard that includes our identity.

diff --git a/xlators/storage/posix/src/posix-common.c b/xlators/storage/posix/src/posix-common.c

index 2cb58ba..ac53796 100644

--- a/xlators/storage/posix/src/posix-common.c

+++ b/xlators/storage/posix/src/posix-common.c

@@ -1041,10 +1041,6 @@ posix_init(xlator_t *this)

     pthread_mutex_init(&_private->janitor_mutex, NULL);

     pthread_cond_init(&_private->janitor_cond, NULL);

     INIT_LIST_HEAD(&_private->fsyncs);

-    ret = posix_spawn_ctx_janitor_thread(this);

-    if (ret)

-        goto out;

-

     ret = gf_thread_create(&_private->fsyncer, NULL, posix_fsyncer, this,

                            "posixfsy");

     if (ret) {

diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c

index 2336add..39dbcce 100644

--- a/xlators/storage/posix/src/posix-helpers.c

+++ b/xlators/storage/posix/src/posix-helpers.c

@@ -1582,104 +1582,6 @@ unlock:

     return;

 }

-static struct posix_fd *

-janitor_get_next_fd(glusterfs_ctx_t *ctx, int32_t janitor_sleep)

-{

-    struct posix_fd *pfd = NULL;

-

-    struct timespec timeout;

-

-    pthread_mutex_lock(&ctx->janitor_lock);

-    {

-        if (list_empty(&ctx->janitor_fds)) {

-            time(&timeout.tv_sec);

-            timeout.tv_sec += janitor_sleep;

-            timeout.tv_nsec = 0;

-

-            pthread_cond_timedwait(&ctx->janitor_cond, &ctx->janitor_lock,

-                                   &timeout);

-            goto unlock;

-        }

-

-        pfd = list_entry(ctx->janitor_fds.next, struct posix_fd, list);

-

-        list_del(ctx->janitor_fds.next);

-    }

-unlock:

-    pthread_mutex_unlock(&ctx->janitor_lock);

-

-    return pfd;

-}

-

-static void *

-posix_ctx_janitor_thread_proc(void *data)

-{

-    xlator_t *this = NULL;

-    struct posix_fd *pfd;

-    glusterfs_ctx_t *ctx = NULL;

-    struct posix_private *priv = NULL;

-    int32_t sleep_duration = 0;

-

-    this = data;

-    ctx = THIS->ctx;

-    THIS = this;

-

-    priv = this->private;

-    sleep_duration = priv->janitor_sleep_duration;

-    while (1) {

-        pfd = janitor_get_next_fd(ctx, sleep_duration);

-        if (pfd) {

-            if (pfd->dir == NULL) {

-                gf_msg_trace(this->name, 0, "janitor: closing file fd=%d",

-                             pfd->fd);

-                sys_close(pfd->fd);

-            } else {

-                gf_msg_debug(this->name, 0, "janitor: closing dir fd=%p",

-                             pfd->dir);

-                sys_closedir(pfd->dir);

-            }

-

-            GF_FREE(pfd);

-        }

-    }

-

-    return NULL;

-}

-

-int

-posix_spawn_ctx_janitor_thread(xlator_t *this)

-{

-    struct posix_private *priv = NULL;

-    int ret = 0;

-    glusterfs_ctx_t *ctx = NULL;

-

-    priv = this->private;

-    ctx = THIS->ctx;

-

-    LOCK(&priv->lock);

-    {

-        if (!ctx->janitor) {

-            pthread_mutex_init(&ctx->janitor_lock, NULL);

-            pthread_cond_init(&ctx->janitor_cond, NULL);

-            INIT_LIST_HEAD(&ctx->janitor_fds);

-

-            ret = gf_thread_create(&ctx->janitor, NULL,

-                                   posix_ctx_janitor_thread_proc, this,

-                                   "posixctxjan");

-

-            if (ret) {

-                gf_msg(this->name, GF_LOG_ERROR, errno, P_MSG_THREAD_FAILED,

-                       "spawning janitor "

-                       "thread failed");

-                goto unlock;

-            }

-        }

-    }

-unlock:

-    UNLOCK(&priv->lock);

-    return ret;

-}

-

 static int

 is_fresh_file(int64_t ctime_sec)

 {

diff --git a/xlators/storage/posix/src/posix-inode-fd-ops.c b/xlators/storage/posix/src/posix-inode-fd-ops.c

index 5748b9f..d135d8b 100644

--- a/xlators/storage/posix/src/posix-inode-fd-ops.c

+++ b/xlators/storage/posix/src/posix-inode-fd-ops.c

@@ -1358,7 +1358,6 @@ posix_releasedir(xlator_t *this, fd_t *fd)

     struct posix_fd *pfd = NULL;

     uint64_t tmp_pfd = 0;

     int ret = 0;

-    glusterfs_ctx_t *ctx = NULL;

     VALIDATE_OR_GOTO(this, out);

     VALIDATE_OR_GOTO(fd, out);

@@ -1376,21 +1375,11 @@ posix_releasedir(xlator_t *this, fd_t *fd)

         goto out;

     }

-    ctx = THIS->ctx;

-

-    pthread_mutex_lock(&ctx->janitor_lock);

-    {

-        INIT_LIST_HEAD(&pfd->list);

-        list_add_tail(&pfd->list, &ctx->janitor_fds);

-        pthread_cond_signal(&ctx->janitor_cond);

-    }

-    pthread_mutex_unlock(&ctx->janitor_lock);

-

-    /*gf_msg_debug(this->name, 0, "janitor: closing dir fd=%p", pfd->dir);

+    gf_msg_debug(this->name, 0, "janitor: closing dir fd=%p", pfd->dir);

     sys_closedir(pfd->dir);

     GF_FREE(pfd);

-    */

+

 out:

     return 0;

 }

@@ -2510,13 +2499,11 @@ posix_release(xlator_t *this, fd_t *fd)

     struct posix_fd *pfd = NULL;

     int ret = -1;

     uint64_t tmp_pfd = 0;

-    glusterfs_ctx_t *ctx = NULL;

     VALIDATE_OR_GOTO(this, out);

     VALIDATE_OR_GOTO(fd, out);

     priv = this->private;

-    ctx = THIS->ctx;

     ret = fd_ctx_del(fd, this, &tmp_pfd);

     if (ret < 0) {

@@ -2531,13 +2518,10 @@ posix_release(xlator_t *this, fd_t *fd)

                "pfd->dir is %p (not NULL) for file fd=%p", pfd->dir, fd);

     }

-    pthread_mutex_lock(&ctx->janitor_lock);

-    {

-        INIT_LIST_HEAD(&pfd->list);

-        list_add_tail(&pfd->list, &ctx->janitor_fds);

-        pthread_cond_signal(&ctx->janitor_cond);

-    }

-    pthread_mutex_unlock(&ctx->janitor_lock);

+    gf_msg_debug(this->name, 0, "janitor: closing dir fd=%p", pfd->dir);

+

+    sys_close(pfd->fd);

+    GF_FREE(pfd);

     if (!priv)

         goto out;

diff --git a/xlators/storage/posix/src/posix.h b/xlators/storage/posix/src/posix.h

index ac9d83c..61495a7 100644

--- a/xlators/storage/posix/src/posix.h

+++ b/xlators/storage/posix/src/posix.h

@@ -666,9 +666,6 @@ posix_cs_maintenance(xlator_t *this, fd_t *fd, loc_t *loc, int *pfd,

 int

 posix_check_dev_file(xlator_t *this, inode_t *inode, char *fop, int *op_errno);

-int

-posix_spawn_ctx_janitor_thread(xlator_t *this);

-

 void

 posix_update_iatt_buf(struct iatt *buf, int fd, char *loc, dict_t *xdata);

-- 

1.8.3.1