From 175c99dccc47d2b4267a8819404e5cbeb8cfba11 Mon Sep 17 00:00:00 2001 From: Mohit Agrawal 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 > Change-Id: Iffb031f0695a7da83d5a2f6bac8863dad225317e > Fixes: bz#1811631 > Signed-off-by: Mohit Agrawal > (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 Reviewed-on: https://code.engineering.redhat.com/gerrit/202791 Tested-by: Mohit Agrawal Reviewed-by: Sunil Kumar Heggodu Gopala Acharya --- 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