diff mbox series

[v1,08/22] sunrpc: Fix connect metrics

Message ID 20180910150937.10564.26277.stgit@manet.1015granger.net (mailing list archive)
State New, archived
Headers show
Series NFS/RDMA client patches for v4.20 | expand

Commit Message

Chuck Lever Sept. 10, 2018, 3:09 p.m. UTC 
For TCP, the logic in xprt_connect_status is currently never invoked
to record a successful connection. Commit 2a4919919a97 ("SUNRPC:
Return EAGAIN instead of ENOTCONN when waking up xprt->pending")
changed the way TCP xprt's are awoken after a connect succeeds.

Instead, change connection-oriented transports to bump connect_count
and compute connect_time the moment that XPRT_CONNECTED is set.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprt.c               |   10 +++-------
 net/sunrpc/xprtrdma/transport.c |    6 +++++-
 net/sunrpc/xprtsock.c           |   10 ++++++----
 3 files changed, 14 insertions(+), 12 deletions(-)

Comments

Anna Schumaker Sept. 12, 2018, 6:41 p.m. UTC | #1 
Hi Chuck,

On Mon, 2018-09-10 at 11:09 -0400, Chuck Lever wrote:
> For TCP, the logic in xprt_connect_status is currently never invoked
> to record a successful connection. Commit 2a4919919a97 ("SUNRPC:
> Return EAGAIN instead of ENOTCONN when waking up xprt->pending")
> changed the way TCP xprt's are awoken after a connect succeeds.
> 
> Instead, change connection-oriented transports to bump connect_count
> and compute connect_time the moment that XPRT_CONNECTED is set.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/xprt.c               |   10 +++-------
>  net/sunrpc/xprtrdma/transport.c |    6 +++++-
>  net/sunrpc/xprtsock.c           |   10 ++++++----
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index a8db2e3f..f03ffa2 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -791,15 +791,11 @@ static void xprt_connect_status(struct rpc_task *task)
>  {
>  	struct rpc_xprt	*xprt = task->tk_rqstp->rq_xprt;

Looks like the only remaining user of the xprt is in a dprintk() towards the
bottom of this function to get the servername.  This is giving me an unused
variable warning when CONFIG_SUNRPC_DEBUG=n, so I'm wondering if you can take
out the variable and just access the server name the long way (task->tk_rqstp-
>rq_xprt->servername)?

Thanks,
Anna

>  
> -	if (task->tk_status == 0) {
> -		xprt->stat.connect_count++;
> -		xprt->stat.connect_time += (long)jiffies - xprt-
> >stat.connect_start;
> +	switch (task->tk_status) {
> +	case 0:
>  		dprintk("RPC: %5u xprt_connect_status: connection
> established\n",
>  				task->tk_pid);
> -		return;
> -	}
> -
> -	switch (task->tk_status) {
> +		break;
>  	case -ECONNREFUSED:
>  	case -ECONNRESET:
>  	case -ECONNABORTED:
> diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
> index 3ae73e6..087acfc 100644
> --- a/net/sunrpc/xprtrdma/transport.c
> +++ b/net/sunrpc/xprtrdma/transport.c
> @@ -242,8 +242,12 @@
>  
>  	spin_lock_bh(&xprt->transport_lock);
>  	if (ep->rep_connected > 0) {
> -		if (!xprt_test_and_set_connected(xprt))
> +		if (!xprt_test_and_set_connected(xprt)) {
> +			xprt->stat.connect_count++;
> +			xprt->stat.connect_time += (long)jiffies -
> +						   xprt->stat.connect_start;
>  			xprt_wake_pending_tasks(xprt, 0);
> +		}
>  	} else {
>  		if (xprt_test_and_clear_connected(xprt))
>  			xprt_wake_pending_tasks(xprt, -ENOTCONN);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 6b7539c..e146caa 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1611,6 +1611,9 @@ static void xs_tcp_state_change(struct sock *sk)
>  			clear_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
>  			xprt_clear_connecting(xprt);
>  
> +			xprt->stat.connect_count++;
> +			xprt->stat.connect_time += (long)jiffies -
> +						   xprt->stat.connect_start;
>  			xprt_wake_pending_tasks(xprt, -EAGAIN);
>  		}
>  		spin_unlock(&xprt->transport_lock);
> @@ -2029,8 +2032,6 @@ static int xs_local_finish_connecting(struct rpc_xprt
> *xprt,
>  	}
>  
>  	/* Tell the socket layer to start connecting... */
> -	xprt->stat.connect_count++;
> -	xprt->stat.connect_start = jiffies;
>  	return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, 0);
>  }
>  
> @@ -2062,6 +2063,9 @@ static int xs_local_setup_socket(struct sock_xprt
> *transport)
>  	case 0:
>  		dprintk("RPC:       xprt %p connected to %s\n",
>  				xprt, xprt->address_strings[RPC_DISPLAY_ADDR]);
> +		xprt->stat.connect_count++;
> +		xprt->stat.connect_time += (long)jiffies -
> +					   xprt->stat.connect_start;
>  		xprt_set_connected(xprt);
>  	case -ENOBUFS:
>  		break;
> @@ -2387,8 +2391,6 @@ static int xs_tcp_finish_connecting(struct rpc_xprt
> *xprt, struct socket *sock)
>  	xs_set_memalloc(xprt);
>  
>  	/* Tell the socket layer to start connecting... */
> -	xprt->stat.connect_count++;
> -	xprt->stat.connect_start = jiffies;
>  	set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
>  	ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
>  	switch (ret) {
>
diff mbox series

Patch

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index a8db2e3f..f03ffa2 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -791,15 +791,11 @@  static void xprt_connect_status(struct rpc_task *task)
 {
 	struct rpc_xprt	*xprt = task->tk_rqstp->rq_xprt;
 
-	if (task->tk_status == 0) {
-		xprt->stat.connect_count++;
-		xprt->stat.connect_time += (long)jiffies - xprt->stat.connect_start;
+	switch (task->tk_status) {
+	case 0:
 		dprintk("RPC: %5u xprt_connect_status: connection established\n",
 				task->tk_pid);
-		return;
-	}
-
-	switch (task->tk_status) {
+		break;
 	case -ECONNREFUSED:
 	case -ECONNRESET:
 	case -ECONNABORTED:
diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
index 3ae73e6..087acfc 100644
--- a/net/sunrpc/xprtrdma/transport.c
+++ b/net/sunrpc/xprtrdma/transport.c
@@ -242,8 +242,12 @@ 
 
 	spin_lock_bh(&xprt->transport_lock);
 	if (ep->rep_connected > 0) {
-		if (!xprt_test_and_set_connected(xprt))
+		if (!xprt_test_and_set_connected(xprt)) {
+			xprt->stat.connect_count++;
+			xprt->stat.connect_time += (long)jiffies -
+						   xprt->stat.connect_start;
 			xprt_wake_pending_tasks(xprt, 0);
+		}
 	} else {
 		if (xprt_test_and_clear_connected(xprt))
 			xprt_wake_pending_tasks(xprt, -ENOTCONN);
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 6b7539c..e146caa 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1611,6 +1611,9 @@  static void xs_tcp_state_change(struct sock *sk)
 			clear_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
 			xprt_clear_connecting(xprt);
 
+			xprt->stat.connect_count++;
+			xprt->stat.connect_time += (long)jiffies -
+						   xprt->stat.connect_start;
 			xprt_wake_pending_tasks(xprt, -EAGAIN);
 		}
 		spin_unlock(&xprt->transport_lock);
@@ -2029,8 +2032,6 @@  static int xs_local_finish_connecting(struct rpc_xprt *xprt,
 	}
 
 	/* Tell the socket layer to start connecting... */
-	xprt->stat.connect_count++;
-	xprt->stat.connect_start = jiffies;
 	return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, 0);
 }
 
@@ -2062,6 +2063,9 @@  static int xs_local_setup_socket(struct sock_xprt *transport)
 	case 0:
 		dprintk("RPC:       xprt %p connected to %s\n",
 				xprt, xprt->address_strings[RPC_DISPLAY_ADDR]);
+		xprt->stat.connect_count++;
+		xprt->stat.connect_time += (long)jiffies -
+					   xprt->stat.connect_start;
 		xprt_set_connected(xprt);
 	case -ENOBUFS:
 		break;
@@ -2387,8 +2391,6 @@  static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	xs_set_memalloc(xprt);
 
 	/* Tell the socket layer to start connecting... */
-	xprt->stat.connect_count++;
-	xprt->stat.connect_start = jiffies;
 	set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
 	ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
 	switch (ret) {