* [PATCH 1/4] SUNRPC: Fix races when closing the socket @ 2021-10-29 20:04 trondmy 2021-10-29 20:04 ` [PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock trondmy 2021-10-30 14:53 ` [PATCH 1/4] SUNRPC: Fix races when closing the socket David Wysochanski 0 siblings, 2 replies; 7+ messages in thread From: trondmy @ 2021-10-29 20:04 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock 2021-10-29 20:04 [PATCH 1/4] SUNRPC: Fix races when closing the socket trondmy @ 2021-10-29 20:04 ` trondmy 2021-10-29 20:04 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() trondmy 2021-10-30 14:53 ` [PATCH 1/4] SUNRPC: Fix races when closing the socket David Wysochanski 1 sibling, 1 reply; 7+ messages in thread From: trondmy @ 2021-10-29 20:04 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() 2021-10-29 20:04 ` [PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock trondmy @ 2021-10-29 20:04 ` trondmy 2021-10-29 20:04 ` [PATCH 4/4] SUNRPC: Prevent immediate close+reconnect trondmy 2021-12-03 16:10 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() David Wysochanski 0 siblings, 2 replies; 7+ messages in thread From: trondmy @ 2021-10-29 20:04 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/4] SUNRPC: Prevent immediate close+reconnect 2021-10-29 20:04 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() trondmy @ 2021-10-29 20:04 ` trondmy 2021-12-03 16:10 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() David Wysochanski 1 sibling, 0 replies; 7+ messages in thread From: trondmy @ 2021-10-29 20:04 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() 2021-10-29 20:04 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() trondmy 2021-10-29 20:04 ` [PATCH 4/4] SUNRPC: Prevent immediate close+reconnect trondmy @ 2021-12-03 16:10 ` David Wysochanski 2021-12-03 16:36 ` David Wysochanski 1 sibling, 1 reply; 7+ messages in thread From: David Wysochanski @ 2021-12-03 16:10 UTC (permalink / raw) To: trondmy; +Cc: linux-nfs 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() 2021-12-03 16:10 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() David Wysochanski @ 2021-12-03 16:36 ` David Wysochanski 0 siblings, 0 replies; 7+ messages in thread From: David Wysochanski @ 2021-12-03 16:36 UTC (permalink / raw) To: trondmy; +Cc: linux-nfs 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] SUNRPC: Fix races when closing the socket 2021-10-29 20:04 [PATCH 1/4] SUNRPC: Fix races when closing the socket trondmy 2021-10-29 20:04 ` [PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock trondmy @ 2021-10-30 14:53 ` David Wysochanski 1 sibling, 0 replies; 7+ messages in thread From: David Wysochanski @ 2021-10-30 14:53 UTC (permalink / raw) To: trondmy; +Cc: Kornievskaia, Olga, linux-nfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-03 16:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-29 20:04 [PATCH 1/4] SUNRPC: Fix races when closing the socket trondmy 2021-10-29 20:04 ` [PATCH 2/4] SUNRPC: Replace use of socket sk_callback_lock with sock_lock trondmy 2021-10-29 20:04 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() trondmy 2021-10-29 20:04 ` [PATCH 4/4] SUNRPC: Prevent immediate close+reconnect trondmy 2021-12-03 16:10 ` [PATCH 3/4] SUNRPC: Clean up xs_tcp_setup_sock() David Wysochanski 2021-12-03 16:36 ` David Wysochanski 2021-10-30 14:53 ` [PATCH 1/4] SUNRPC: Fix races when closing the socket David Wysochanski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox