summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authoranand <anekkunt@redhat.com>2015-04-17 14:19:46 +0530
committerVijay Bellur <vbellur@redhat.com>2015-04-28 01:53:28 -0700
commitada6b3a8800867934af57a57d5312f5a5d8374f0 (patch)
treee6e7c6fa2c22c00fd0dd62be86f460adb0c98c27
parentd4889b2cfd29e6ecc911d2b29d1f85d516a66eaf (diff)
libglusterfs: Implementation of sync lock as recursive lock to avoid crash.
Problem : In glusterd,we are using big lock which is implemented based on sync task frame work for thread synchronization and rcu lock for data consistency. sync task frame work swap the threads if there is no worker poll threads available,due to this rcu lock and rcu unlock was happening in different threads (urcu-bp will not allow this),resulting into glusterd crash. fix : To avoid releasing the sync lock(big lock) in between rcu critical section,implemented sync lock as recursive lock. More details: link : http://www.spinics.net/lists/gluster-devel/msg14632.html Change-Id: I2b56c1caf3f0470f219b1adcaf62cce29cdc6b88 BUG: 1211640 Signed-off-by: anand <anekkunt@redhat.com> Reviewed-on: http://review.gluster.org/10285 Reviewed-by: Atin Mukherjee <amukherj@redhat.com> Tested-by: Gluster Build System <jenkins@build.gluster.com> Tested-by: NetBSD Build System Reviewed-by: Vijay Bellur <vbellur@redhat.com>
-rw-r--r--libglusterfs/src/syncop.c174
-rw-r--r--libglusterfs/src/syncop.h26
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-mgmt.c18
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-op-sm.c2
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-syncop.c12
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-utils.c4
-rw-r--r--xlators/mgmt/glusterd/src/glusterd.c2
7 files changed, 151 insertions, 87 deletions
diff --git a/libglusterfs/src/syncop.c b/libglusterfs/src/syncop.c
index 20d4d00561c..cb08b03d44b 100644
--- a/libglusterfs/src/syncop.c
+++ b/libglusterfs/src/syncop.c
@@ -827,16 +827,20 @@ syncenv_new (size_t stacksize, int procmin, int procmax)
int
-synclock_init (synclock_t *lock)
+synclock_init (synclock_t *lock, lock_attr_t attr)
{
- if (!lock)
- return -1;
+ if (!lock)
+ return -1;
- pthread_cond_init (&lock->cond, 0);
- lock->lock = 0;
- INIT_LIST_HEAD (&lock->waitq);
+ pthread_cond_init (&lock->cond, 0);
+ lock->type = LOCK_NULL;
+ lock->owner = NULL;
+ lock->owner_tid = 0;
+ lock->lock = 0;
+ lock->attr = attr;
+ INIT_LIST_HEAD (&lock->waitq);
- return pthread_mutex_init (&lock->guard, 0);
+ return pthread_mutex_init (&lock->guard, 0);
}
@@ -854,32 +858,70 @@ synclock_destroy (synclock_t *lock)
static int
__synclock_lock (struct synclock *lock)
{
- struct synctask *task = NULL;
+ struct synctask *task = NULL;
- if (!lock)
- return -1;
+ if (!lock)
+ return -1;
- task = synctask_get ();
+ task = synctask_get ();
- while (lock->lock) {
- if (task) {
- /* called within a synctask */
- list_add_tail (&task->waitq, &lock->waitq);
+ if (lock->lock && (lock->attr == SYNC_LOCK_RECURSIVE)) {
+ /*Recursive lock (if same owner requested for lock again then
+ *increment lock count and return success).
+ *Note:same number of unlocks required.
+ */
+ switch (lock->type) {
+ case LOCK_TASK:
+ if (task == lock->owner) {
+ lock->lock++;
+ gf_log ("", GF_LOG_TRACE, "Recursive lock called by "
+ "sync task.owner= %p,lock=%d", lock->owner, lock->lock);
+ return 0;
+ }
+ break;
+ case LOCK_THREAD:
+ if (pthread_self () == lock->owner_tid) {
+ lock->lock++;
+ gf_log ("", GF_LOG_TRACE, "Recursive lock called by "
+ "thread ,owner=%u lock=%d", (unsigned int) lock->owner_tid,
+ lock->lock);
+ return 0;
+ }
+ break;
+ default:
+ gf_log ("", GF_LOG_CRITICAL, "unknown lock type");
+ break;
+ }
+ }
+
+
+ while (lock->lock) {
+ if (task) {
+ /* called within a synctask */
+ list_add_tail (&task->waitq, &lock->waitq);
pthread_mutex_unlock (&lock->guard);
synctask_yield (task);
/* task is removed from waitq in unlock,
* under lock->guard.*/
pthread_mutex_lock (&lock->guard);
- } else {
- /* called by a non-synctask */
- pthread_cond_wait (&lock->cond, &lock->guard);
- }
- }
+ } else {
+ /* called by a non-synctask */
+ pthread_cond_wait (&lock->cond, &lock->guard);
+ }
+ }
- lock->lock = _gf_true;
- lock->owner = task;
+ if (task) {
+ lock->type = LOCK_TASK;
+ lock->owner = task; /* for synctask*/
- return 0;
+ } else {
+ lock->type = LOCK_THREAD;
+ lock->owner_tid = pthread_self (); /* for non-synctask */
+
+ }
+ lock->lock = 1;
+
+ return 0;
}
@@ -925,37 +967,73 @@ unlock:
static int
__synclock_unlock (synclock_t *lock)
{
- struct synctask *task = NULL;
- struct synctask *curr = NULL;
+ struct synctask *task = NULL;
+ struct synctask *curr = NULL;
- if (!lock)
- return -1;
+ if (!lock)
+ return -1;
+
+ if (lock->lock == 0) {
+ gf_log ("", GF_LOG_CRITICAL, "Unlock called before lock ");
+ return -1;
+ }
+ curr = synctask_get ();
+ /*unlock should be called by lock owner
+ *i.e this will not allow the lock in nonsync task and unlock
+ * in sync task and vice-versa
+ */
+ switch (lock->type) {
+ case LOCK_TASK:
+ if (curr == lock->owner) {
+ lock->lock--;
+ gf_log ("", GF_LOG_TRACE, "Unlock success %p, remaining"
+ " locks=%d", lock->owner, lock->lock);
+ } else {
+ gf_log ("", GF_LOG_WARNING, "Unlock called by %p, but"
+ " lock held by %p", curr, lock->owner);
+ }
- curr = synctask_get ();
+ break;
+ case LOCK_THREAD:
+ if (pthread_self () == lock->owner_tid) {
+ lock->lock--;
+ gf_log ("", GF_LOG_TRACE, "Unlock success %u, remaining"
+ " locks=%d", (unsigned int)lock->owner_tid, lock->lock);
+ } else {
+ gf_log ("", GF_LOG_WARNING, "Unlock called by %u, but"
+ " lock held by %u", (unsigned int) pthread_self(),
+ (unsigned int) lock->owner_tid);
+ }
- if (lock->owner != curr) {
- /* warn ? */
- }
+ break;
+ default:
+ break;
+ }
- lock->lock = _gf_false;
-
- /* There could be both synctasks and non synctasks
- waiting (or none, or either). As a mid-approach
- between maintaining too many waiting counters
- at one extreme and a thundering herd on unlock
- at the other, call a cond_signal (which wakes
- one waiter) and first synctask waiter. So at
- most we have two threads waking up to grab the
- just released lock.
- */
- pthread_cond_signal (&lock->cond);
- if (!list_empty (&lock->waitq)) {
- task = list_entry (lock->waitq.next, struct synctask, waitq);
+ if (lock->lock > 0) {
+ return 0;
+ }
+ lock->type = LOCK_NULL;
+ lock->owner = NULL;
+ lock->owner_tid = 0;
+ lock->lock = 0;
+ /* There could be both synctasks and non synctasks
+ waiting (or none, or either). As a mid-approach
+ between maintaining too many waiting counters
+ at one extreme and a thundering herd on unlock
+ at the other, call a cond_signal (which wakes
+ one waiter) and first synctask waiter. So at
+ most we have two threads waking up to grab the
+ just released lock.
+ */
+ pthread_cond_signal (&lock->cond);
+ if (!list_empty (&lock->waitq)) {
+ task = list_entry (lock->waitq.next, struct synctask, waitq);
list_del_init (&task->waitq);
- synctask_wake (task);
- }
+ synctask_wake (task);
+ }
- return 0;
+ return 0;
}
diff --git a/libglusterfs/src/syncop.h b/libglusterfs/src/syncop.h
index 3a7798fa17e..f41706a9d37 100644
--- a/libglusterfs/src/syncop.h
+++ b/libglusterfs/src/syncop.h
@@ -114,12 +114,26 @@ struct syncenv {
};
+typedef enum {
+ LOCK_NULL = 0,
+ LOCK_TASK,
+ LOCK_THREAD
+} lock_type_t;
+
+typedef enum {
+ SYNC_LOCK_DEFAULT = 0,
+ SYNC_LOCK_RECURSIVE, /*it allows recursive locking*/
+} lock_attr_t;
+
struct synclock {
- pthread_mutex_t guard; /* guard the remaining members, pair @cond */
- pthread_cond_t cond; /* waiting non-synctasks */
- struct list_head waitq; /* waiting synctasks */
- gf_boolean_t lock; /* _gf_true or _gf_false, lock status */
- struct synctask *owner; /* NULL if current owner is not a synctask */
+ pthread_mutex_t guard; /* guard the remaining members, pair @cond */
+ pthread_cond_t cond; /* waiting non-synctasks */
+ struct list_head waitq; /* waiting synctasks */
+ volatile int lock; /* true(non zero) or false(zero), lock status */
+ lock_attr_t attr;
+ struct synctask *owner; /* NULL if current owner is not a synctask */
+ pthread_t owner_tid;
+ lock_type_t type;
};
typedef struct synclock synclock_t;
@@ -336,7 +350,7 @@ syncop_create_frame (xlator_t *this)
return frame;
}
-int synclock_init (synclock_t *lock);
+int synclock_init (synclock_t *lock, lock_attr_t attr);
int synclock_destroy (synclock_t *lock);
int synclock_lock (synclock_t *lock);
int synclock_trylock (synclock_t *lock);
diff --git a/xlators/mgmt/glusterd/src/glusterd-mgmt.c b/xlators/mgmt/glusterd/src/glusterd-mgmt.c
index e07d4100d00..539efb5900d 100644
--- a/xlators/mgmt/glusterd/src/glusterd-mgmt.c
+++ b/xlators/mgmt/glusterd/src/glusterd-mgmt.c
@@ -378,14 +378,11 @@ gd_mgmt_v3_lock (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_LOCK,
gd_mgmt_v3_lock_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_lock_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
@@ -653,14 +650,11 @@ gd_mgmt_v3_pre_validate_req (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_PRE_VALIDATE,
gd_mgmt_v3_pre_validate_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_pre_val_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
@@ -911,14 +905,11 @@ gd_mgmt_v3_brick_op_req (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_BRICK_OP,
gd_mgmt_v3_brick_op_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_brick_op_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
@@ -1154,14 +1145,11 @@ gd_mgmt_v3_commit_req (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_COMMIT,
gd_mgmt_v3_commit_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_commit_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
@@ -1376,14 +1364,11 @@ gd_mgmt_v3_post_validate_req (glusterd_op_t op, int32_t op_ret, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_POST_VALIDATE,
gd_mgmt_v3_post_validate_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_post_val_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
@@ -1588,14 +1573,11 @@ gd_mgmt_v3_unlock (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
-
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_UNLOCK,
gd_mgmt_v3_unlock_cbk,
(xdrproc_t) xdr_gd1_mgmt_v3_unlock_req);
- synclock_lock (&conf->big_lock);
out:
GF_FREE (req.dict.dict_val);
gf_log (this->name, GF_LOG_TRACE, "Returning %d", ret);
diff --git a/xlators/mgmt/glusterd/src/glusterd-op-sm.c b/xlators/mgmt/glusterd/src/glusterd-op-sm.c
index 33df2c31d8a..42477ebf6cd 100644
--- a/xlators/mgmt/glusterd/src/glusterd-op-sm.c
+++ b/xlators/mgmt/glusterd/src/glusterd-op-sm.c
@@ -6924,6 +6924,6 @@ int
glusterd_op_sm_init ()
{
CDS_INIT_LIST_HEAD (&gd_op_sm_queue);
- synclock_init (&gd_op_sm_lock);
+ synclock_init (&gd_op_sm_lock, SYNC_LOCK_DEFAULT);
return 0;
}
diff --git a/xlators/mgmt/glusterd/src/glusterd-syncop.c b/xlators/mgmt/glusterd/src/glusterd-syncop.c
index ee8f1eadf2e..e049e2e15e9 100644
--- a/xlators/mgmt/glusterd/src/glusterd-syncop.c
+++ b/xlators/mgmt/glusterd/src/glusterd-syncop.c
@@ -406,14 +406,12 @@ gd_syncop_mgmt_v3_lock (glusterd_op_t op, dict_t *op_ctx,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_LOCK,
gd_syncop_mgmt_v3_lock_cbk,
(xdrproc_t)
xdr_gd1_mgmt_v3_lock_req);
- synclock_lock (&conf->big_lock);
out:
gf_log ("", GF_LOG_DEBUG, "Returning %d", ret);
return ret;
@@ -503,14 +501,12 @@ gd_syncop_mgmt_v3_unlock (dict_t *op_ctx, glusterd_peerinfo_t *peerinfo,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_v3_prog,
GLUSTERD_MGMT_V3_UNLOCK,
gd_syncop_mgmt_v3_unlock_cbk,
(xdrproc_t)
xdr_gd1_mgmt_v3_unlock_req);
- synclock_lock (&conf->big_lock);
out:
gf_log ("", GF_LOG_DEBUG, "Returning %d", ret);
return ret;
@@ -598,13 +594,11 @@ gd_syncop_mgmt_lock (glusterd_peerinfo_t *peerinfo, struct syncargs *args,
gf_uuid_copy (req.uuid, my_uuid);
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_prog,
GLUSTERD_MGMT_CLUSTER_LOCK,
gd_syncop_mgmt_lock_cbk,
(xdrproc_t) xdr_gd1_mgmt_cluster_lock_req);
- synclock_lock (&conf->big_lock);
return ret;
}
@@ -689,13 +683,11 @@ gd_syncop_mgmt_unlock (glusterd_peerinfo_t *peerinfo, struct syncargs *args,
gf_uuid_copy (req.uuid, my_uuid);
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, &req, args, &peerid,
&gd_mgmt_prog,
GLUSTERD_MGMT_CLUSTER_UNLOCK,
gd_syncop_mgmt_unlock_cbk,
(xdrproc_t) xdr_gd1_mgmt_cluster_lock_req);
- synclock_lock (&conf->big_lock);
return ret;
}
@@ -826,12 +818,10 @@ gd_syncop_mgmt_stage_op (glusterd_peerinfo_t *peerinfo, struct syncargs *args,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, req, args, &peerid,
&gd_mgmt_prog, GLUSTERD_MGMT_STAGE_OP,
gd_syncop_stage_op_cbk,
(xdrproc_t) xdr_gd1_mgmt_stage_op_req);
- synclock_lock (&conf->big_lock);
out:
gd_stage_op_req_free (req);
return ret;
@@ -1108,12 +1098,10 @@ gd_syncop_mgmt_commit_op (glusterd_peerinfo_t *peerinfo, struct syncargs *args,
gf_uuid_copy (peerid, peerinfo->uuid);
- synclock_unlock (&conf->big_lock);
ret = gd_syncop_submit_request (peerinfo->rpc, req, args, &peerid,
&gd_mgmt_prog, GLUSTERD_MGMT_COMMIT_OP,
gd_syncop_commit_op_cbk,
(xdrproc_t) xdr_gd1_mgmt_commit_op_req);
- synclock_lock (&conf->big_lock);
out:
gd_commit_op_req_free (req);
return ret;
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index 2103fd62e03..3025cd6f118 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -2730,6 +2730,7 @@ glusterd_spawn_daemons (void *opaque)
gf_boolean_t start_bricks = !conf->restart_done;
int ret = -1;
+ synclock_lock (&conf->big_lock);
if (start_bricks) {
glusterd_restart_bricks (conf);
conf->restart_done = _gf_true;
@@ -9096,7 +9097,8 @@ glusterd_launch_synctask (synctask_fn_t fn, void *opaque)
this = THIS;
priv = this->private;
- synclock_lock (&priv->big_lock);
+ /* synclock_lock must be called from within synctask, @fn must call it before
+ * it starts with its work*/
ret = synctask_new (this->ctx->env, fn, gd_default_synctask_cbk, NULL,
opaque);
if (ret)
diff --git a/xlators/mgmt/glusterd/src/glusterd.c b/xlators/mgmt/glusterd/src/glusterd.c
index 361cc007327..3735c62abc1 100644
--- a/xlators/mgmt/glusterd/src/glusterd.c
+++ b/xlators/mgmt/glusterd/src/glusterd.c
@@ -1554,7 +1554,7 @@ init (xlator_t *this)
conf->gfs_mgmt = &gd_brick_prog;
strncpy (conf->workdir, workdir, PATH_MAX);
- synclock_init (&conf->big_lock);
+ synclock_init (&conf->big_lock, SYNC_LOCK_RECURSIVE);
pthread_mutex_init (&conf->xprt_lock, NULL);
INIT_LIST_HEAD (&conf->xprt_list);