From 79e4c7b2fad6db15863efb4e979525b1bd4862ea Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Fri, 5 Jun 2015 10:33:11 +0530 Subject: stack: use list_head for managing frames PROBLEM -------- statedump requests that traverse call frames of all call stacks in execution may race with a STACK_RESET on a stack. This could crash the corresponding glusterfs process. For e.g, recently we observed this in a regression test case tests/basic/afr/sparse-self-heal.t. FIX --- gf_proc_dump_pending_frames takes a (TRY_LOCK) call_pool->lock before iterating through call frames of all call stacks in progress. With this fix, STACK_RESET removes its call frames under the same lock. Additional info ---------------- This fix makes call_stack_t to use struct list_head in place of custom doubly-linked list implementation. This makes call_frame_t manipulation easier to maintain in the context of STACK_WIND et al. BUG: 1229658 Change-Id: I7e43bccd3994cd9184ab982dba3dbc10618f0d94 Signed-off-by: Krishnan Parthasarathi Reviewed-on: http://review.gluster.org/11095 Reviewed-by: Niels de Vos Tested-by: Gluster Build System Reviewed-by: Pranith Kumar Karampuri Tested-by: NetBSD Build System --- libglusterfs/src/common-utils.c | 21 ++++----- libglusterfs/src/stack.c | 75 ++++++++++++++----------------- libglusterfs/src/stack.h | 98 ++++++++++++++++++++++++++--------------- 3 files changed, 103 insertions(+), 91 deletions(-) (limited to 'libglusterfs') diff --git a/libglusterfs/src/common-utils.c b/libglusterfs/src/common-utils.c index 48a1c41efcf..1b02ca5f217 100644 --- a/libglusterfs/src/common-utils.c +++ b/libglusterfs/src/common-utils.c @@ -568,6 +568,7 @@ gf_print_trace (int32_t signum, glusterfs_ctx_t *ctx) { char msg[1024] = {0,}; char timestr[64] = {0,}; + call_stack_t *stack = NULL; /* Now every gf_log call will just write to a buffer and when the * buffer becomes full, its written to the log-file. Suppose the process @@ -583,23 +584,19 @@ gf_print_trace (int32_t signum, glusterfs_ctx_t *ctx) /* Pending frames, (if any), list them in order */ gf_msg_plain_nomem (GF_LOG_ALERT, "pending frames:"); { - struct list_head *trav = - ((call_pool_t *)ctx->pool)->all_frames.next; - while (trav != (&((call_pool_t *)ctx->pool)->all_frames)) { - call_frame_t *tmp = - (call_frame_t *)(&((call_stack_t *)trav)->frames); - if (tmp->root->type == GF_OP_TYPE_FOP) + /* FIXME: traversing stacks outside pool->lock */ + list_for_each_entry (stack, &ctx->pool->all_frames, + all_frames) { + if (stack->type == GF_OP_TYPE_FOP) sprintf (msg,"frame : type(%d) op(%s)", - tmp->root->type, - gf_fop_list[tmp->root->op]); + stack->type, + gf_fop_list[stack->op]); else sprintf (msg,"frame : type(%d) op(%d)", - tmp->root->type, - tmp->root->op); + stack->type, + stack->op); gf_msg_plain_nomem (GF_LOG_ALERT, msg); - - trav = trav->next; } } diff --git a/libglusterfs/src/stack.c b/libglusterfs/src/stack.c index a7fb9510399..2488cc7f0ba 100644 --- a/libglusterfs/src/stack.c +++ b/libglusterfs/src/stack.c @@ -11,25 +11,11 @@ #include "statedump.h" #include "stack.h" -static inline -int call_frames_count (call_frame_t *call_frame) -{ - call_frame_t *pos; - int32_t count = 0; - - if (!call_frame) - return count; - - for (pos = call_frame; pos != NULL; pos = pos->next) - count++; - - return count; -} - call_frame_t * create_frame (xlator_t *xl, call_pool_t *pool) { call_stack_t *stack = NULL; + call_frame_t *frame = NULL; if (!xl || !pool) { return NULL; @@ -39,18 +25,31 @@ create_frame (xlator_t *xl, call_pool_t *pool) if (!stack) return NULL; + INIT_LIST_HEAD (&stack->myframes); + + frame = mem_get0 (pool->frame_mem_pool); + if (!frame) { + mem_put (stack); + return NULL; + } + + frame->root = stack; + frame->this = xl; + LOCK_INIT (&frame->lock); + INIT_LIST_HEAD (&frame->frames); + list_add (&frame->frames, &stack->myframes); + stack->pool = pool; - stack->frames.root = stack; - stack->frames.this = xl; stack->ctx = xl->ctx; if (stack->ctx->measure_latency) { if (gettimeofday (&stack->tv, NULL) == -1) gf_log ("stack", GF_LOG_ERROR, "gettimeofday () failed." " (%s)", strerror (errno)); - memcpy (&stack->frames.begin, &stack->tv, sizeof (stack->tv)); + memcpy (&frame->begin, &stack->tv, sizeof (stack->tv)); } + LOCK (&pool->lock); { list_add (&stack->all_frames, &pool->all_frames); @@ -58,10 +57,9 @@ create_frame (xlator_t *xl, call_pool_t *pool) } UNLOCK (&pool->lock); - LOCK_INIT (&stack->frames.lock); LOCK_INIT (&stack->stack_lock); - return &stack->frames; + return frame; } void @@ -136,7 +134,7 @@ gf_proc_dump_call_stack (call_stack_t *call_stack, const char *key_buf,...) char prefix[GF_DUMP_MAX_BUF_LEN]; va_list ap; call_frame_t *trav; - int32_t cnt, i; + int32_t i = 1, cnt = 0; char timestr[256] = {0,}; if (!call_stack) @@ -144,13 +142,12 @@ gf_proc_dump_call_stack (call_stack_t *call_stack, const char *key_buf,...) GF_ASSERT (key_buf); - cnt = call_frames_count(&call_stack->frames); - memset(prefix, 0, sizeof(prefix)); va_start(ap, key_buf); vsnprintf(prefix, GF_DUMP_MAX_BUF_LEN, key_buf, ap); va_end(ap); + cnt = call_frames_count (call_stack); if (call_stack->ctx->measure_latency) { gf_time_fmt (timestr, sizeof timestr, call_stack->tv.tv_sec, gf_timefmt_FT); @@ -176,14 +173,10 @@ gf_proc_dump_call_stack (call_stack_t *call_stack, const char *key_buf,...) gf_proc_dump_write("type", "%d", call_stack->type); gf_proc_dump_write("cnt", "%d", cnt); - trav = &call_stack->frames; - - for (i = 1; i <= cnt; i++) { - if (trav) { - gf_proc_dump_add_section("%s.frame.%d", prefix, i); - gf_proc_dump_call_frame(trav, "%s.frame.%d", prefix, i); - trav = trav->next; - } + list_for_each_entry (trav, &call_stack->myframes, frames) { + gf_proc_dump_add_section("%s.frame.%d", prefix, i); + gf_proc_dump_call_frame(trav, "%s.frame.%d", prefix, i); + i++; } } @@ -317,14 +310,13 @@ gf_proc_dump_call_stack_to_dict (call_stack_t *call_stack, int ret = -1; char key[GF_DUMP_MAX_BUF_LEN] = {0,}; call_frame_t *trav = NULL; - int count = 0; int i = 0; + int count = 0; if (!call_stack || !dict) return; - count = call_frames_count (&call_stack->frames); - + count = call_frames_count (call_stack); memset (key, 0, sizeof (key)); snprintf (key, sizeof (key), "%s.uid", prefix); ret = dict_set_int32 (dict, key, call_stack->uid); @@ -372,15 +364,12 @@ gf_proc_dump_call_stack_to_dict (call_stack_t *call_stack, if (ret) return; - trav = &call_stack->frames; - for (i = 0; i < count; i++) { - if (trav) { - memset (key, 0, sizeof (key)); - snprintf (key, sizeof (key), "%s.frame%d", - prefix, i); - gf_proc_dump_call_frame_to_dict (trav, key, dict); - trav = trav->next; - } + list_for_each_entry (trav, &call_stack->myframes, frames) { + memset (key, 0, sizeof (key)); + snprintf (key, sizeof (key), "%s.frame%d", + prefix, i); + gf_proc_dump_call_frame_to_dict (trav, key, dict); + i++; } return; diff --git a/libglusterfs/src/stack.h b/libglusterfs/src/stack.h index e0cc45eeb60..ffe5c1a1085 100644 --- a/libglusterfs/src/stack.h +++ b/libglusterfs/src/stack.h @@ -59,8 +59,7 @@ struct call_pool { struct _call_frame_t { call_stack_t *root; /* stack root */ call_frame_t *parent; /* previous BP */ - call_frame_t *next; - call_frame_t *prev; /* maintenance list */ + struct list_head frames; void *local; /* local variables */ xlator_t *this; /* implicit object */ ret_fn_t ret; /* op_return address */ @@ -103,7 +102,8 @@ struct _call_stack_t { gf_lkowner_t lk_owner; glusterfs_ctx_t *ctx; - call_frame_t frames; + struct list_head myframes; /* List of call_frame_t that go + to make the call stack */ int32_t op; int8_t type; @@ -133,10 +133,8 @@ static inline void FRAME_DESTROY (call_frame_t *frame) { void *local = NULL; - if (frame->next) - frame->next->prev = frame->prev; - if (frame->prev) - frame->prev->next = frame->next; + + list_del_init (&frame->frames); if (frame->local) { local = frame->local; frame->local = NULL; @@ -155,6 +153,8 @@ static inline void STACK_DESTROY (call_stack_t *stack) { void *local = NULL; + call_frame_t *frame = NULL; + call_frame_t *tmp = NULL; LOCK (&stack->pool->lock); { @@ -163,16 +163,10 @@ STACK_DESTROY (call_stack_t *stack) } UNLOCK (&stack->pool->lock); - if (stack->frames.local) { - local = stack->frames.local; - stack->frames.local = NULL; - } - - LOCK_DESTROY (&stack->frames.lock); LOCK_DESTROY (&stack->stack_lock); - while (stack->frames.next) { - FRAME_DESTROY (stack->frames.next); + list_for_each_entry_safe (frame, tmp, &stack->myframes, frames) { + FRAME_DESTROY (frame); } GF_FREE (stack->groups_large); @@ -187,14 +181,28 @@ static inline void STACK_RESET (call_stack_t *stack) { void *local = NULL; + call_frame_t *frame = NULL; + call_frame_t *tmp = NULL; + call_frame_t *last = NULL; + struct list_head toreset = {0}; + + INIT_LIST_HEAD (&toreset); + + /* We acquire call_pool->lock only to remove the frames from this stack + * to preserve atomicity. This synchronizes across concurrent requests + * like statedump, STACK_DESTROY etc. */ - if (stack->frames.local) { - local = stack->frames.local; - stack->frames.local = NULL; + LOCK (&stack->pool->lock); + { + last = list_last_entry (&stack->myframes, call_frame_t, frames); + list_del_init (&last->frames); + list_splice_init (&stack->myframes, &toreset); + list_add (&last->frames, &stack->myframes); } + UNLOCK (&stack->pool->lock); - while (stack->frames.next) { - FRAME_DESTROY (stack->frames.next); + list_for_each_entry_safe (frame, tmp, &toreset, frames) { + FRAME_DESTROY (frame); } if (local) @@ -244,11 +252,7 @@ STACK_RESET (call_stack_t *stack) LOCK_INIT (&_new->lock); \ LOCK(&frame->root->stack_lock); \ { \ - _new->next = frame->root->frames.next; \ - _new->prev = &frame->root->frames; \ - if (frame->root->frames.next) \ - frame->root->frames.next->prev = _new; \ - frame->root->frames.next = _new; \ + list_add (&_new->frames, &frame->root->myframes);\ frame->ref_count++; \ } \ UNLOCK(&frame->root->stack_lock); \ @@ -298,12 +302,8 @@ STACK_RESET (call_stack_t *stack) LOCK_INIT (&_new->lock); \ LOCK(&frame->root->stack_lock); \ { \ + list_add (&_new->frames, &frame->root->myframes);\ frame->ref_count++; \ - _new->next = frame->root->frames.next; \ - _new->prev = &frame->root->frames; \ - if (frame->root->frames.next) \ - frame->root->frames.next->prev = _new; \ - frame->root->frames.next = _new; \ } \ UNLOCK(&frame->root->stack_lock); \ fn##_cbk = rfn; \ @@ -391,11 +391,27 @@ call_stack_alloc_groups (call_stack_t *stack, int ngrps) return 0; } +static inline +int call_frames_count (call_stack_t *call_stack) +{ + call_frame_t *pos; + int32_t count = 0; + + if (!call_stack) + return count; + + list_for_each_entry (pos, &call_stack->myframes, frames) + count++; + + return count; +} + static inline call_frame_t * copy_frame (call_frame_t *frame) { call_stack_t *newstack = NULL; call_stack_t *oldstack = NULL; + call_frame_t *newframe = NULL; if (!frame) { return NULL; @@ -406,6 +422,19 @@ copy_frame (call_frame_t *frame) return NULL; } + INIT_LIST_HEAD (&newstack->myframes); + + newframe = mem_get0 (frame->root->pool->frame_mem_pool); + if (!newframe) { + mem_put (newstack); + return NULL; + } + + newframe->this = frame->this; + newframe->root = newstack; + INIT_LIST_HEAD (&newframe->frames); + list_add (&newframe->frames, &newstack->myframes); + oldstack = frame->root; newstack->uid = oldstack->uid; @@ -421,9 +450,6 @@ copy_frame (call_frame_t *frame) memcpy (newstack->groups, oldstack->groups, sizeof (gid_t) * oldstack->ngrps); newstack->unique = oldstack->unique; - - newstack->frames.this = frame->this; - newstack->frames.root = newstack; newstack->pool = oldstack->pool; newstack->lk_owner = oldstack->lk_owner; newstack->ctx = oldstack->ctx; @@ -432,11 +458,11 @@ copy_frame (call_frame_t *frame) if (gettimeofday (&newstack->tv, NULL) == -1) gf_log ("stack", GF_LOG_ERROR, "gettimeofday () failed." " (%s)", strerror (errno)); - memcpy (&newstack->frames.begin, &newstack->tv, + memcpy (&newframe->begin, &newstack->tv, sizeof (newstack->tv)); } - LOCK_INIT (&newstack->frames.lock); + LOCK_INIT (&newframe->lock); LOCK_INIT (&newstack->stack_lock); LOCK (&oldstack->pool->lock); @@ -446,7 +472,7 @@ copy_frame (call_frame_t *frame) } UNLOCK (&oldstack->pool->lock); - return &newstack->frames; + return newframe; } void gf_proc_dump_pending_frames(call_pool_t *call_pool); -- cgit