rpc/clnt: Don't let consumers manage "connected" state

The state management of "connected" in rpc is ad-hoc as far as the
responsibility goes. Note that there is nothing wrong with
functionality itself. rpc layer manages this state in disconnect
codepath and has exposed an api to manage this one from
consumers. Note that rpc layer never sets "connected" to true by
itself, which forces the consumers to use this api to get a working
rpc connection. The situation is best captured from a comment in code
from Jeff Darcy in glusterfsd/src/gf-attach.c:

-/*
- * In a sane world, the generic RPC layer would be capable of tracking
- * connection status by itself, with no help from us.  It might invoke our
- * callback if we had registered one, but only to provide information.  Sadly,
- * we don't live in that world.  Instead, the callback *must* exist and *must*
- * call rpc_clnt_{set,unset}_connected, because that's the only way those
- * fields get set (with RPC both above and below us on the stack).  If we don't
- * do that, then rpc_clnt_submit doesn't think we're connected even when we
- * are.  It calls the socket code to reconnect, but the socket code tracks this
- * stuff in a sane way so it knows we're connected and returns EINPROGRESS.
- * Then we're stuck, connected but unable to use the connection.  To make it
- * work, we define and register this trivial callback.
- */

Also, consumers of rpc know about state of connection only through the
notifications sent by rpc-clnt. So, consumers don't have any extra
information to manage the state and hence letting them manage the
state is counter intuitive. This patch cleans that up and instead
moves the responsibility of state management of rpc layer into
itself.

Change-Id: I31e641a60795fc480ca753917f4b2579f1e05094
Signed-off-by: Raghavendra G <rgowdapp@redhat.com>
Fixes: bz#1585585

10 files changed, 15 insertions, 99 deletions

diff --git a/api/src/glfs-mgmt.c b/api/src/glfs-mgmt.c
index 229caa98777..e6c84fd4b13 100644
--- a/api/src/glfs-mgmt.c
+++ b/api/src/glfs-mgmt.c
@@ -958,8 +958,6 @@ mgmt_rpc_notify (struct rpc_clnt *rpc, void *mydata, rpc_clnt_event_t event,
                 }
                 break;
 	case RPC_CLNT_CONNECT:
-		rpc_clnt_set_connected (&((struct rpc_clnt*)ctx->mgmt)->conn);
-
 		ret = glfs_volfile_fetch (fs);
 		if (ret && (ctx->active == NULL)) {
 			/* Do it only for the first time */
diff --git a/glusterfsd/src/gf_attach.c b/glusterfsd/src/gf_attach.c
index 3f248292ddf..1c1106c06f6 100644
--- a/glusterfsd/src/gf_attach.c
+++ b/glusterfsd/src/gf_attach.c
@@ -37,39 +37,6 @@ struct rpc_clnt_program gf_attach_prog = {
         .numproc   = GLUSTERD_BRICK_MAXVALUE,
 };
 
-/*
- * In a sane world, the generic RPC layer would be capable of tracking
- * connection status by itself, with no help from us.  It might invoke our
- * callback if we had registered one, but only to provide information.  Sadly,
- * we don't live in that world.  Instead, the callback *must* exist and *must*
- * call rpc_clnt_{set,unset}_connected, because that's the only way those
- * fields get set (with RPC both above and below us on the stack).  If we don't
- * do that, then rpc_clnt_submit doesn't think we're connected even when we
- * are.  It calls the socket code to reconnect, but the socket code tracks this
- * stuff in a sane way so it knows we're connected and returns EINPROGRESS.
- * Then we're stuck, connected but unable to use the connection.  To make it
- * work, we define and register this trivial callback.
- */
-int
-my_notify (struct rpc_clnt *rpc, void *mydata,
-           rpc_clnt_event_t event, void *data)
-{
-        switch (event) {
-        case RPC_CLNT_CONNECT:
-                printf ("connected\n");
-                rpc_clnt_set_connected (&rpc->conn);
-                break;
-        case RPC_CLNT_DISCONNECT:
-                printf ("disconnected\n");
-                rpc_clnt_unset_connected (&rpc->conn);
-                break;
-        default:
-                fprintf (stderr, "unknown RPC event\n");
-        }
-
-        return 0;
-}
-
 int32_t
 my_callback (struct rpc_req *req, struct iovec *iov, int count, void *frame)
 {
@@ -232,7 +199,7 @@ done_parsing:
                 return EXIT_FAILURE;
         }
 
-        if (rpc_clnt_register_notify (rpc, my_notify, NULL) != 0) {
+        if (rpc_clnt_register_notify (rpc, NULL, NULL) != 0) {
                 fprintf (stderr, "rpc_clnt_register_notify failed\n");
                 return EXIT_FAILURE;
         }
diff --git a/glusterfsd/src/glusterfsd-mgmt.c b/glusterfsd/src/glusterfsd-mgmt.c
index 5666a532696..863765d644f 100644
--- a/glusterfsd/src/glusterfsd-mgmt.c
+++ b/glusterfsd/src/glusterfsd-mgmt.c
@@ -2402,8 +2402,6 @@ mgmt_rpc_notify (struct rpc_clnt *rpc, void *mydata, rpc_clnt_event_t event,
                         server->volfile_server);
                 break;
         case RPC_CLNT_CONNECT:
-                rpc_clnt_set_connected (&((struct rpc_clnt*)ctx->mgmt)->conn);
-
                 ret = glusterfs_volfile_fetch (ctx);
                 if (ret) {
                         emval = ret;
diff --git a/rpc/rpc-lib/src/libgfrpc.sym b/rpc/rpc-lib/src/libgfrpc.sym
index 4fab688c66d..a7cb5f6b5cb 100644
--- a/rpc/rpc-lib/src/libgfrpc.sym
+++ b/rpc/rpc-lib/src/libgfrpc.sym
@@ -10,11 +10,9 @@ rpc_clnt_reconnect
 rpc_clnt_reconnect_cleanup
 rpc_clnt_ref
 rpc_clnt_register_notify
-rpc_clnt_set_connected
 rpc_clnt_start
 rpc_clnt_submit
 rpc_clnt_unref
-rpc_clnt_unset_connected
 rpc_reply_to_xdr
 rpcsvc_auth_array
 rpcsvc_auth_check
diff --git a/rpc/rpc-lib/src/rpc-clnt.c b/rpc/rpc-lib/src/rpc-clnt.c
index 0471a268c66..330a96837e6 100644
--- a/rpc/rpc-lib/src/rpc-clnt.c
+++ b/rpc/rpc-lib/src/rpc-clnt.c
@@ -529,6 +529,7 @@ rpc_clnt_connection_cleanup (rpc_clnt_connection_t *conn)
                 }
 
                 conn->connected = 0;
+                conn->disconnected = 1;
 
                 unref = rpc_clnt_remove_ping_timer_locked (clnt);
                 /*reset rpc msgs stats*/
@@ -788,44 +789,6 @@ out:
         return ret;
 }
 
-
-void
-rpc_clnt_set_connected (rpc_clnt_connection_t *conn)
-{
-        if (!conn) {
-                goto out;
-        }
-
-        pthread_mutex_lock (&conn->lock);
-        {
-                conn->connected = 1;
-                conn->disconnected = _gf_false;
-        }
-        pthread_mutex_unlock (&conn->lock);
-
-out:
-        return;
-}
-
-
-void
-rpc_clnt_unset_connected (rpc_clnt_connection_t *conn)
-{
-        if (!conn) {
-                goto out;
-        }
-
-        pthread_mutex_lock (&conn->lock);
-        {
-                conn->connected = 0;
-                conn->disconnected = _gf_true;
-        }
-        pthread_mutex_unlock (&conn->lock);
-
-out:
-        return;
-}
-
 gf_boolean_t
 is_rpc_clnt_disconnected (rpc_clnt_connection_t *conn)
 {
@@ -1000,14 +963,19 @@ rpc_clnt_notify (rpc_transport_t *trans, void *mydata,
 
         case RPC_TRANSPORT_CONNECT:
         {
-
-                /* Every time there is a disconnection, processes
-                 * should try to connect to 'glusterd' (ie, default
-                 * port) or whichever port given as 'option remote-port'
-                 * in volume file. */
-                /* Below code makes sure the (re-)configured port lasts
-                 * for just one successful attempt */
-                conn->config.remote_port = 0;
+                pthread_mutex_lock (&conn->lock);
+                {
+                        /* Every time there is a disconnection, processes
+                         * should try to connect to 'glusterd' (ie, default
+                         * port) or whichever port given as 'option remote-port'
+                         * in volume file. */
+                        /* Below code makes sure the (re-)configured port lasts
+                         * for just one successful attempt */
+                        conn->config.remote_port = 0;
+                        conn->connected = 1;
+                        conn->disconnected = 0;
+                }
+                pthread_mutex_unlock (&conn->lock);
 
                 /* auth value should be set to lower version available
                  * and will be set to appropriate version supported by
diff --git a/rpc/rpc-lib/src/rpc-clnt.h b/rpc/rpc-lib/src/rpc-clnt.h
index ea81b41c180..2d6cb060c4e 100644
--- a/rpc/rpc-lib/src/rpc-clnt.h
+++ b/rpc/rpc-lib/src/rpc-clnt.h
@@ -235,11 +235,6 @@ rpc_clnt_unref (struct rpc_clnt *rpc);
 
 int rpc_clnt_connection_cleanup (rpc_clnt_connection_t *conn);
 int rpc_clnt_reconnect_cleanup (rpc_clnt_connection_t *conn);
-
-void rpc_clnt_set_connected (rpc_clnt_connection_t *conn);
-
-void rpc_clnt_unset_connected (rpc_clnt_connection_t *conn);
-
 gf_boolean_t is_rpc_clnt_disconnected (rpc_clnt_connection_t *conn);
 
 void rpc_clnt_reconnect (void *trans_ptr);
diff --git a/xlators/features/changelog/lib/src/gf-changelog-rpc.c b/xlators/features/changelog/lib/src/gf-changelog-rpc.c
index c1139423d6d..7eb5416ae98 100644
--- a/xlators/features/changelog/lib/src/gf-changelog-rpc.c
+++ b/xlators/features/changelog/lib/src/gf-changelog-rpc.c
@@ -21,7 +21,6 @@ gf_changelog_rpc_notify (struct rpc_clnt *rpc,
 {
         switch (event) {
         case RPC_CLNT_CONNECT:
-                rpc_clnt_set_connected (&rpc->conn);
                 break;
         case RPC_CLNT_DISCONNECT:
         case RPC_CLNT_MSG:
diff --git a/xlators/features/changelog/src/changelog-ev-handle.c b/xlators/features/changelog/src/changelog-ev-handle.c
index b5681944567..46d45fbd778 100644
--- a/xlators/features/changelog/src/changelog-ev-handle.c
+++ b/xlators/features/changelog/src/changelog-ev-handle.c
@@ -138,7 +138,6 @@ changelog_rpc_notify (struct rpc_clnt *rpc,
 
         switch (event) {
         case RPC_CLNT_CONNECT:
-                rpc_clnt_set_connected (&rpc->conn);
                 selection = &priv->ev_selection;
 
                 LOCK (&c_clnt->wait_lock);
diff --git a/xlators/mgmt/glusterd/src/glusterd-handler.c b/xlators/mgmt/glusterd/src/glusterd-handler.c
index 9e8a586cbc4..64ff9ffcaf4 100644
--- a/xlators/mgmt/glusterd/src/glusterd-handler.c
+++ b/xlators/mgmt/glusterd/src/glusterd-handler.c
@@ -6111,7 +6111,6 @@ __glusterd_brick_rpc_notify (struct rpc_clnt *rpc, void *mydata,
 
                         break;
                 }
-                rpc_clnt_set_connected (&rpc->conn);
                 gf_msg_debug (this->name, 0, "Connected to %s:%s",
                         brickinfo->hostname, brickinfo->path);
 
@@ -6126,7 +6125,6 @@ __glusterd_brick_rpc_notify (struct rpc_clnt *rpc, void *mydata,
                 break;
 
         case RPC_CLNT_DISCONNECT:
-                rpc_clnt_unset_connected (&rpc->conn);
                 if (rpc != brickinfo->rpc) {
                         /*
                          * There used to be a bunch of races in the volume
@@ -6358,7 +6356,6 @@ __glusterd_peer_rpc_notify (struct rpc_clnt *rpc, void *mydata,
         switch (event) {
         case RPC_CLNT_CONNECT:
         {
-                rpc_clnt_set_connected (&rpc->conn);
                 gf_msg_debug (this->name, 0, "got RPC_CLNT_CONNECT");
                 peerinfo->connected = 1;
                 peerinfo->quorum_action = _gf_true;
@@ -6391,7 +6388,6 @@ __glusterd_peer_rpc_notify (struct rpc_clnt *rpc, void *mydata,
                 if (is_rpc_clnt_disconnected (&rpc->conn))
                         break;
 
-                rpc_clnt_unset_connected (&rpc->conn);
                 gf_msg (this->name, GF_LOG_INFO, 0,
                         GD_MSG_PEER_DISCONNECTED,
                         "Peer <%s> (<%s>), in state <%s>, has disconnected "
diff --git a/xlators/protocol/client/src/client-handshake.c b/xlators/protocol/client/src/client-handshake.c
index 5c0b4750e2e..1c705f59404 100644
--- a/xlators/protocol/client/src/client-handshake.c
+++ b/xlators/protocol/client/src/client-handshake.c
@@ -1175,8 +1175,6 @@ client_setvolume_cbk (struct rpc_req *req, struct iovec *iov, int count, void *m
                 conf->rpc->conn.name,
                 remote_subvol);
 
-        rpc_clnt_set_connected (&conf->rpc->conn);
-
         op_ret = 0;
         conf->connecting = 0;
         conf->connected = 1;