On Wed, 2018-12-12 at 16:56 +0000, Trond Myklebust wrote: > On Wed, 2018-12-12 at 08:51 -0500, Dave Wysochanski wrote: > > Commit 9b30889c548a changed the handling of TCP_CLOSE inside > > xs_tcp_state_change. Prior to this change, the XPRT_CONNECTED bit > > was cleared unconditionally inside xprt_disconnect_done, similar > > to the handling of TCP_CLOSE_WAIT. After the change the clearing > > of XPRT_CONNECTED depends on successfully queueing a work based > > xprt_autoclose which depends on XPRT_LOCKED and may not happen. > > This is significant in the case of an unexpected RST from the > > server, as the client will only see xs_tcp_state_change called with > > sk_state == TCP_CLOSE. Restore the unconditional clear_bit on > > XPRT_CONNECTED while handling TCP_CLOSE and make it consistent > > with handling TCP_CLOSE_WAIT. > > > > Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx> > > --- > > net/sunrpc/xprtsock.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > index 8a5e823e0b33..b9789036051d 100644 > > --- a/net/sunrpc/xprtsock.c > > +++ b/net/sunrpc/xprtsock.c > > @@ -1492,6 +1492,7 @@ static void xs_tcp_state_change(struct sock > > *sk) > > if (sk->sk_err) > > xprt_wake_pending_tasks(xprt, -sk- > > >sk_err); > > /* Trigger the socket release */ > > + clear_bit(XPRT_CONNECTED, &xprt->state); > > xs_tcp_force_close(xprt); > > } > > out: > > Hi Dave, > > This isn't needed for 4.20 or newer because call_transmit() will now > always call xprt_end_transmit(). I suggest that a stable fix do > something similar (perhaps conditional on the error returned by > xprt_transmit()?). > > It appears the "Special cases" comment inside call_transmit_status() should be removed now, do you agree? Also I guess you are not concerned about this issue anymore that created the comment and the cases? https://lore.kernel.org/patchwork/patch/165428/ Also for a stable fix, would you prefer handling all of the codes on lines 39-43 the same and removing that comment, or just EPIPE we are seeing in the testcase? After some discussion with Scott we're testing variants on the above (see below for possible fixes). Any preferences on option 1 or option 2? Thanks. Upstream (4.20-rc6) has a comment but it looks wrong due to code in call_transmit(): 1971 static void 1972 call_transmit(struct rpc_task *task) 1973 { 1974 dprint_status(task); 1975 1976 task->tk_status = 0; 1977 if (test_bit(RPC_TASK_NEED_XMIT, &task->tk_runstate)) { 1978 if (!xprt_prepare_transmit(task)) 1979 return; 1980 xprt_transmit(task); 1981 } 1982 task->tk_action = call_transmit_status; 1983 xprt_end_transmit(task); <--------------------- Always drop the XPRT_LOCK 1984 } ... 1989 static void 1990 call_transmit_status(struct rpc_task *task) 1991 { 1992 task->tk_action = call_status; 1993 1994 /* 1995 * Common case: success. Force the compiler to put this 1996 * test first. 1997 */ 1998 if (task->tk_status == 0) { 1999 xprt_request_wait_receive(task); 2000 return; 2001 } 2002 2003 switch (task->tk_status) { 2004 default: 2005 dprint_status(task); 2006 break; 2007 case -EBADMSG: 2008 task->tk_status = 0; 2009 task->tk_action = call_encode; 2010 break; 2011 /* 2012 * Special cases: if we've been waiting on the <---- WRONG 2013 * socket's write_space() callback, or if the <----- WRONG 2014 * socket just returned a connection error, <------- WRONG 2015 * then hold onto the transport lock. <----------- WRONG 2016 */ 2017 case -ENOBUFS: 2018 rpc_delay(task, HZ>>2); 2019 /* fall through */ 2020 case -EBADSLT: 2021 case -EAGAIN: 2022 task->tk_action = call_transmit; 2023 task->tk_status = 0; 2024 break; 2025 case -ECONNREFUSED: 2026 case -EHOSTDOWN: 2027 case -ENETDOWN: 2028 case -EHOSTUNREACH: 2029 case -ENETUNREACH: 2030 case -EPERM: 2031 if (RPC_IS_SOFTCONN(task)) { 2032 if (!task->tk_msg.rpc_proc->p_proc) 2033 trace_xprt_ping(task->tk_xprt, 2034 task->tk_status); 2035 rpc_exit(task, task->tk_status); 2036 break; 2037 } 2038 /* fall through */ 2039 case -ECONNRESET: 2040 case -ECONNABORTED: 2041 case -EADDRINUSE: 2042 case -ENOTCONN: 2043 case -EPIPE: 2044 break; 2045 } 2046 } For 4.16 - 4.19 possible fixes Possible fix #1 - just remove EPIPE and/or all the codes and move these to 'default': 2002 static void 2003 call_transmit_status(struct rpc_task *task) 2004 { 2005 task->tk_action = call_status; 2006 2007 /* 2008 * Common case: success. Force the compiler to put this 2009 * test first. 2010 */ 2011 if (task->tk_status == 0) { 2012 xprt_end_transmit(task); 2013 rpc_task_force_reencode(task); 2014 return; 2015 } 2016 2017 switch (task->tk_status) { 2018 case -EAGAIN: 2019 case -ENOBUFS: 2020 break; 2021 default: 2022 dprint_status(task); 2023 xprt_end_transmit(task); 2024 rpc_task_force_reencode(task); 2025 break; 2026 /* 2027 * Special cases: if we've been waiting on the 2028 * socket's write_space() callback, or if the 2029 * socket just returned a connection error, <----------- WRONG ??? 2030 * then hold onto the transport lock. <-------------- WRONG ??? 2031 */ 2032 case -ECONNREFUSED: 2033 case -EHOSTDOWN: 2034 case -EHOSTUNREACH: 2035 case -ENETUNREACH: 2036 case -EPERM: 2037 if (RPC_IS_SOFTCONN(task)) { 2038 xprt_end_transmit(task); 2039 rpc_exit(task, task->tk_status); 2040 break; 2041 } 2042 case -ECONNRESET: <------- ???? 2043 case -ECONNABORTED: <------ ???? 2044 case -EADDRINUSE: <------- ???? 2045 case -ENOTCONN: <-------- ???? What about these others ??? 2046 case -EPIPE: <------------------------------ remove this line to fix bug 2047 rpc_task_force_reencode(task); 2048 } 2049 } Possible fix #2: make calling xprt_end_transmit() conditional on both EPIPE and xprt_connected() case -EPIPE: + if (xprt_connected(task->tk_xprt)) + xprt_end_transmit(task);
