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 <pranithk@gluster.com>
Reviewed-on: http://review.gluster.com/2957
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Jeff Darcy <jdarcy@redhat.com>
Reviewed-by: Amar Tumballi <amarts@redhat.com>
Reviewed-by: Anand Avati <avati@redhat.com>

4 files changed, 27 insertions, 38 deletions

diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c
index db029b2b5..7613912e1 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 (&ltable->lock);
-        {
-                list_splice_init (&ltable->inodelk_lockers,
-                                  &inodelk_lockers);
-
-                list_splice_init (&ltable->entrylk_lockers, &entrylk_lockers);
-        }
-        UNLOCK (&ltable->lock);
+        list_splice_init (&ltable->inodelk_lockers,
+                          &inodelk_lockers);
 
-        LOCK_DESTROY (&ltable->lock);
+        list_splice_init (&ltable->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 (&ltable->lock);
-                        {
-                                list_splice_init (&ltable->inodelk_lockers,
-                                                  &inodelk_lockers);
+                        list_splice_init (&ltable->inodelk_lockers,
+                                          &inodelk_lockers);
 
-                                list_splice_init (&ltable->entrylk_lockers,
-                                                  &entrylk_lockers);
-                        }
-                        UNLOCK (&ltable->lock);
-                        LOCK_DESTROY (&ltable->lock);
+                        list_splice_init (&ltable->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 9a502b478..5ce5e36ac 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 6024d100b..831c309a2 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 3dae07b03..3cf7eb2f8 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)) {