From 938ee38c02cce2a743c672f9c03798ebcbb1e348 Mon Sep 17 00:00:00 2001
From: Atin Mukherjee <amukherj@redhat.com>
Date: Thu, 26 Oct 2017 14:26:30 +0530
Subject: [PATCH 44/74] glusterd: fix brick restart parallelism
glusterd's brick restart logic is not always sequential as there is
atleast three different ways how the bricks are restarted.
1. through friend-sm and glusterd_spawn_daemons ()
2. through friend-sm and handling volume quorum action
3. through friend handshaking when there is a mimatch on quorum on
friend import.
In a brick multiplexing setup, glusterd ended up trying to spawn the
same brick process couple of times as almost in fraction of milliseconds
two threads hit glusterd_brick_start () because of which glusterd didn't
have any choice of rejecting any one of them as for both the case brick
start criteria met.
As a solution, it'd be better to control this madness by two different
flags, one is a boolean called start_triggered which indicates a brick
start has been triggered and it continues to be true till a brick dies
or killed, the second is a mutex lock to ensure for a particular brick
we don't end up getting into glusterd_brick_start () more than once at
same point of time.
>mainline patch : https://review.gluster.org/#/c/18577
Change-Id: I292f1e58d6971e111725e1baea1fe98b890b43e2
BUG: 1505363
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/123056
Reviewed-by: Gaurav Yadav <gyadav@redhat.com>
---
xlators/mgmt/glusterd/src/glusterd-handler.c | 24 ++++++++-----
xlators/mgmt/glusterd/src/glusterd-op-sm.c | 31 ++++++++++-------
xlators/mgmt/glusterd/src/glusterd-server-quorum.c | 15 +++++++--
xlators/mgmt/glusterd/src/glusterd-utils.c | 39 +++++++++++++++++-----
xlators/mgmt/glusterd/src/glusterd-volume-ops.c | 8 +++++
xlators/mgmt/glusterd/src/glusterd.h | 2 ++
6 files changed, 87 insertions(+), 32 deletions(-)
diff --git a/xlators/mgmt/glusterd/src/glusterd-handler.c b/xlators/mgmt/glusterd/src/glusterd-handler.c
index 34e751c..c3b9252 100644
--- a/xlators/mgmt/glusterd/src/glusterd-handler.c
+++ b/xlators/mgmt/glusterd/src/glusterd-handler.c
@@ -5946,16 +5946,22 @@ glusterd_mark_bricks_stopped_by_proc (glusterd_brick_proc_t *brick_proc) {
int ret = -1;
cds_list_for_each_entry (brickinfo, &brick_proc->bricks, brick_list) {
- ret = glusterd_get_volinfo_from_brick (brickinfo->path, &volinfo);
+ ret = glusterd_get_volinfo_from_brick (brickinfo->path,
+ &volinfo);
if (ret) {
- gf_msg (THIS->name, GF_LOG_ERROR, 0, GD_MSG_VOLINFO_GET_FAIL,
- "Failed to get volinfo from brick(%s)",
- brickinfo->path);
+ gf_msg (THIS->name, GF_LOG_ERROR, 0,
+ GD_MSG_VOLINFO_GET_FAIL, "Failed to get volinfo"
+ " from brick(%s)", brickinfo->path);
goto out;
}
- cds_list_for_each_entry (brickinfo_tmp, &volinfo->bricks, brick_list) {
- if (strcmp (brickinfo->path, brickinfo_tmp->path) == 0)
- glusterd_set_brick_status (brickinfo_tmp, GF_BRICK_STOPPED);
+ cds_list_for_each_entry (brickinfo_tmp, &volinfo->bricks,
+ brick_list) {
+ if (strcmp (brickinfo->path,
+ brickinfo_tmp->path) == 0) {
+ glusterd_set_brick_status (brickinfo_tmp,
+ GF_BRICK_STOPPED);
+ brickinfo_tmp->start_triggered = _gf_false;
+ }
}
}
return 0;
@@ -6129,8 +6135,10 @@ __glusterd_brick_rpc_notify (struct rpc_clnt *rpc, void *mydata,
if (temp == 1)
break;
}
- } else
+ } else {
glusterd_set_brick_status (brickinfo, GF_BRICK_STOPPED);
+ brickinfo->start_triggered = _gf_false;
+ }
break;
case RPC_CLNT_DESTROY:
diff --git a/xlators/mgmt/glusterd/src/glusterd-op-sm.c b/xlators/mgmt/glusterd/src/glusterd-op-sm.c
index 9641b4f..5b8f833 100644
--- a/xlators/mgmt/glusterd/src/glusterd-op-sm.c
+++ b/xlators/mgmt/glusterd/src/glusterd-op-sm.c
@@ -2402,18 +2402,25 @@ glusterd_start_bricks (glusterd_volinfo_t *volinfo)
GF_ASSERT (volinfo);
cds_list_for_each_entry (brickinfo, &volinfo->bricks, brick_list) {
- ret = glusterd_brick_start (volinfo, brickinfo, _gf_false);
- if (ret) {
- gf_msg (THIS->name, GF_LOG_ERROR, 0,
- GD_MSG_BRICK_DISCONNECTED,
- "Failed to start %s:%s for %s",
- brickinfo->hostname, brickinfo->path,
- volinfo->volname);
- gf_event (EVENT_BRICK_START_FAILED,
- "peer=%s;volume=%s;brick=%s",
- brickinfo->hostname, volinfo->volname,
- brickinfo->path);
- goto out;
+ if (!brickinfo->start_triggered) {
+ pthread_mutex_lock (&brickinfo->restart_mutex);
+ {
+ ret = glusterd_brick_start (volinfo, brickinfo,
+ _gf_false);
+ }
+ pthread_mutex_unlock (&brickinfo->restart_mutex);
+ if (ret) {
+ gf_msg (THIS->name, GF_LOG_ERROR, 0,
+ GD_MSG_BRICK_DISCONNECTED,
+ "Failed to start %s:%s for %s",
+ brickinfo->hostname, brickinfo->path,
+ volinfo->volname);
+ gf_event (EVENT_BRICK_START_FAILED,
+ "peer=%s;volume=%s;brick=%s",
+ brickinfo->hostname, volinfo->volname,
+ brickinfo->path);
+ goto out;
+ }
}
}
diff --git a/xlators/mgmt/glusterd/src/glusterd-server-quorum.c b/xlators/mgmt/glusterd/src/glusterd-server-quorum.c
index 4706403..995a568 100644
--- a/xlators/mgmt/glusterd/src/glusterd-server-quorum.c
+++ b/xlators/mgmt/glusterd/src/glusterd-server-quorum.c
@@ -362,10 +362,19 @@ glusterd_do_volume_quorum_action (xlator_t *this, glusterd_volinfo_t *volinfo,
list_for_each_entry (brickinfo, &volinfo->bricks, brick_list) {
if (!glusterd_is_local_brick (this, volinfo, brickinfo))
continue;
- if (quorum_status == DOESNT_MEET_QUORUM)
+ if (quorum_status == DOESNT_MEET_QUORUM) {
glusterd_brick_stop (volinfo, brickinfo, _gf_false);
- else
- glusterd_brick_start (volinfo, brickinfo, _gf_false);
+ } else {
+ if (!brickinfo->start_triggered) {
+ pthread_mutex_lock (&brickinfo->restart_mutex);
+ {
+ glusterd_brick_start (volinfo,
+ brickinfo,
+ _gf_false);
+ }
+ pthread_mutex_unlock (&brickinfo->restart_mutex);
+ }
+ }
}
volinfo->quorum_status = quorum_status;
if (quorum_status == MEETS_QUORUM) {
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index bb236df..18de517 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -1084,7 +1084,7 @@ glusterd_brickinfo_new (glusterd_brickinfo_t **brickinfo)
goto out;
CDS_INIT_LIST_HEAD (&new_brickinfo->brick_list);
-
+ pthread_mutex_init (&new_brickinfo->restart_mutex, NULL);
*brickinfo = new_brickinfo;
ret = 0;
@@ -2481,7 +2481,7 @@ glusterd_volume_stop_glusterfs (glusterd_volinfo_t *volinfo,
(void) sys_unlink (pidfile);
brickinfo->status = GF_BRICK_STOPPED;
-
+ brickinfo->start_triggered = _gf_false;
if (del_brick)
glusterd_delete_brick (volinfo, brickinfo);
out:
@@ -5817,13 +5817,14 @@ glusterd_brick_start (glusterd_volinfo_t *volinfo,
* three different triggers for an attempt to start the brick process
* due to the quorum handling code in glusterd_friend_sm.
*/
- if (brickinfo->status == GF_BRICK_STARTING) {
+ if (brickinfo->status == GF_BRICK_STARTING ||
+ brickinfo->start_triggered) {
gf_msg_debug (this->name, 0, "brick %s is already in starting "
"phase", brickinfo->path);
ret = 0;
goto out;
}
-
+ brickinfo->start_triggered = _gf_true;
GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, brickinfo, conf);
if (gf_is_service_running (pidfile, &pid)) {
if (brickinfo->status != GF_BRICK_STARTING &&
@@ -5936,6 +5937,9 @@ run:
}
out:
+ if (ret && brickinfo) {
+ brickinfo->start_triggered = _gf_false;
+ }
gf_msg_debug (this->name, 0, "returning %d ", ret);
return ret;
}
@@ -5997,11 +6001,19 @@ glusterd_restart_bricks (glusterd_conf_t *conf)
start_svcs = _gf_true;
glusterd_svcs_manager (NULL);
}
-
cds_list_for_each_entry (brickinfo, &volinfo->bricks,
brick_list) {
- glusterd_brick_start (volinfo, brickinfo,
- _gf_false);
+ if (!brickinfo->start_triggered) {
+ pthread_mutex_lock
+ (&brickinfo->restart_mutex);
+ {
+ glusterd_brick_start
+ (volinfo, brickinfo,
+ _gf_false);
+ }
+ pthread_mutex_unlock
+ (&brickinfo->restart_mutex);
+ }
}
ret = glusterd_store_volinfo
(volinfo, GLUSTERD_VOLINFO_VER_AC_NONE);
@@ -6040,8 +6052,17 @@ glusterd_restart_bricks (glusterd_conf_t *conf)
"volume %s", volinfo->volname);
cds_list_for_each_entry (brickinfo, &volinfo->bricks,
brick_list) {
- glusterd_brick_start (volinfo, brickinfo,
- _gf_false);
+ if (!brickinfo->start_triggered) {
+ pthread_mutex_lock
+ (&brickinfo->restart_mutex);
+ {
+ glusterd_brick_start
+ (volinfo, brickinfo,
+ _gf_false);
+ }
+ pthread_mutex_unlock
+ (&brickinfo->restart_mutex);
+ }
}
ret = glusterd_store_volinfo
(volinfo, GLUSTERD_VOLINFO_VER_AC_NONE);
diff --git a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
index 834acab..bec5f72 100644
--- a/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-volume-ops.c
@@ -2545,6 +2545,14 @@ glusterd_start_volume (glusterd_volinfo_t *volinfo, int flags,
GF_ASSERT (volinfo);
cds_list_for_each_entry (brickinfo, &volinfo->bricks, brick_list) {
+ /* Mark start_triggered to false so that in case if this brick
+ * was brought down through gf_attach utility, the
+ * brickinfo->start_triggered wouldn't have been updated to
+ * _gf_false
+ */
+ if (flags & GF_CLI_FLAG_OP_FORCE) {
+ brickinfo->start_triggered = _gf_false;
+ }
ret = glusterd_brick_start (volinfo, brickinfo, wait);
/* If 'force' try to start all bricks regardless of success or
* failure
diff --git a/xlators/mgmt/glusterd/src/glusterd.h b/xlators/mgmt/glusterd/src/glusterd.h
index 722d2f8..d4bb236 100644
--- a/xlators/mgmt/glusterd/src/glusterd.h
+++ b/xlators/mgmt/glusterd/src/glusterd.h
@@ -240,6 +240,8 @@ struct glusterd_brickinfo {
uint64_t statfs_fsid;
uint32_t fs_share_count;
gf_boolean_t port_registered;
+ gf_boolean_t start_triggered;
+ pthread_mutex_t restart_mutex;
};
typedef struct glusterd_brickinfo glusterd_brickinfo_t;
--
1.8.3.1