* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.