by Anna Schumaker

[permalink] [raw]

Subject: Re: [PATCH v1 08/22] sunrpc: Fix connect metrics

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 <[email protected]>
> ---
> 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) {
>