From ed98a2c88a54dd14790632f97abf61403557ab7a Mon Sep 17 00:00:00 2001

From: Jeff Darcy <jdarcy@redhat.com>

Date: Mon, 20 Mar 2017 12:32:33 -0400

Subject: [PATCH 366/369] glusterd: hold off volume deletes while still

 restarting bricks

We need to do this because modifying the volume/brick tree while

glusterd_restart_bricks is still walking it can lead to segfaults.

Without waiting we could accidentally "slip in" while attach_brick has

released big_lock between retries and make such a modification.

>Reviewed-on: https://review.gluster.org/16927

>Smoke: Gluster Build System <jenkins@build.gluster.org>

>NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>

>CentOS-regression: Gluster Build System <jenkins@build.gluster.org>

>Reviewed-by: Atin Mukherjee <amukherj@redhat.com>

Change-Id: I30ccc4efa8d286aae847250f5d4fb28956a74b03

BUG: 1438052

Signed-off-by: Jeff Darcy <jeff@pl.atyp.us>

Reviewed-on: https://code.engineering.redhat.com/gerrit/102298

Reviewed-by: Atin Mukherjee <amukherj@redhat.com>

Tested-by: Atin Mukherjee <amukherj@redhat.com>

---

 ...ports.t => bug-1421590-brick-mux-reuse-ports.t} |  5 ++

 tests/bugs/core/bug-1432542-mpx-restart-crash.t    | 91 ++++++++++++++++++++++

 xlators/mgmt/glusterd/src/glusterd-op-sm.c         | 15 ++++

 xlators/mgmt/glusterd/src/glusterd-utils.c         | 39 +++++++---

 xlators/mgmt/glusterd/src/glusterd-volume-ops.c    |  3 -

 xlators/mgmt/glusterd/src/glusterd.c               |  2 +-

 xlators/mgmt/glusterd/src/glusterd.h               |  1 +

 7 files changed, 141 insertions(+), 15 deletions(-)

 rename tests/bugs/core/{bug-1421590-brick-mux-resuse-ports.t => bug-1421590-brick-mux-reuse-ports.t} (86%)

 create mode 100644 tests/bugs/core/bug-1432542-mpx-restart-crash.t

diff --git a/tests/bugs/core/bug-1421590-brick-mux-resuse-ports.t b/tests/bugs/core/bug-1421590-brick-mux-reuse-ports.t

similarity index 86%

rename from tests/bugs/core/bug-1421590-brick-mux-resuse-ports.t

rename to tests/bugs/core/bug-1421590-brick-mux-reuse-ports.t

index ed401f6..a227f82 100644

--- a/tests/bugs/core/bug-1421590-brick-mux-resuse-ports.t

+++ b/tests/bugs/core/bug-1421590-brick-mux-reuse-ports.t

@@ -21,6 +21,11 @@ push_trapfunc "cleanup"

 TEST $CLI volume create $V0 $H0:$B0/brick{0,1}

 TEST $CLI volume start $V0

+# We can't expect a valid port number instantly.  We need to wait for the

+# bricks to finish coming up.  In every other case we use EXPECT_WITHIN, but

+# this first time we need to wait more explicitly.

+sleep $PROCESS_UP_TIMEOUT

+

 port_brick0=$(get_nth_brick_port_for_volume $V0 1)

 # restart the volume

diff --git a/tests/bugs/core/bug-1432542-mpx-restart-crash.t b/tests/bugs/core/bug-1432542-mpx-restart-crash.t

new file mode 100644

index 0000000..970a181

--- /dev/null

+++ b/tests/bugs/core/bug-1432542-mpx-restart-crash.t

@@ -0,0 +1,91 @@

+#!/bin/bash

+

+. $(dirname $0)/../../include.rc

+. $(dirname $0)/../../volume.rc

+. $(dirname $0)/../../traps.rc

+

+NUM_VOLS=20

+MOUNT_BASE=$(dirname $M0)

+

+# GlusterD reports that bricks are started when in fact their attach requests

+# might still need to be retried.  That's a bit of a hack, but there's no

+# feasible way to wait at that point (in attach_brick) and the rest of the

+# code is unprepared to deal with transient errors so the whole "brick start"

+# would fail.  Meanwhile, glusterfsd can only handle attach requests at a

+# rather slow rate.  After GlusterD tries to start a couple of hundred bricks,

+# glusterfsd can fall behind and we start getting mount failures.  Arguably,

+# those are spurious because we will eventually catch up.  We're just not

+# ready *yet*.  More to the point, even if the errors aren't spurious that's

+# not what we're testing right now.  Therefore, we give glusterfsd a bit more

+# breathing room for this test than we would otherwise.

+MOUNT_TIMEOUT=15

+

+get_brick_base () {

+	printf "%s/vol%02d" $B0 $1

+}

+

+get_mount_point () {

+	printf "%s/vol%02d" $MOUNT_BASE $1

+}

+

+create_volume () {

+

+	local vol_name=$(printf "%s-vol%02d" $V0 $1)

+

+	local brick_base=$(get_brick_base $1)

+	local cmd="$CLI volume create $vol_name replica 2"

+	local b

+	for b in $(seq 0 5); do

+		local this_brick=${brick_base}/brick$b

+		mkdir -p $this_brick

+		cmd="$cmd $H0:$this_brick"

+	done

+	TEST $cmd

+	TEST $CLI volume start $vol_name

+	EXPECT_WITHIN $PROCESS_UP_TIMEOUT "Started" volinfo_field $vol_name "Status"

+	local mount_point=$(get_mount_point $1)

+	mkdir -p $mount_point

+	TEST $GFS -s $H0 --volfile-id=$vol_name $mount_point

+}

+

+cleanup_func () {

+	local v

+	for v in $(seq 1 $NUM_VOLS); do

+		local mount_point=$(get_mount_point $v)

+		force_umount $mount_point

+		rm -rf $mount_point

+		local vol_name=$(printf "%s-vol%02d" $V0 $v)

+		$CLI volume stop $vol_name

+		$CLI volume delete $vol_name

+		rm -rf $(get_brick_base $1) &

+	done &> /dev/null

+	wait

+}

+push_trapfunc cleanup_func

+

+TEST glusterd

+TEST $CLI volume set all cluster.brick-multiplex on

+

+# Our infrastructure can't handle an arithmetic expression here.  The formula

+# is (NUM_VOLS-1)*5 because it sees each TEST/EXPECT once but needs the other

+# NUM_VOLS-1 and there are 5 such statements in each iteration.

+TESTS_EXPECTED_IN_LOOP=95

+for i in $(seq 1 $NUM_VOLS); do

+	create_volume $i

+	TEST dd if=/dev/zero of=$(get_mount_point $i)/a_file bs=4k count=1

+done

+

+# Kill glusterd, and wait a bit for all traces to disappear.

+TEST killall -9 glusterd

+sleep 5

+TEST killall -9 glusterfsd

+sleep 5

+

+# Restart glusterd.  This is where the brick daemon supposedly dumps core,

+# though I (jdarcy) have yet to see that.  Again, give it a while to settle,

+# just to be sure.

+TEST glusterd

+

+cleanup_func

+trap - EXIT

+cleanup

diff --git a/xlators/mgmt/glusterd/src/glusterd-op-sm.c b/xlators/mgmt/glusterd/src/glusterd-op-sm.c

index 98ae7b6..dae6c1e 100644

--- a/xlators/mgmt/glusterd/src/glusterd-op-sm.c

+++ b/xlators/mgmt/glusterd/src/glusterd-op-sm.c

@@ -5935,6 +5935,15 @@ glusterd_op_stage_validate (glusterd_op_t op, dict_t *dict, char **op_errstr,

         return ret;

 }

+static void

+glusterd_wait_for_blockers (glusterd_conf_t *priv)

+{

+        while (priv->blockers) {

+                synclock_unlock (&priv->big_lock);

+                sleep (1);

+                synclock_lock (&priv->big_lock);

+        }

+}

 int32_t

 glusterd_op_commit_perform (glusterd_op_t op, dict_t *dict, char **op_errstr,

@@ -5954,18 +5963,22 @@ glusterd_op_commit_perform (glusterd_op_t op, dict_t *dict, char **op_errstr,

                         break;

                 case GD_OP_STOP_VOLUME:

+                        glusterd_wait_for_blockers (this->private);

                         ret = glusterd_op_stop_volume (dict);

                         break;

                 case GD_OP_DELETE_VOLUME:

+                        glusterd_wait_for_blockers (this->private);

                         ret = glusterd_op_delete_volume (dict);

                         break;

                 case GD_OP_ADD_BRICK:

+                        glusterd_wait_for_blockers (this->private);

                         ret = glusterd_op_add_brick (dict, op_errstr);

                         break;

                 case GD_OP_REPLACE_BRICK:

+                        glusterd_wait_for_blockers (this->private);

                         ret = glusterd_op_replace_brick (dict, rsp_dict);

                         break;

@@ -5976,11 +5989,13 @@ glusterd_op_commit_perform (glusterd_op_t op, dict_t *dict, char **op_errstr,

                         ret = glusterd_op_set_ganesha (dict, op_errstr);

                         break;

+

                 case GD_OP_RESET_VOLUME:

                         ret = glusterd_op_reset_volume (dict, op_errstr);

                         break;

                 case GD_OP_REMOVE_BRICK:

+                        glusterd_wait_for_blockers (this->private);

                         ret = glusterd_op_remove_brick (dict, op_errstr);

                         break;

diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c

index 77f97a5..2adfb47 100644

--- a/xlators/mgmt/glusterd/src/glusterd-utils.c

+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c

@@ -3123,8 +3123,8 @@ out:

 int

 glusterd_spawn_daemons (void *opaque)

 {

-        glusterd_conf_t *conf = THIS->private;

-        int             ret             = -1;

+        glusterd_conf_t *conf   = THIS->private;

+        int             ret     = -1;

         synclock_lock (&conf->big_lock);

         glusterd_restart_bricks (conf);

@@ -4891,9 +4891,13 @@ static int32_t

 my_callback (struct rpc_req *req, struct iovec *iov, int count, void *v_frame)

 {

         call_frame_t    *frame  = v_frame;

+        glusterd_conf_t *conf   = frame->this->private;

-        STACK_DESTROY (frame->root);

+        synclock_lock (&conf->big_lock);

+        --(conf->blockers);

+        synclock_unlock (&conf->big_lock);

+        STACK_DESTROY (frame->root);

         return 0;

 }

@@ -4910,6 +4914,7 @@ send_attach_req (xlator_t *this, struct rpc_clnt *rpc, char *path, int op)

         void                            *req = &brick_req;

         void                            *errlbl   = &&err;

         struct rpc_clnt_connection      *conn;

+        glusterd_conf_t                 *conf     = this->private;

         extern struct rpc_clnt_program  gd_brick_prog;

         if (!rpc) {

@@ -4969,9 +4974,13 @@ send_attach_req (xlator_t *this, struct rpc_clnt *rpc, char *path, int op)

         iov.iov_len = ret;

         /* Send the msg */

+        ++(conf->blockers);

         ret = rpc_clnt_submit (rpc, &gd_brick_prog, op,

-                               my_callback, &iov, 1, NULL, 0, iobref, frame,

-                               NULL, 0, NULL, 0, NULL);

+                               my_callback, &iov, 1, NULL, 0, iobref,

+                               frame, NULL, 0, NULL, 0, NULL);

+        if (ret) {

+                --(conf->blockers);

+        }

         return ret;

 free_iobref:

@@ -5003,6 +5012,8 @@ attach_brick (xlator_t *this,

         char            full_id[PATH_MAX]       = {'\0',};

         char            path[PATH_MAX]          = {'\0',};

         int             ret;

+        int             tries;

+        rpc_clnt_t      *rpc;

         gf_log (this->name, GF_LOG_INFO,

                 "add brick %s to existing process for %s",

@@ -5039,12 +5050,15 @@ attach_brick (xlator_t *this,

         }

         (void) build_volfile_path (full_id, path, sizeof(path), NULL);

-        int tries = 0;

-        while (tries++ <= 15) {

-                ret = send_attach_req (this, other_brick->rpc, path,

-                                       GLUSTERD_BRICK_ATTACH);

-                if (!ret) {

-                        return 0;

+        for (tries = 15; tries > 0; --tries) {

+                rpc = rpc_clnt_ref (other_brick->rpc);

+                if (rpc) {

+                        ret = send_attach_req (this, rpc, path,

+                                               GLUSTERD_BRICK_ATTACH);

+                        rpc_clnt_unref (rpc);

+                        if (!ret) {

+                                return 0;

+                        }

                 }

                 /*

                  * It might not actually be safe to manipulate the lock like

@@ -5410,6 +5424,8 @@ glusterd_restart_bricks (glusterd_conf_t *conf)

         conf = this->private;

         GF_VALIDATE_OR_GOTO (this->name, conf, out);

+        ++(conf->blockers);

+

         ret = glusterd_get_quorum_cluster_counts (this, &active_count,

                                                   &quorum_count);

         if (ret)

@@ -5489,6 +5505,7 @@ glusterd_restart_bricks (glusterd_conf_t *conf)

         ret = 0;

 out:

+        --(conf->blockers);

         conf->restart_done = _gf_true;

         return ret;

 }

diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c

index 3941b06..08906ba 100644

--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c

+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c

@@ -2788,14 +2788,11 @@ glusterd_op_delete_volume (dict_t *dict)

 {

         int                                     ret = 0;

         char                                    *volname = NULL;

-        glusterd_conf_t                         *priv = NULL;

         glusterd_volinfo_t                      *volinfo = NULL;

         xlator_t                                *this = NULL;

         this = THIS;

         GF_ASSERT (this);

-        priv = this->private;

-        GF_ASSERT (priv);

         ret = dict_get_str (dict, "volname", &volname);

         if (ret) {

diff --git a/xlators/mgmt/glusterd/src/glusterd.c b/xlators/mgmt/glusterd/src/glusterd.c

index ae81de6..7d66718 100644

--- a/xlators/mgmt/glusterd/src/glusterd.c

+++ b/xlators/mgmt/glusterd/src/glusterd.c

@@ -1869,10 +1869,10 @@ init (xlator_t *this)

         if (ret < 0)

                 goto out;

+        conf->blockers = 0;

         /* If there are no 'friends', this would be the best time to

          * spawn process/bricks that may need (re)starting since last

          * time (this) glusterd was up.*/

-

         if (cds_list_empty (&conf->peers)) {

                 glusterd_launch_synctask (glusterd_spawn_daemons, NULL);

         }

diff --git a/xlators/mgmt/glusterd/src/glusterd.h b/xlators/mgmt/glusterd/src/glusterd.h

index d80ad20..671ec58 100644

--- a/xlators/mgmt/glusterd/src/glusterd.h

+++ b/xlators/mgmt/glusterd/src/glusterd.h

@@ -184,6 +184,7 @@ typedef struct {

         int                        ping_timeout;

         uint32_t                   generation;

         int32_t                    workers;

+        uint32_t                   blockers;

 } glusterd_conf_t;

-- 

1.8.3.1