diff options
author | Poornima G <pgurusid@redhat.com> | 2015-02-19 21:29:02 +0530 |
---|---|---|
committer | Shyamsundar Ranganathan <srangana@redhat.com> | 2015-03-04 07:15:12 -0800 |
commit | 9c5011b8e49e34d736ba7cfadc0802e8b80682a7 (patch) | |
tree | cacffa0ab6ba732d72a9c6f9eadd050c60cbae15 | |
parent | f82756b4909cebaf533570aec2b05ba55a7dcc95 (diff) |
glfs_fini: Clean up all the resources allocated in glfs_new.
Initially even after calling glfs_fini(), all the threads created
during init and many other resources like memory pool, iobuf pool,
event pool and other memory allocs were not being freed.
With this patch these resources are freed in glfs_fini().
The two thumb rules followed in this patch are:
- The threads are not killed, they are made to exit voluntarily,
once the queued tasks are completed. The main thread waits for
the other threads to exit.
- Free the memory pools and destroy the graphs only after all the
other threads are stopped, so that there are less chances of
hitting access after free.
Resources freed and its order:
1. Destroy the inode table of all the graphs - Call forget on all the inodes.
This will not be required when the cleanup during graph switch is
implemented to perform inode table destroy.
2. Deactivate the current graph, call fini of all the xlators.
3. Syncenv destroy - Join the synctask threads and cleanup syncenv resources
Sets the destroy mode, complete the existing synctasks, then join the
synctask threads.
After entering the destroy mode,
-if a new synctask is submitted, it fails.
-if syncenv_new() is called, it will end up creating new threads,
but this is called only during init.
4. Poller thread destroy
Register an event handler which sets the destroy mode for the poller.
Once the poller is done processing all the events, it exits.
5. Tear down the logging framework
The log file is closed and the log level is set to none, after this
point no log messages appear either in log file or in stderr.
6. Destroy the timer thread
Set the destroy bit, once the pending timer events are processed
the timer thread exits.
Note: Log infrastructure should be shutdown before destroying the timer
thread as gf_log uses timers.
7. Destroy the glusterfs_ctx_t
For all the graphs(active and passive), free graph, xlator structs and few other lists.
Free the memory pools - iobuf pool, event pool, dict, logbuf pool,
stub mem pool, stack mem pool, frame mem pool.
Few things not addressed in this patch:
1. rpc_transport object not destroyed, the PARENT_DOWN should have
destroyed this object but has not, needs to be addressed as a part
of different patch
2. Each xlator fini should clean up the local pool allocated by its xlator.
Needs to be addresses as a part of different patch.
3. Each xlator should implement forget to free its inode_ctx.
Needs to be addresses as a part of different patch.
3. Few other leaks reported by valgrind.
4. fd and fd contexts
The numbers:
The resource usage by the test case in this patch:
Without the fix, Memory: ~3GB; Threads: ~81
With this fix, Memory: 300MB; Threads: 1(main thread)
Change-Id: I96b9277541737aa8372b4e6c9eed380cb871e7c2
BUG: 1093594
Signed-off-by: Poornima G <pgurusid@redhat.com>
Reviewed-on: http://review.gluster.org/7642
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>
Reviewed-by: Raghavendra Talur <rtalur@redhat.com>
Reviewed-by: Krishnan Parthasarathi <kparthas@redhat.com>
Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Reviewed-by: Shyamsundar Ranganathan <srangana@redhat.com>
-rw-r--r-- | api/src/glfs-mem-types.h | 3 | ||||
-rw-r--r-- | api/src/glfs.c | 126 | ||||
-rw-r--r-- | tests/bugs/bug-1093594.c | 315 | ||||
-rw-r--r-- | tests/bugs/bug-1093594.t | 20 | ||||
-rw-r--r-- | xlators/cluster/afr/src/afr-common.c | 1 | ||||
-rw-r--r-- | xlators/performance/io-cache/src/io-cache.c | 7 |
6 files changed, 462 insertions, 10 deletions
diff --git a/api/src/glfs-mem-types.h b/api/src/glfs-mem-types.h index 76f4fc774cc..0a2d4a7df22 100644 --- a/api/src/glfs-mem-types.h +++ b/api/src/glfs-mem-types.h @@ -16,8 +16,7 @@ #define GF_MEM_TYPE_START (gf_common_mt_end + 1) enum glfs_mem_types_ { - glfs_mt_glfs_t = GF_MEM_TYPE_START, - glfs_mt_call_pool_t, + glfs_mt_call_pool_t = GF_MEM_TYPE_START, glfs_mt_xlator_t, glfs_mt_glfs_fd_t, glfs_mt_glfs_io_t, diff --git a/api/src/glfs.c b/api/src/glfs.c index 8389d674266..7d493720121 100644 --- a/api/src/glfs.c +++ b/api/src/glfs.c @@ -572,7 +572,7 @@ pub_glfs_new (const char *volname) if (ret) return NULL; - fs = GF_CALLOC (1, sizeof (*fs), glfs_mt_glfs_t); + fs = CALLOC (1, sizeof (*fs)); if (!fs) return NULL; fs->ctx = ctx; @@ -581,7 +581,7 @@ pub_glfs_new (const char *volname) fs->ctx->cmd_args.volfile_id = gf_strdup (volname); - fs->volname = gf_strdup (volname); + fs->volname = strdup (volname); pthread_mutex_init (&fs->mutex, NULL); pthread_cond_init (&fs->cond, NULL); @@ -603,7 +603,7 @@ priv_glfs_new_from_ctx (glusterfs_ctx_t *ctx) if (!ctx) return NULL; - fs = GF_CALLOC (1, sizeof (*fs), glfs_mt_glfs_t); + fs = CALLOC (1, sizeof (*fs)); if (!fs) return NULL; fs->ctx = ctx; @@ -631,7 +631,8 @@ priv_glfs_free_from_ctx (struct glfs *fs) (void) pthread_mutex_destroy (&fs->mutex); - GF_FREE (fs); + FREE (fs->volname); + FREE (fs); } GFAPI_SYMVER_PRIVATE_DEFAULT(glfs_free_from_ctx, 3.7.0); @@ -813,6 +814,73 @@ pub_glfs_init (struct glfs *fs) GFAPI_SYMVER_PUBLIC_DEFAULT(glfs_init, 3.4.0); +static int +glusterfs_ctx_destroy (glusterfs_ctx_t *ctx) +{ + call_pool_t *pool = NULL; + int ret = 0; + glusterfs_graph_t *trav_graph = NULL; + glusterfs_graph_t *tmp = NULL; + + if (ctx == NULL) + return 0; + + /* For all the graphs, crawl through the xlator_t structs and free + * all its members except for the mem_acct.rec member, + * as GF_FREE will be referencing it. + */ + list_for_each_entry_safe (trav_graph, tmp, &ctx->graphs, list) { + xlator_tree_free_members (trav_graph->first); + } + + /* Free the memory pool */ + if (ctx->stub_mem_pool) + mem_pool_destroy (ctx->stub_mem_pool); + if (ctx->dict_pool) + mem_pool_destroy (ctx->dict_pool); + if (ctx->dict_data_pool) + mem_pool_destroy (ctx->dict_data_pool); + if (ctx->dict_pair_pool) + mem_pool_destroy (ctx->dict_pair_pool); + if (ctx->logbuf_pool) + mem_pool_destroy (ctx->logbuf_pool); + + pool = ctx->pool; + if (pool) { + if (pool->frame_mem_pool) + mem_pool_destroy (pool->frame_mem_pool); + if (pool->stack_mem_pool) + mem_pool_destroy (pool->stack_mem_pool); + LOCK_DESTROY (&pool->lock); + GF_FREE (pool); + } + + /* Free the event pool */ + ret = event_pool_destroy (ctx->event_pool); + + /* Free the iobuf pool */ + iobuf_pool_destroy (ctx->iobuf_pool); + + GF_FREE (ctx->process_uuid); + GF_FREE (ctx->cmd_args.volfile_id); + + pthread_mutex_destroy (&(ctx->lock)); + pthread_mutex_destroy (&(ctx->notify_lock)); + pthread_cond_destroy (&(ctx->notify_cond)); + + /* Free all the graph structs and its containing xlator_t structs + * from this point there should be no reference to GF_FREE/GF_CALLOC + * as it will try to access mem_acct and the below funtion would + * have freed the same. + */ + list_for_each_entry_safe (trav_graph, tmp, &ctx->graphs, list) { + glusterfs_graph_destroy_residual (trav_graph); + } + + FREE (ctx); + + return ret; +} int pub_glfs_fini (struct glfs *fs) @@ -901,11 +969,59 @@ pub_glfs_fini (struct glfs *fs) glfs_subvol_done (fs, subvol); } - if (gf_log_fini(ctx) != 0) + ctx->cleanup_started = 1; + + if (fs_init != 0) { + /* Destroy all the inode tables of all the graphs. + * NOTE: + * - inode objects should be destroyed before calling fini() + * of each xlator, as fini() and forget() of the xlators + * can share few common locks or data structures, calling + * fini first might destroy those required by forget + * ( eg: in quick-read) + * - The call to inode_table_destroy_all is not required when + * the cleanup during graph switch is implemented to perform + * inode table destroy. + */ + inode_table_destroy_all (ctx); + + /* Call fini() of all the xlators in the active graph + * NOTE: + * - xlator fini() should be called before destroying any of + * the threads. (eg: fini() in protocol-client uses timer + * thread) */ + glusterfs_graph_deactivate (ctx->active); + + /* Join the syncenv_processor threads and cleanup + * syncenv resources*/ + syncenv_destroy (ctx->env); + + /* Join the poller thread */ + if (event_dispatch_destroy (ctx->event_pool) != 0) + ret = -1; + } + + /* log infra has to be brought down before destroying + * timer registry, as logging uses timer infra + */ + if (gf_log_fini (ctx) != 0) + ret = -1; + + /* Join the timer thread */ + if (fs_init != 0) { + gf_timer_registry_destroy (ctx); + } + + /* Destroy the context and the global pools */ + if (glusterfs_ctx_destroy (ctx) != 0) ret = -1; + + glfs_free_from_ctx (fs); + fail: if (!ret) ret = err; + return ret; } diff --git a/tests/bugs/bug-1093594.c b/tests/bugs/bug-1093594.c new file mode 100644 index 00000000000..8f5aa9be66c --- /dev/null +++ b/tests/bugs/bug-1093594.c @@ -0,0 +1,315 @@ +#include "../../api/src/glfs.h" +#include "../../api/src/glfs-handles.h" +#include <errno.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#define WRITE_SIZE (128*1024) +#define READ_WRITE_LOOP 100 +#define FOP_LOOP_COUNT 20 +#define TEST_CASE_LOOP 20 + +int gfapi = 1; +static int extension = 1; + +static int +large_number_of_fops (glfs_t *fs) { + int ret = 0; + int i = 0; + glfs_fd_t *fd = NULL; + glfs_fd_t *fd1 = NULL; + char *dir1 = NULL, *dir2 = NULL, *filename1 = NULL, *filename2 = NULL; + char *buf = NULL; + struct stat sb = {0, }; + + for (i = 0 ; i < FOP_LOOP_COUNT ; i++) { + ret = asprintf (&dir1, "dir%d", extension); + if (ret < 0) { + fprintf (stderr, "cannot construct filename (%s)", + strerror (errno)); + return ret; + } + + extension++; + + ret = glfs_mkdir (fs, dir1, 0755); + if (ret < 0) { + fprintf (stderr, "mkdir(%s): %s\n", dir1, strerror (errno)); + return -1; + } + + fd = glfs_opendir (fs, dir1); + if (!fd) { + fprintf (stderr, "/: %s\n", strerror (errno)); + return -1; + } + + ret = glfs_fsetxattr (fd, "user.dirfattr", "fsetxattr", 8, 0); + if (ret < 0) { + fprintf (stderr, "fsetxattr(%s): %d (%s)\n", dir1, ret, + strerror (errno)); + return -1; + } + + ret = glfs_closedir (fd); + if (ret < 0) { + fprintf (stderr, "glfs_closedir failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_rmdir (fs, dir1); + if (ret < 0) { + fprintf (stderr, "glfs_unlink failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = asprintf (&filename1, "file%d", extension); + if (ret < 0) { + fprintf (stderr, "cannot construct filename (%s)", + strerror (errno)); + return ret; + } + + ret = asprintf (&filename2, "file-%d", extension); + if (ret < 0) { + fprintf (stderr, "cannot construct filename (%s)", + strerror (errno)); + return ret; + } + + extension++; + + fd = glfs_creat (fs, filename1, O_RDWR, 0644); + if (!fd) { + fprintf (stderr, "%s: (%p) %s\n", filename1, fd, + strerror (errno)); + return -1; + } + + ret = glfs_rename (fs, filename1, filename2); + if (ret < 0) { + fprintf (stderr, "glfs_rename failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_lstat (fs, filename2, &sb); + if (ret < 0) { + fprintf (stderr, "glfs_lstat failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_close (fd); + if (ret < 0) { + fprintf (stderr, "glfs_close failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_unlink (fs, filename2); + if (ret < 0) { + fprintf (stderr, "glfs_unlink failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + } +} + +static int +large_read_write (glfs_t *fs) { + + int ret = 0; + int j = 0; + glfs_fd_t *fd = NULL; + glfs_fd_t *fd1 = NULL; + char *filename = NULL; + char *buf = NULL; + + ret = asprintf (&filename, "filerw%d", extension); + if (ret < 0) { + fprintf (stderr, "cannot construct filename (%s)", + strerror (errno)); + return ret; + } + + extension++; + + fd = glfs_creat (fs, filename, O_RDWR, 0644); + if (!fd) { + fprintf (stderr, "%s: (%p) %s\n", filename, fd, + strerror (errno)); + return -1; + } + + buf = (char *) malloc (WRITE_SIZE); + memset (buf, '-', WRITE_SIZE); + + for (j = 0; j < READ_WRITE_LOOP; j++) { + ret = glfs_write (fd, buf, WRITE_SIZE, 0); + if (ret < 0) { + fprintf (stderr, "Write(%s): %d (%s)\n", filename, ret, + strerror (errno)); + return ret; + } + } + + fd1 = glfs_open (fs, filename, O_RDWR); + if (fd1 < 0) { + fprintf (stderr, "Open(%s): %d (%s)\n", filename, ret, + strerror (errno)); + return -1; + } + + glfs_lseek (fd1, 0, SEEK_SET); + for (j = 0; j < READ_WRITE_LOOP; j++) { + ret = glfs_read (fd1, buf, WRITE_SIZE, 0); + if (ret < 0) { + fprintf (stderr, "Read(%s): %d (%s)\n", filename, ret, + strerror (errno)); + return ret; + } + } + + for (j = 0; j < READ_WRITE_LOOP; j++) { + ret = glfs_write (fd1, buf, WRITE_SIZE, 0); + if (ret < 0) { + fprintf (stderr, "Write(%s): %d (%s)\n", filename, ret, + strerror (errno)); + return ret; + } + } + + glfs_close (fd); + glfs_close (fd1); + ret = glfs_unlink (fs, filename); + if (ret < 0) { + fprintf (stderr, "glfs_unlink failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + free (buf); + free (filename); +} + +static int +volfile_change (const char *volname) { + int ret = 0; + char *cmd = NULL, *cmd1 = NULL; + + ret = asprintf (&cmd, "gluster volume set %s stat-prefetch off", + volname); + if (ret < 0) { + fprintf (stderr, "cannot construct cli command string (%s)", + strerror (errno)); + return ret; + } + + ret = asprintf (&cmd1, "gluster volume set %s stat-prefetch on", + volname); + if (ret < 0) { + fprintf (stderr, "cannot construct cli command string (%s)", + strerror (errno)); + return ret; + } + + ret = system (cmd); + if (ret < 0) { + fprintf (stderr, "stat-prefetch off on (%s) failed", volname); + return ret; + } + + ret = system (cmd1); + if (ret < 0) { + fprintf (stderr, "stat-prefetch on on (%s) failed", volname); + return ret; + } + + free (cmd); + free (cmd1); + return ret; +} + +int +main (int argc, char *argv[]) +{ + glfs_t *fs = NULL; + int ret = 0; + int i = 0; + glfs_fd_t *fd = NULL; + glfs_fd_t *fd1 = NULL; + char *topdir = "topdir", *filename = "file1"; + char *buf = NULL; + char *logfile = NULL; + + if (argc != 3) { + fprintf (stderr, + "Expect following args %s <Vol> <log file>\n" + , argv[0]); + return -1; + } + + logfile = argv[2]; + + for (i = 0; i < TEST_CASE_LOOP; i++) { + fs = glfs_new (argv[1]); + if (!fs) { + fprintf (stderr, "glfs_new: returned NULL (%s)\n", + strerror (errno)); + return -1; + } + + ret = glfs_set_volfile_server (fs, "tcp", "localhost", 24007); + if (ret < 0) { + fprintf (stderr, "glfs_set_volfile_server failed ret:%d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_set_logging (fs, logfile, 7); + if (ret < 0) { + fprintf (stderr, "glfs_set_logging failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_init (fs); + if (ret < 0) { + fprintf (stderr, "glfs_init failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = large_number_of_fops (fs); + if (ret < 0) + return -1; + + ret = large_read_write (fs); + if (ret < 0) + return -1; + + ret = volfile_change (argv[1]); + if (ret < 0) + return -1; + + ret = large_number_of_fops (fs); + if (ret < 0) + return -1; + + ret = large_read_write (fs); + if (ret < 0) + return -1; + + ret = glfs_fini (fs); + if (ret < 0) { + fprintf (stderr, "glfs_fini failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + } + return 0; +} diff --git a/tests/bugs/bug-1093594.t b/tests/bugs/bug-1093594.t new file mode 100644 index 00000000000..524c6240344 --- /dev/null +++ b/tests/bugs/bug-1093594.t @@ -0,0 +1,20 @@ +#!/bin/bash +. $(dirname $0)/../include.rc +. $(dirname $0)/../volume.rc + +cleanup; + +## Start and create a volume +TEST glusterd; +TEST pidof glusterd; +TEST $CLI volume info; + +TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2}; +TEST $CLI volume start $V0; +logdir=`gluster --print-logdir` + +build_tester $(dirname $0)/bug-1093594.c -lgfapi +TEST $(dirname $0)/bug-1093594 $V0 $logdir/bug-1093594.log + +cleanup_tester $(dirname $0)/bug-1093594 +cleanup; diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c index 04a90aae4c6..f954b1399db 100644 --- a/xlators/cluster/afr/src/afr-common.c +++ b/xlators/cluster/afr/src/afr-common.c @@ -4073,7 +4073,6 @@ afr_priv_destroy (afr_private_t *priv) if (!priv) goto out; - inode_unref (priv->root_inode); GF_FREE (priv->last_event); if (priv->pending_key) { for (i = 0; i < priv->child_count; i++) diff --git a/xlators/performance/io-cache/src/io-cache.c b/xlators/performance/io-cache/src/io-cache.c index facff5038a7..21acad02657 100644 --- a/xlators/performance/io-cache/src/io-cache.c +++ b/xlators/performance/io-cache/src/io-cache.c @@ -2062,7 +2062,6 @@ fini (xlator_t *this) { ioc_table_t *table = NULL; struct ioc_priority *curr = NULL, *tmp = NULL; - int i = 0; table = this->private; @@ -2082,11 +2081,15 @@ fini (xlator_t *this) GF_FREE (curr); } - for (i = 0; i < table->max_pri; i++) { + /* inode_lru and inodes list can be empty in case fini() is + * called soon after init()? Hence commenting the below asserts. + */ + /*for (i = 0; i < table->max_pri; i++) { GF_ASSERT (list_empty (&table->inode_lru[i])); } GF_ASSERT (list_empty (&table->inodes)); + */ pthread_mutex_destroy (&table->table_lock); GF_FREE (table); |