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>

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");