From c085871e3919df2b309b76633e75d5449899437a Mon Sep 17 00:00:00 2001 From: Jeff Darcy Date: Tue, 28 Apr 2015 04:40:00 -0400 Subject: core: use reference counting for mem_acct structures When freeing memory, our memory-accounting code expects to be able to dereference from the (previously) allocated block to its owning translator. However, as we have already found once in option validation and twice in logging, that translator might itself have been freed and the dereference attempt causes on of our daemons to crash with SIGSEGV. This patch attempts to fix that as follows: * We no longer embed a struct mem_acct directly in a struct xlator, but instead allocate it separately. * Allocated memory blocks now contain a pointer to the mem_acct instead of the xlator. * The mem_acct structure contains a reference count, manipulated in both the normal and translator allocate/free code using atomic increments and decrements. * Because it's now a separate structure, we can defer freeing the mem_acct until its reference count reaches zero (either way). * Some unit tests were disabled, because they embedded their own copies of the implementation for what they were supposedly testing. Life's too short to spend time fixing tests that seem designed to impede progress by requiring a certain implementation as well as behavior. Change-Id: Id929b11387927136f78626901729296b6c0d0fd7 BUG: 1211749 Signed-off-by: Jeff Darcy Reviewed-on: http://review.gluster.org/10417 Tested-by: Gluster Build System Reviewed-by: Krishnan Parthasarathi Reviewed-by: Niels de Vos Reviewed-by: Pranith Kumar Karampuri --- libglusterfs/src/unittest/mem_pool_unittest.c | 99 ++++++++++++++------------- 1 file changed, 50 insertions(+), 49 deletions(-) (limited to 'libglusterfs/src/unittest') diff --git a/libglusterfs/src/unittest/mem_pool_unittest.c b/libglusterfs/src/unittest/mem_pool_unittest.c index 906ebb0a7f3..00c7688637f 100644 --- a/libglusterfs/src/unittest/mem_pool_unittest.c +++ b/libglusterfs/src/unittest/mem_pool_unittest.c @@ -58,19 +58,20 @@ helper_xlator_init(uint32_t num_types) xl = test_calloc(1, sizeof(xlator_t)); assert_non_null(xl); - xl->mem_acct.num_types = num_types; - xl->mem_acct.rec = test_calloc(num_types, sizeof(struct mem_acct_rec)); - assert_non_null(xl->mem_acct.rec); + xl->mem_acct->num_types = num_types; + xl->mem_acct = test_calloc (sizeof(struct mem_acct) + + sizeof(struct mem_acct_rec) * num_types); + assert_non_null(xl->mem_acct); xl->ctx = test_calloc(1, sizeof(glusterfs_ctx_t)); assert_non_null(xl->ctx); for (i = 0; i < num_types; i++) { - ret = LOCK_INIT(&(xl->mem_acct.rec[i].lock)); + ret = LOCK_INIT(&(xl->mem_acct->rec[i].lock)); assert_int_equal(ret, 0); } - ENSURE(num_types == xl->mem_acct.num_types); + ENSURE(num_types == xl->mem_acct->num_types); ENSURE(NULL != xl); return xl; @@ -81,12 +82,12 @@ helper_xlator_destroy(xlator_t *xl) { int i, ret; - for (i = 0; i < xl->mem_acct.num_types; i++) { - ret = LOCK_DESTROY(&(xl->mem_acct.rec[i].lock)); + for (i = 0; i < xl->mem_acct->num_types; i++) { + ret = LOCK_DESTROY(&(xl->mem_acct->rec[i].lock)); assert_int_equal(ret, 0); } - free(xl->mem_acct.rec); + free(xl->mem_acct->rec); free(xl->ctx); free(xl); return 0; @@ -145,9 +146,9 @@ test_gf_mem_set_acct_info_asserts(void **state) // Check xl is NULL expect_assert_failure(gf_mem_set_acct_info(NULL, &alloc_ptr, size, type, "")); - // Check xl->mem_acct.rec = NULL + // Check xl->mem_acct = NULL expect_assert_failure(gf_mem_set_acct_info(&xltest, &alloc_ptr, 0, type, "")); - // Check type <= xl->mem_acct.num_types + // Check type <= xl->mem_acct->num_types type = 100; expect_assert_failure(gf_mem_set_acct_info(&xltest, &alloc_ptr, 0, type, "")); // Check alloc is NULL @@ -158,8 +159,8 @@ test_gf_mem_set_acct_info_asserts(void **state) // Test number of types type = 100; - assert_true(NULL != xl->mem_acct.rec); - assert_true(type > xl->mem_acct.num_types); + assert_true(NULL != xl->mem_acct); + assert_true(type > xl->mem_acct->num_types); expect_assert_failure(gf_mem_set_acct_info(xl, &alloc_ptr, size, type, "")); helper_xlator_destroy(xl); @@ -180,7 +181,7 @@ test_gf_mem_set_acct_info_memory(void **state) // Initialize xl xl = helper_xlator_init(10); - assert_null(xl->mem_acct.rec[type].typestr); + assert_null(xl->mem_acct->rec[type].typestr); // Test allocation temp_ptr = test_calloc(1, size + GF_MEM_HEADER_SIZE + GF_MEM_TRAILER_SIZE); @@ -189,12 +190,12 @@ test_gf_mem_set_acct_info_memory(void **state) gf_mem_set_acct_info(xl, &alloc_ptr, size, type, typestr); //Check values - assert_ptr_equal(typestr, xl->mem_acct.rec[type].typestr); - assert_int_equal(xl->mem_acct.rec[type].size, size); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].max_size, size); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 1); + assert_ptr_equal(typestr, xl->mem_acct->rec[type].typestr); + assert_int_equal(xl->mem_acct->rec[type].size, size); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].max_size, size); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 1); // Check memory helper_check_memory_headers(temp_ptr, xl, size, type); @@ -235,11 +236,11 @@ test_gf_calloc_default_calloc(void **state) memset(mem, 0x5A, size); // Check xl did not change - assert_int_equal(xl->mem_acct.rec[type].size, 0); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].max_size, 0); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].size, 0); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].max_size, 0); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 0); free(mem); helper_xlator_destroy(xl); @@ -269,11 +270,11 @@ test_gf_calloc_mem_acct_enabled(void **state) memset(mem, 0x5A, size); // Check xl values - assert_int_equal(xl->mem_acct.rec[type].size, size); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].max_size, size); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].size, size); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].max_size, size); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 1); // Check memory helper_check_memory_headers(mem - sizeof(mem_header_t), xl, size, type); @@ -302,11 +303,11 @@ test_gf_malloc_default_malloc(void **state) memset(mem, 0x5A, size); // Check xl did not change - assert_int_equal(xl->mem_acct.rec[type].size, 0); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].max_size, 0); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].size, 0); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].max_size, 0); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 0); free(mem); helper_xlator_destroy(xl); @@ -336,11 +337,11 @@ test_gf_malloc_mem_acct_enabled(void **state) memset(mem, 0x5A, size); // Check xl values - assert_int_equal(xl->mem_acct.rec[type].size, size); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 1); - assert_int_equal(xl->mem_acct.rec[type].max_size, size); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].size, size); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 1); + assert_int_equal(xl->mem_acct->rec[type].max_size, size); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 1); // Check memory helper_check_memory_headers(mem - sizeof(mem_header_t), xl, size, type); @@ -374,11 +375,11 @@ test_gf_realloc_default_realloc(void **state) memset(mem, 0x5A, size); // Check xl did not change - assert_int_equal(xl->mem_acct.rec[type].size, 0); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 0); - assert_int_equal(xl->mem_acct.rec[type].max_size, 0); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].size, 0); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 0); + assert_int_equal(xl->mem_acct->rec[type].max_size, 0); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 0); free(mem); helper_xlator_destroy(xl); @@ -419,11 +420,11 @@ test_gf_realloc_mem_acct_enabled(void **state) // not to the realloc + the malloc. // Is this a bug? // - assert_int_equal(xl->mem_acct.rec[type].size, size+1024); - assert_int_equal(xl->mem_acct.rec[type].num_allocs, 2); - assert_int_equal(xl->mem_acct.rec[type].total_allocs, 2); - assert_int_equal(xl->mem_acct.rec[type].max_size, size+1024); - assert_int_equal(xl->mem_acct.rec[type].max_num_allocs, 2); + assert_int_equal(xl->mem_acct->rec[type].size, size+1024); + assert_int_equal(xl->mem_acct->rec[type].num_allocs, 2); + assert_int_equal(xl->mem_acct->rec[type].total_allocs, 2); + assert_int_equal(xl->mem_acct->rec[type].max_size, size+1024); + assert_int_equal(xl->mem_acct->rec[type].max_num_allocs, 2); // Check memory helper_check_memory_headers(mem - sizeof(mem_header_t), xl, size, type); -- cgit