From 6aab9d9602dc1ef62a2d1d63aa1764a062bf9d9f Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Sat, 3 Mar 2012 17:28:17 +0530 Subject: protocol/server: Avoid race in add/del locker, connection_cleanup conn->ltable address keeps changing in server_connection_cleanup every time it is called. i.e. New ltable is created every time it is called. Here is the race that happened: --------------------------------------------------- thread-1 | thread-2 add_locker is called with | conn->ltable. lets call the | ltable address lt1 | | connection cleanup is called | and do_lock_table_cleanup is | triggered for lt1. locker | lists are splice_inited under | the lt1->lock lt1 adds the locker under | lt1->lock (lets call this l1) | | GF_FREE(lt1) happens in | do_lock_table_cleanup The locker l1 that is added just before lt1 is freed will never be cleared in the subsequent server_connection_cleanups as there does not exist a reference to the locker. The stale lock remains in the locks xlator even though the transport on which it was issued is destroyed. Change-Id: I0a02f16c703d1e7598b083aa1057cda9624eb3fe BUG: 787601 Signed-off-by: Pranith Kumar K Reviewed-on: http://review.gluster.com/2957 Tested-by: Gluster Build System Reviewed-by: Jeff Darcy Reviewed-by: Amar Tumballi Reviewed-by: Anand Avati --- xlators/protocol/server/src/server-helpers.c | 44 +++++++++++----------------- xlators/protocol/server/src/server-helpers.h | 4 +-- xlators/protocol/server/src/server.h | 1 - xlators/protocol/server/src/server3_1-fops.c | 16 +++++----- 4 files changed, 27 insertions(+), 38 deletions(-) (limited to 'xlators/protocol') diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c index db029b2b573..7613912e137 100644 --- a/xlators/protocol/server/src/server-helpers.c +++ b/xlators/protocol/server/src/server-helpers.c @@ -133,14 +133,14 @@ free_state (server_state_t *state) int -gf_add_locker (struct _lock_table *table, const char *volume, +gf_add_locker (server_connection_t *conn, const char *volume, loc_t *loc, fd_t *fd, pid_t pid, gf_lkowner_t *owner, glusterfs_fop_t type) { int32_t ret = -1; struct _locker *new = NULL; + struct _lock_table *table = NULL; - GF_VALIDATE_OR_GOTO ("server", table, out); GF_VALIDATE_OR_GOTO ("server", volume, out); new = GF_CALLOC (1, sizeof (struct _locker), gf_server_mt_locker_t); @@ -160,21 +160,22 @@ gf_add_locker (struct _lock_table *table, const char *volume, new->pid = pid; new->owner = *owner; - LOCK (&table->lock); + pthread_mutex_lock (&conn->lock); { + table = conn->ltable; if (type == GF_FOP_ENTRYLK) list_add_tail (&new->lockers, &table->entrylk_lockers); else list_add_tail (&new->lockers, &table->inodelk_lockers); } - UNLOCK (&table->lock); + pthread_mutex_unlock (&conn->lock); out: return ret; } int -gf_del_locker (struct _lock_table *table, const char *volume, +gf_del_locker (server_connection_t *conn, const char *volume, loc_t *loc, fd_t *fd, gf_lkowner_t *owner, glusterfs_fop_t type) { @@ -183,14 +184,15 @@ gf_del_locker (struct _lock_table *table, const char *volume, int32_t ret = -1; struct list_head *head = NULL; struct list_head del; + struct _lock_table *table = NULL; - GF_VALIDATE_OR_GOTO ("server", table, out); GF_VALIDATE_OR_GOTO ("server", volume, out); INIT_LIST_HEAD (&del); - LOCK (&table->lock); + pthread_mutex_lock (&conn->lock); { + table = conn->ltable; if (type == GF_FOP_ENTRYLK) { head = &table->entrylk_lockers; } else { @@ -209,7 +211,7 @@ gf_del_locker (struct _lock_table *table, const char *volume, list_move_tail (&locker->lockers, &del); } } - UNLOCK (&table->lock); + pthread_mutex_unlock (&conn->lock); tmp = NULL; locker = NULL; @@ -241,7 +243,6 @@ gf_lock_table_new (void) } INIT_LIST_HEAD (&new->entrylk_lockers); INIT_LIST_HEAD (&new->inodelk_lockers); - LOCK_INIT (&new->lock); out: return new; } @@ -291,16 +292,10 @@ do_lock_table_cleanup (xlator_t *this, server_connection_t *conn, INIT_LIST_HEAD (&inodelk_lockers); INIT_LIST_HEAD (&entrylk_lockers); - LOCK (<able->lock); - { - list_splice_init (<able->inodelk_lockers, - &inodelk_lockers); - - list_splice_init (<able->entrylk_lockers, &entrylk_lockers); - } - UNLOCK (<able->lock); + list_splice_init (<able->inodelk_lockers, + &inodelk_lockers); - LOCK_DESTROY (<able->lock); + list_splice_init (<able->entrylk_lockers, &entrylk_lockers); GF_FREE (ltable); flock.l_type = F_UNLCK; @@ -606,16 +601,11 @@ server_connection_destroy (xlator_t *this, server_connection_t *conn) INIT_LIST_HEAD (&entrylk_lockers); if (ltable) { - LOCK (<able->lock); - { - list_splice_init (<able->inodelk_lockers, - &inodelk_lockers); + list_splice_init (<able->inodelk_lockers, + &inodelk_lockers); - list_splice_init (<able->entrylk_lockers, - &entrylk_lockers); - } - UNLOCK (<able->lock); - LOCK_DESTROY (<able->lock); + list_splice_init (<able->entrylk_lockers, + &entrylk_lockers); GF_FREE (ltable); } diff --git a/xlators/protocol/server/src/server-helpers.h b/xlators/protocol/server/src/server-helpers.h index 9a502b47876..5ce5e36acfe 100644 --- a/xlators/protocol/server/src/server-helpers.h +++ b/xlators/protocol/server/src/server-helpers.h @@ -48,7 +48,7 @@ void free_state (server_state_t *state); void server_loc_wipe (loc_t *loc); int32_t -gf_add_locker (struct _lock_table *table, const char *volume, +gf_add_locker (server_connection_t *conn, const char *volume, loc_t *loc, fd_t *fd, pid_t pid, @@ -56,7 +56,7 @@ gf_add_locker (struct _lock_table *table, const char *volume, glusterfs_fop_t type); int32_t -gf_del_locker (struct _lock_table *table, const char *volume, +gf_del_locker (server_connection_t *conn, const char *volume, loc_t *loc, fd_t *fd, gf_lkowner_t *owner, diff --git a/xlators/protocol/server/src/server.h b/xlators/protocol/server/src/server.h index 6024d100b30..831c309a245 100644 --- a/xlators/protocol/server/src/server.h +++ b/xlators/protocol/server/src/server.h @@ -49,7 +49,6 @@ struct _locker { struct _lock_table { struct list_head inodelk_lockers; struct list_head entrylk_lockers; - gf_lock_t lock; size_t count; }; diff --git a/xlators/protocol/server/src/server3_1-fops.c b/xlators/protocol/server/src/server3_1-fops.c index 3dae07b0385..3cf7eb2f820 100644 --- a/xlators/protocol/server/src/server3_1-fops.c +++ b/xlators/protocol/server/src/server3_1-fops.c @@ -219,11 +219,11 @@ server_inodelk_cbk (call_frame_t *frame, void *cookie, xlator_t *this, if (op_ret >= 0) { if (state->flock.l_type == F_UNLCK) - gf_del_locker (conn->ltable, state->volume, + gf_del_locker (conn, state->volume, &state->loc, NULL, &frame->root->lk_owner, GF_FOP_INODELK); else - gf_add_locker (conn->ltable, state->volume, + gf_add_locker (conn, state->volume, &state->loc, NULL, frame->root->pid, &frame->root->lk_owner, GF_FOP_INODELK); @@ -261,11 +261,11 @@ server_finodelk_cbk (call_frame_t *frame, void *cookie, xlator_t *this, if (op_ret >= 0) { if (state->flock.l_type == F_UNLCK) - gf_del_locker (conn->ltable, state->volume, + gf_del_locker (conn, state->volume, NULL, state->fd, &frame->root->lk_owner, GF_FOP_INODELK); else - gf_add_locker (conn->ltable, state->volume, + gf_add_locker (conn, state->volume, NULL, state->fd, frame->root->pid, &frame->root->lk_owner, GF_FOP_INODELK); @@ -302,11 +302,11 @@ server_entrylk_cbk (call_frame_t *frame, void *cookie, xlator_t *this, if (op_ret >= 0) { if (state->cmd == ENTRYLK_UNLOCK) - gf_del_locker (conn->ltable, state->volume, + gf_del_locker (conn, state->volume, &state->loc, NULL, &frame->root->lk_owner, GF_FOP_ENTRYLK); else - gf_add_locker (conn->ltable, state->volume, + gf_add_locker (conn, state->volume, &state->loc, NULL, frame->root->pid, &frame->root->lk_owner, GF_FOP_ENTRYLK); @@ -342,11 +342,11 @@ server_fentrylk_cbk (call_frame_t *frame, void *cookie, xlator_t *this, state = CALL_STATE(frame); if (op_ret >= 0) { if (state->cmd == ENTRYLK_UNLOCK) - gf_del_locker (conn->ltable, state->volume, + gf_del_locker (conn, state->volume, NULL, state->fd, &frame->root->lk_owner, GF_FOP_ENTRYLK); else - gf_add_locker (conn->ltable, state->volume, + gf_add_locker (conn, state->volume, NULL, state->fd, frame->root->pid, &frame->root->lk_owner, GF_FOP_ENTRYLK); } else if ((op_errno != ENOSYS) && (op_errno != EAGAIN)) { -- cgit