summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKrishnan Parthasarathi <kparthas@redhat.com>2015-06-05 10:33:11 +0530
committerNiels de Vos <ndevos@redhat.com>2015-06-22 00:41:43 -0700
commit79e4c7b2fad6db15863efb4e979525b1bd4862ea (patch)
tree5bcf810a74536b5751213661ebf90370b02d6ff3
parent6b4add6b3f54b6d3202535a492eaf906d619ad75 (diff)
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 <kparthas@redhat.com> Reviewed-on: http://review.gluster.org/11095 Reviewed-by: Niels de Vos <ndevos@redhat.com> Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com> Tested-by: NetBSD Build System <jenkins@build.gluster.org>
-rw-r--r--libglusterfs/src/common-utils.c21
-rw-r--r--libglusterfs/src/stack.c75
-rw-r--r--libglusterfs/src/stack.h98
-rw-r--r--xlators/features/protect/src/prot_client.c7
-rw-r--r--xlators/meta/src/frames-file.c6
5 files changed, 109 insertions, 98 deletions
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);
diff --git a/xlators/features/protect/src/prot_client.c b/xlators/features/protect/src/prot_client.c
index dde3abdadc8..79636410b94 100644
--- a/xlators/features/protect/src/prot_client.c
+++ b/xlators/features/protect/src/prot_client.c
@@ -35,10 +35,9 @@ pcli_print_trace (char *name, call_frame_t *frame)
int i;
gf_log (name, GF_LOG_INFO, "Translator stack:");
- while (frame) {
+ list_for_each_entry (frame, &frame->root->myframes, frames) {
gf_log (name, GF_LOG_INFO, "%s (%s)",
frame->wind_from, frame->this->name);
- frame = frame->next;
}
size = backtrace (frames, NUM_FRAMES);
@@ -77,7 +76,7 @@ pcli_rename (call_frame_t *frame, xlator_t *this, loc_t *oldloc,
if (value != PROT_ACT_NONE) {
gf_log (this->name, GF_LOG_WARNING,
"got rename for protected %s", oldloc->path);
- pcli_print_trace (this->name, frame->next);
+ pcli_print_trace (this->name, frame);
if (value == PROT_ACT_REJECT) {
STACK_UNWIND_STRICT (rename, frame, -1, EPERM,
NULL, NULL, NULL, NULL, NULL,
@@ -161,7 +160,7 @@ pcli_unlink (call_frame_t *frame, xlator_t *this, loc_t *loc, int xflag,
if (value != PROT_ACT_NONE) {
gf_log (this->name, GF_LOG_WARNING,
"got unlink for protected %s", loc->path);
- pcli_print_trace(this->name,frame->next);
+ pcli_print_trace(this->name, frame);
if (value == PROT_ACT_REJECT) {
STACK_UNWIND_STRICT (unlink, frame, -1, EPERM,
NULL, NULL, NULL);
diff --git a/xlators/meta/src/frames-file.c b/xlators/meta/src/frames-file.c
index 7d48d7a62d4..ebac3d9cbaa 100644
--- a/xlators/meta/src/frames-file.c
+++ b/xlators/meta/src/frames-file.c
@@ -39,8 +39,7 @@ frames_file_fill (xlator_t *this, inode_t *file, strfd_t *strfd)
strprintf (strfd, "\t\t\"Number\": %d,\n", ++i);
strprintf (strfd, "\t\t\"Frame\": [\n");
j = 1;
- for (frame = &stack->frames; frame;
- frame = frame->next) {
+ list_for_each_entry (frame, &stack->myframes, frames) {
strprintf (strfd, "\t\t {\n");
strprintf (strfd, "\t\t\t\"Number\": %d,\n",
j++);
@@ -71,7 +70,8 @@ frames_file_fill (xlator_t *this, inode_t *file, strfd_t *strfd)
frame->unwind_to);
strprintf (strfd, "\t\t\t\"Complete\": %d\n",
frame->complete);
- if (frame->next == NULL)
+ if (list_is_last (&frame->frames,
+ &stack->myframes))
strprintf (strfd, "\t\t }\n");
else
strprintf (strfd, "\t\t },\n");