[PATCH 1/4] SUNRPC: Fix races when closing the socket

[PATCH 1/4] SUNRPC: Fix races when closing the socket

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Ensure that we bump the xprt->connect_cookie when we set the
XPRT_CLOSE_WAIT flag so that another call to
xprt_conditional_disconnect() won't race with the reconnection.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprt.c     | 2 ++
 net/sunrpc/xprtsock.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 48560188e84d..691fe5a682b6 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -735,6 +735,8 @@ static void xprt_autoclose(struct work_struct *work)
 	unsigned int pflags = memalloc_nofs_save();
 
 	trace_xprt_disconnect_auto(xprt);
+	xprt->connect_cookie++;
+	smp_mb__before_atomic();
 	clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
 	xprt->ops->close(xprt);
 	xprt_release_write(xprt, NULL);
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 04f1b78bcbca..b18d13479104 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1134,6 +1134,7 @@ static void xs_run_error_worker(struct sock_xprt *transport, unsigned int nr)
 
 static void xs_sock_reset_connection_flags(struct rpc_xprt *xprt)
 {
+	xprt->connect_cookie++;
 	smp_mb__before_atomic();
 	clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
 	clear_bit(XPRT_CLOSING, &xprt->state);
-- 
2.31.1

[PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Since we do things like setting flags, etc it really is more appropriate
to use sock_lock().

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 38 +++++++++++---------------------------
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index b18d13479104..291b63136c08 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -1154,14 +1154,13 @@ static void xs_error_report(struct sock *sk)
 	struct sock_xprt *transport;
 	struct rpc_xprt *xprt;
 
-	read_lock_bh(&sk->sk_callback_lock);
 	if (!(xprt = xprt_from_sock(sk)))
-		goto out;
+		return;
 
 	transport = container_of(xprt, struct sock_xprt, xprt);
 	transport->xprt_err = -sk->sk_err;
 	if (transport->xprt_err == 0)
-		goto out;
+		return;
 	dprintk("RPC:       xs_error_report client %p, error=%d...\n",
 			xprt, -transport->xprt_err);
 	trace_rpc_socket_error(xprt, sk->sk_socket, transport->xprt_err);
@@ -1169,8 +1168,6 @@ static void xs_error_report(struct sock *sk)
 	/* barrier ensures xprt_err is set before XPRT_SOCK_WAKE_ERROR */
 	smp_mb__before_atomic();
 	xs_run_error_worker(transport, XPRT_SOCK_WAKE_ERROR);
- out:
-	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void xs_reset_transport(struct sock_xprt *transport)
@@ -1189,7 +1186,7 @@ static void xs_reset_transport(struct sock_xprt *transport)
 	kernel_sock_shutdown(sock, SHUT_RDWR);
 
 	mutex_lock(&transport->recv_mutex);
-	write_lock_bh(&sk->sk_callback_lock);
+	lock_sock(sk);
 	transport->inet = NULL;
 	transport->sock = NULL;
 	transport->file = NULL;
@@ -1198,10 +1195,10 @@ static void xs_reset_transport(struct sock_xprt *transport)
 
 	xs_restore_old_callbacks(transport, sk);
 	xprt_clear_connected(xprt);
-	write_unlock_bh(&sk->sk_callback_lock);
 	xs_sock_reset_connection_flags(xprt);
 	/* Reset stream record info */
 	xs_stream_reset_connect(transport);
+	release_sock(sk);
 	mutex_unlock(&transport->recv_mutex);
 
 	trace_rpc_socket_close(xprt, sock);
@@ -1365,7 +1362,6 @@ static void xs_data_ready(struct sock *sk)
 {
 	struct rpc_xprt *xprt;
 
-	read_lock_bh(&sk->sk_callback_lock);
 	dprintk("RPC:       xs_data_ready...\n");
 	xprt = xprt_from_sock(sk);
 	if (xprt != NULL) {
@@ -1380,7 +1376,6 @@ static void xs_data_ready(struct sock *sk)
 		if (!test_and_set_bit(XPRT_SOCK_DATA_READY, &transport->sock_state))
 			queue_work(xprtiod_workqueue, &transport->recv_worker);
 	}
-	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 /*
@@ -1409,9 +1404,8 @@ static void xs_tcp_state_change(struct sock *sk)
 	struct rpc_xprt *xprt;
 	struct sock_xprt *transport;
 
-	read_lock_bh(&sk->sk_callback_lock);
 	if (!(xprt = xprt_from_sock(sk)))
-		goto out;
+		return;
 	dprintk("RPC:       xs_tcp_state_change client %p...\n", xprt);
 	dprintk("RPC:       state %x conn %d dead %d zapped %d sk_shutdown %d\n",
 			sk->sk_state, xprt_connected(xprt),
@@ -1472,8 +1466,6 @@ static void xs_tcp_state_change(struct sock *sk)
 		/* Trigger the socket release */
 		xs_run_error_worker(transport, XPRT_SOCK_WAKE_DISCONNECT);
 	}
- out:
-	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void xs_write_space(struct sock *sk)
@@ -1512,13 +1504,9 @@ static void xs_write_space(struct sock *sk)
  */
 static void xs_udp_write_space(struct sock *sk)
 {
-	read_lock_bh(&sk->sk_callback_lock);
-
 	/* from net/core/sock.c:sock_def_write_space */
 	if (sock_writeable(sk))
 		xs_write_space(sk);
-
-	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 /**
@@ -1533,13 +1521,9 @@ static void xs_udp_write_space(struct sock *sk)
  */
 static void xs_tcp_write_space(struct sock *sk)
 {
-	read_lock_bh(&sk->sk_callback_lock);
-
 	/* from net/core/stream.c:sk_stream_write_space */
 	if (sk_stream_is_writeable(sk))
 		xs_write_space(sk);
-
-	read_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void xs_udp_do_set_buffer_size(struct rpc_xprt *xprt)
@@ -1834,7 +1818,7 @@ static int xs_local_finish_connecting(struct rpc_xprt *xprt,
 	if (!transport->inet) {
 		struct sock *sk = sock->sk;
 
-		write_lock_bh(&sk->sk_callback_lock);
+		lock_sock(sk);
 
 		xs_save_old_callbacks(transport, sk);
 
@@ -1850,7 +1834,7 @@ static int xs_local_finish_connecting(struct rpc_xprt *xprt,
 		transport->sock = sock;
 		transport->inet = sk;
 
-		write_unlock_bh(&sk->sk_callback_lock);
+		release_sock(sk);
 	}
 
 	xs_stream_start_connect(transport);
@@ -2032,7 +2016,7 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	if (!transport->inet) {
 		struct sock *sk = sock->sk;
 
-		write_lock_bh(&sk->sk_callback_lock);
+		lock_sock(sk);
 
 		xs_save_old_callbacks(transport, sk);
 
@@ -2049,7 +2033,7 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 
 		xs_set_memalloc(xprt);
 
-		write_unlock_bh(&sk->sk_callback_lock);
+		release_sock(sk);
 	}
 	xs_udp_do_set_buffer_size(xprt);
 
@@ -2195,7 +2179,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 		xs_tcp_set_socket_timeouts(xprt, sock);
 		tcp_sock_set_nodelay(sk);
 
-		write_lock_bh(&sk->sk_callback_lock);
+		lock_sock(sk);
 
 		xs_save_old_callbacks(transport, sk);
 
@@ -2215,7 +2199,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 		transport->sock = sock;
 		transport->inet = sk;
 
-		write_unlock_bh(&sk->sk_callback_lock);
+		release_sock(sk);
 	}
 
 	if (!xprt_bound(xprt))
-- 
2.31.1

[PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock()

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Move the error handling into a single switch statement for clarity.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 68 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 291b63136c08..7fb302e202bc 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt,
 static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 {
 	struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
-	int ret = -ENOTCONN;
 
 	if (!transport->inet) {
 		struct sock *sk = sock->sk;
@@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 	}
 
 	if (!xprt_bound(xprt))
-		goto out;
+		return -ENOTCONN;
 
 	xs_set_memalloc(xprt);
 
@@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
 
 	/* Tell the socket layer to start connecting... */
 	set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
-	ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
-	switch (ret) {
-	case 0:
-		xs_set_srcport(transport, sock);
-		fallthrough;
-	case -EINPROGRESS:
-		/* SYN_SENT! */
-		if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
-			xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
-		break;
-	case -EADDRNOTAVAIL:
-		/* Source port number is unavailable. Try a new one! */
-		transport->srcport = 0;
-	}
-out:
-	return ret;
+	return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
 }
 
 /**
@@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work)
 		container_of(work, struct sock_xprt, connect_worker.work);
 	struct socket *sock = transport->sock;
 	struct rpc_xprt *xprt = &transport->xprt;
-	int status = -EIO;
+	int status;
 
 	if (!sock) {
 		sock = xs_create_sock(xprt, transport,
 				xs_addr(xprt)->sa_family, SOCK_STREAM,
 				IPPROTO_TCP, true);
 		if (IS_ERR(sock)) {
-			status = PTR_ERR(sock);
+			xprt_wake_pending_tasks(xprt, PTR_ERR(sock));
 			goto out;
 		}
 	}
@@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work)
 			xprt, -status, xprt_connected(xprt),
 			sock->sk->sk_state);
 	switch (status) {
-	default:
-		printk("%s: connect returned unhandled error %d\n",
-			__func__, status);
-		fallthrough;
-	case -EADDRNOTAVAIL:
-		/* We're probably in TIME_WAIT. Get rid of existing socket,
-		 * and retry
-		 */
-		xs_tcp_force_close(xprt);
-		break;
 	case 0:
+		xs_set_srcport(transport, sock);
+		fallthrough;
 	case -EINPROGRESS:
+		/* SYN_SENT! */
+		if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
+			xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
+		fallthrough;
 	case -EALREADY:
-		xprt_unlock_connect(xprt, transport);
-		return;
+		goto out_unlock;
+	case -EADDRNOTAVAIL:
+		/* Source port number is unavailable. Try a new one! */
+		transport->srcport = 0;
+		status = -EAGAIN;
+		break;
 	case -EINVAL:
 		/* Happens, for instance, if the user specified a link
 		 * local IPv6 address without a scope-id.
@@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work)
 	case -EHOSTUNREACH:
 	case -EADDRINUSE:
 	case -ENOBUFS:
-		/* xs_tcp_force_close() wakes tasks with a fixed error code.
-		 * We need to wake them first to ensure the correct error code.
-		 */
-		xprt_wake_pending_tasks(xprt, status);
-		xs_tcp_force_close(xprt);
-		goto out;
+		break;
+	default:
+		printk("%s: connect returned unhandled error %d\n",
+			__func__, status);
+		status = -EAGAIN;
 	}
-	status = -EAGAIN;
+
+	/* xs_tcp_force_close() wakes tasks with a fixed error code.
+	 * We need to wake them first to ensure the correct error code.
+	 */
+	xprt_wake_pending_tasks(xprt, status);
+	xs_tcp_force_close(xprt);
 out:
 	xprt_clear_connecting(xprt);
+out_unlock:
 	xprt_unlock_connect(xprt, transport);
-	xprt_wake_pending_tasks(xprt, status);
 }
 
 /**
-- 
2.31.1

[PATCH 4/4] SUNRPC: Prevent immediate close+reconnect

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If we have already set up the socket and are waiting for it to connect,
then don't immediately close and retry.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtsock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7fb302e202bc..ae48c9c84ee1 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2314,7 +2314,7 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
 
 	WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport));
 
-	if (transport->sock != NULL) {
+	if (transport->sock != NULL && !xprt_connecting(xprt)) {
 		dprintk("RPC:       xs_connect delayed xprt %p for %lu "
 				"seconds\n",
 				xprt, xprt->reestablish_timeout / HZ);
-- 
2.31.1

Re: [PATCH 1/4] SUNRPC: Fix races when closing the socket

[David Wysochanski]

On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> Ensure that we bump the xprt->connect_cookie when we set the
> XPRT_CLOSE_WAIT flag so that another call to
> xprt_conditional_disconnect() won't race with the reconnection.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/xprt.c     | 2 ++
>  net/sunrpc/xprtsock.c | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 48560188e84d..691fe5a682b6 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -735,6 +735,8 @@ static void xprt_autoclose(struct work_struct *work)
>         unsigned int pflags = memalloc_nofs_save();
>
>         trace_xprt_disconnect_auto(xprt);
> +       xprt->connect_cookie++;
> +       smp_mb__before_atomic();
>         clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
>         xprt->ops->close(xprt);
>         xprt_release_write(xprt, NULL);
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 04f1b78bcbca..b18d13479104 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -1134,6 +1134,7 @@ static void xs_run_error_worker(struct sock_xprt *transport, unsigned int nr)
>
>  static void xs_sock_reset_connection_flags(struct rpc_xprt *xprt)
>  {
> +       xprt->connect_cookie++;
>         smp_mb__before_atomic();
>         clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
>         clear_bit(XPRT_CLOSING, &xprt->state);
> --
> 2.31.1
>

Hey Trond,

Are you working on this "double SYN" problem that Olga / Netapp found
or something else?

FWIW, late yesterday, I tested these patches on top of your "testing"
branch and still see "double SYNs".  I have a simple reproducer I'm
using where I fire off a bunch of "flocks" in parallel then reboot the
server.

If I can help with testing or something similar, let me know.  I also
noticed that the tracepoints that would be useful for these reconnect
problems do not have 'src' TCP port in them, which would be helpful.
I don't have a patch for that yet but started looking into it.

Re: [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock()

[David Wysochanski]

On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote:
>
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> Move the error handling into a single switch statement for clarity.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/xprtsock.c | 68 ++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 40 deletions(-)
>
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index 291b63136c08..7fb302e202bc 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt,
>  static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
>  {
>         struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> -       int ret = -ENOTCONN;
>
>         if (!transport->inet) {
>                 struct sock *sk = sock->sk;
> @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
>         }
>
>         if (!xprt_bound(xprt))
> -               goto out;
> +               return -ENOTCONN;
>
>         xs_set_memalloc(xprt);
>
> @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
>
>         /* Tell the socket layer to start connecting... */
>         set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
> -       ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> -       switch (ret) {
> -       case 0:
> -               xs_set_srcport(transport, sock);
> -               fallthrough;
> -       case -EINPROGRESS:
> -               /* SYN_SENT! */
> -               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> -                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> -               break;
> -       case -EADDRNOTAVAIL:
> -               /* Source port number is unavailable. Try a new one! */
> -               transport->srcport = 0;
> -       }
> -out:
> -       return ret;
> +       return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
>  }
>
>  /**
> @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work)
>                 container_of(work, struct sock_xprt, connect_worker.work);
>         struct socket *sock = transport->sock;
>         struct rpc_xprt *xprt = &transport->xprt;
> -       int status = -EIO;
> +       int status;
>
>         if (!sock) {
>                 sock = xs_create_sock(xprt, transport,
>                                 xs_addr(xprt)->sa_family, SOCK_STREAM,
>                                 IPPROTO_TCP, true);
>                 if (IS_ERR(sock)) {
> -                       status = PTR_ERR(sock);
> +                       xprt_wake_pending_tasks(xprt, PTR_ERR(sock));
>                         goto out;
>                 }
>         }
> @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work)
>                         xprt, -status, xprt_connected(xprt),
>                         sock->sk->sk_state);
>         switch (status) {
> -       default:
> -               printk("%s: connect returned unhandled error %d\n",
> -                       __func__, status);
> -               fallthrough;
> -       case -EADDRNOTAVAIL:
> -               /* We're probably in TIME_WAIT. Get rid of existing socket,
> -                * and retry
> -                */
> -               xs_tcp_force_close(xprt);
> -               break;
>         case 0:
> +               xs_set_srcport(transport, sock);
> +               fallthrough;
>         case -EINPROGRESS:
> +               /* SYN_SENT! */
> +               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> +                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> +               fallthrough;
>         case -EALREADY:
> -               xprt_unlock_connect(xprt, transport);
> -               return;
> +               goto out_unlock;
> +       case -EADDRNOTAVAIL:
> +               /* Source port number is unavailable. Try a new one! */
> +               transport->srcport = 0;
> +               status = -EAGAIN;
> +               break;
>         case -EINVAL:
>                 /* Happens, for instance, if the user specified a link
>                  * local IPv6 address without a scope-id.
> @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work)
>         case -EHOSTUNREACH:
>         case -EADDRINUSE:
>         case -ENOBUFS:
> -               /* xs_tcp_force_close() wakes tasks with a fixed error code.
> -                * We need to wake them first to ensure the correct error code.
> -                */
> -               xprt_wake_pending_tasks(xprt, status);
> -               xs_tcp_force_close(xprt);
> -               goto out;
> +               break;
> +       default:
> +               printk("%s: connect returned unhandled error %d\n",
> +                       __func__, status);
> +               status = -EAGAIN;
>         }
> -       status = -EAGAIN;
> +
> +       /* xs_tcp_force_close() wakes tasks with a fixed error code.
> +        * We need to wake them first to ensure the correct error code.
> +        */
> +       xprt_wake_pending_tasks(xprt, status);
> +       xs_tcp_force_close(xprt);

Isn't this a problem to call xs_tcp_force_close() unconditionally at
the bottom of xs_tcp_setup_socket()?
Before this patch we only called this depending on certain returns
from kernel_connect().



>  out:
>         xprt_clear_connecting(xprt);
> +out_unlock:
>         xprt_unlock_connect(xprt, transport);
> -       xprt_wake_pending_tasks(xprt, status);
>  }
>
>  /**
> --
> 2.31.1
>

Re: [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock()

[David Wysochanski]

On Fri, Dec 3, 2021 at 11:10 AM David Wysochanski <dwysocha@redhat.com> wrote:
>
> On Fri, Oct 29, 2021 at 4:11 PM <trondmy@kernel.org> wrote:
> >
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> >
> > Move the error handling into a single switch statement for clarity.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  net/sunrpc/xprtsock.c | 68 ++++++++++++++++++-------------------------
> >  1 file changed, 28 insertions(+), 40 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> > index 291b63136c08..7fb302e202bc 100644
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2159,7 +2159,6 @@ static void xs_tcp_set_connect_timeout(struct rpc_xprt *xprt,
> >  static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >  {
> >         struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> > -       int ret = -ENOTCONN;
> >
> >         if (!transport->inet) {
> >                 struct sock *sk = sock->sk;
> > @@ -2203,7 +2202,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >         }
> >
> >         if (!xprt_bound(xprt))
> > -               goto out;
> > +               return -ENOTCONN;
> >
> >         xs_set_memalloc(xprt);
> >
> > @@ -2211,22 +2210,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> >
> >         /* Tell the socket layer to start connecting... */
> >         set_bit(XPRT_SOCK_CONNECTING, &transport->sock_state);
> > -       ret = kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> > -       switch (ret) {
> > -       case 0:
> > -               xs_set_srcport(transport, sock);
> > -               fallthrough;
> > -       case -EINPROGRESS:
> > -               /* SYN_SENT! */
> > -               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > -                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > -               break;
> > -       case -EADDRNOTAVAIL:
> > -               /* Source port number is unavailable. Try a new one! */
> > -               transport->srcport = 0;
> > -       }
> > -out:
> > -       return ret;
> > +       return kernel_connect(sock, xs_addr(xprt), xprt->addrlen, O_NONBLOCK);
> >  }
> >
> >  /**
> > @@ -2241,14 +2225,14 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >                 container_of(work, struct sock_xprt, connect_worker.work);
> >         struct socket *sock = transport->sock;
> >         struct rpc_xprt *xprt = &transport->xprt;
> > -       int status = -EIO;
> > +       int status;
> >
> >         if (!sock) {
> >                 sock = xs_create_sock(xprt, transport,
> >                                 xs_addr(xprt)->sa_family, SOCK_STREAM,
> >                                 IPPROTO_TCP, true);
> >                 if (IS_ERR(sock)) {
> > -                       status = PTR_ERR(sock);
> > +                       xprt_wake_pending_tasks(xprt, PTR_ERR(sock));
> >                         goto out;
> >                 }
> >         }
> > @@ -2265,21 +2249,21 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >                         xprt, -status, xprt_connected(xprt),
> >                         sock->sk->sk_state);
> >         switch (status) {
> > -       default:
> > -               printk("%s: connect returned unhandled error %d\n",
> > -                       __func__, status);
> > -               fallthrough;
> > -       case -EADDRNOTAVAIL:
> > -               /* We're probably in TIME_WAIT. Get rid of existing socket,
> > -                * and retry
> > -                */
> > -               xs_tcp_force_close(xprt);
> > -               break;
> >         case 0:
> > +               xs_set_srcport(transport, sock);
> > +               fallthrough;
> >         case -EINPROGRESS:
> > +               /* SYN_SENT! */
> > +               if (xprt->reestablish_timeout < XS_TCP_INIT_REEST_TO)
> > +                       xprt->reestablish_timeout = XS_TCP_INIT_REEST_TO;
> > +               fallthrough;
> >         case -EALREADY:
> > -               xprt_unlock_connect(xprt, transport);
> > -               return;
> > +               goto out_unlock;
> > +       case -EADDRNOTAVAIL:
> > +               /* Source port number is unavailable. Try a new one! */
> > +               transport->srcport = 0;
> > +               status = -EAGAIN;
> > +               break;
> >         case -EINVAL:
> >                 /* Happens, for instance, if the user specified a link
> >                  * local IPv6 address without a scope-id.
> > @@ -2291,18 +2275,22 @@ static void xs_tcp_setup_socket(struct work_struct *work)
> >         case -EHOSTUNREACH:
> >         case -EADDRINUSE:
> >         case -ENOBUFS:
> > -               /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > -                * We need to wake them first to ensure the correct error code.
> > -                */
> > -               xprt_wake_pending_tasks(xprt, status);
> > -               xs_tcp_force_close(xprt);
> > -               goto out;
> > +               break;
> > +       default:
> > +               printk("%s: connect returned unhandled error %d\n",
> > +                       __func__, status);
> > +               status = -EAGAIN;
> >         }
> > -       status = -EAGAIN;
> > +
> > +       /* xs_tcp_force_close() wakes tasks with a fixed error code.
> > +        * We need to wake them first to ensure the correct error code.
> > +        */
> > +       xprt_wake_pending_tasks(xprt, status);
> > +       xs_tcp_force_close(xprt);
>
> Isn't this a problem to call xs_tcp_force_close() unconditionally at
> the bottom of xs_tcp_setup_socket()?
> Before this patch we only called this depending on certain returns
> from kernel_connect().
>
Nevermind - I see the fallthroughs and out_unlock

>
>
> >  out:
> >         xprt_clear_connecting(xprt);
> > +out_unlock:
> >         xprt_unlock_connect(xprt, transport);
> > -       xprt_wake_pending_tasks(xprt, status);
> >  }
> >
> >  /**
> > --
> > 2.31.1
> >