protocol/server: Remove connection from conf->conns w.o. race

1) Adding the connection to conf->conns used to
happen in conf->mutex, but removing happened under conn->lock.
Fixed that as below.
When the connection object is created conn's ref, bind_ref count
is set to '1'. For bind_ref ref/unref happens under conf->mutex
whenever server_connection_get, put is called.
When bind_ref goes to '0' connection object is removed from
conf->conns under conf->mutex.  After it is removed from the list,
conn_unref is called outside the conf->mutex.
conn_ref/unref still happens under conn->lock.

2) Fixed races in server_connection_cleaup in grace_timer_handler
and server_setvolume.

Change-Id: Ie7b63b10f658af909a11c3327066667f5b7bd114
BUG: 801675
Signed-off-by: Pranith Kumar K <pranithk@gluster.com>
Reviewed-on: http://review.gluster.com/2911
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Amar Tumballi <amarts@redhat.com>
Reviewed-by: Vijay Bellur <vijay@gluster.com>

4 files changed, 47 insertions, 8 deletions

diff --git a/xlators/protocol/server/src/server-handshake.c b/xlators/protocol/server/src/server-handshake.c
index d1e3659ab..ba80185fe 100644
--- a/xlators/protocol/server/src/server-handshake.c
+++ b/xlators/protocol/server/src/server-handshake.c
@@ -432,8 +432,8 @@ server_setvolume (rpcsvc_request_t *req)
         }
 
         cancelled = server_cancel_conn_timer (this, conn);
-        if (cancelled)
-                server_conn_unref (conn);
+        if (cancelled)//Do connection_put on behalf of grace-timer-handler.
+                server_connection_put (this, conn, NULL);
         if (conn->lk_version != 0 &&
             conn->lk_version != lk_version) {
                 (void) server_connection_cleanup (this, conn);
@@ -723,7 +723,7 @@ server_set_lk_version (rpcsvc_request_t *req)
 
         conn = server_connection_get (this, args.uid);
         conn->lk_version = args.lk_ver;
-        server_conn_unref (conn);
+        server_connection_put (this, conn, NULL);
 
         rsp.lk_ver   = args.lk_ver;
 
diff --git a/xlators/protocol/server/src/server-helpers.c b/xlators/protocol/server/src/server-helpers.c
index 28d59aa44..db029b2b5 100644
--- a/xlators/protocol/server/src/server-helpers.c
+++ b/xlators/protocol/server/src/server-helpers.c
@@ -648,7 +648,6 @@ server_conn_unref (server_connection_t *conn)
                 conn->ref--;
 
                 if (!conn->ref) {
-                        list_del_init (&conn->list);
                         todel = conn;
                 }
         }
@@ -691,6 +690,7 @@ server_connection_get (xlator_t *this, const char *id)
                 list_for_each_entry (trav, &conf->conns, list) {
                         if (!strncmp (trav->id, id, strlen (id))) {
                                 conn = trav;
+                                conn->bind_ref++;
                                 goto unlock;
                         }
                 }
@@ -706,6 +706,8 @@ server_connection_get (xlator_t *this, const char *id)
                 conn->fdtable = gf_fd_fdtable_alloc ();
                 conn->ltable  = gf_lock_table_new ();
                 conn->this    = this;
+                conn->bind_ref = 1;
+                conn->ref     = 1;//when bind_ref becomes 0 it calls conn_unref
                 pthread_mutex_init (&conn->lock, NULL);
                 list_add (&conn->list, &conf->conns);
 
@@ -713,11 +715,38 @@ server_connection_get (xlator_t *this, const char *id)
 unlock:
         pthread_mutex_unlock (&conf->mutex);
 out:
-        if (conn)
-                server_conn_ref (conn);
         return conn;
 }
 
+server_connection_t*
+server_connection_put (xlator_t *this, server_connection_t *conn,
+                       gf_boolean_t *detached)
+{
+        server_conf_t       *conf = NULL;
+        gf_boolean_t        unref = _gf_false;
+
+        if (detached)
+                *detached = _gf_false;
+        conf = this->private;
+        pthread_mutex_lock (&conf->mutex);
+        {
+                conn->bind_ref--;
+                if (!conn->bind_ref) {
+                        list_del_init (&conn->list);
+                        unref = _gf_true;
+                }
+        }
+        pthread_mutex_unlock (&conf->mutex);
+        if (unref) {
+                gf_log (this->name, GF_LOG_INFO, "Shutting down connection %s",
+                        conn->id);
+                if (detached)
+                        *detached = _gf_true;
+                server_conn_unref (conn);
+                conn = NULL;
+        }
+        return conn;
+}
 
 static call_frame_t *
 server_alloc_frame (rpcsvc_request_t *req)
diff --git a/xlators/protocol/server/src/server.c b/xlators/protocol/server/src/server.c
index 7ac8590a4..b75ad172e 100644
--- a/xlators/protocol/server/src/server.c
+++ b/xlators/protocol/server/src/server.c
@@ -42,6 +42,7 @@ grace_time_handler (void *data)
         server_connection_t     *conn = NULL;
         xlator_t                *this = NULL;
         gf_boolean_t            cancelled = _gf_false;
+        gf_boolean_t            detached = _gf_false;
 
         conn = data;
         this = conn->this;
@@ -49,11 +50,15 @@ grace_time_handler (void *data)
         GF_VALIDATE_OR_GOTO (THIS->name, conn, out);
         GF_VALIDATE_OR_GOTO (THIS->name, this, out);
 
-        gf_log (this->name, GF_LOG_INFO, "grace timer expired");
+        gf_log (this->name, GF_LOG_INFO, "grace timer expired for %s", conn->id);
 
         cancelled = server_cancel_conn_timer (this, conn);
         if (cancelled) {
-                server_connection_cleanup (this, conn);
+                //conn should not be destroyed in conn_put, so take a ref.
+                server_conn_ref (conn);
+                server_connection_put (this, conn, &detached);
+                if (detached)//reconnection did not happen :-(
+                        server_connection_cleanup (this, conn);
                 server_conn_unref (conn);
         }
 out:
diff --git a/xlators/protocol/server/src/server.h b/xlators/protocol/server/src/server.h
index c54454e3e..6024d100b 100644
--- a/xlators/protocol/server/src/server.h
+++ b/xlators/protocol/server/src/server.h
@@ -60,6 +60,7 @@ struct _server_connection {
         struct list_head    list;
         char               *id;
         int                 ref;
+        int                 bind_ref;
         pthread_mutex_t     lock;
         fdtable_t          *fdtable;
         struct _lock_table *ltable;
@@ -75,6 +76,10 @@ typedef struct _server_connection server_connection_t;
 server_connection_t *
 server_connection_get (xlator_t *this, const char *id);
 
+server_connection_t *
+server_connection_put (xlator_t *this, server_connection_t *conn,
+                       gf_boolean_t *detached);
+
 server_connection_t*
 server_conn_unref (server_connection_t *conn);