From: Dave Wysochanski <dwysocha@redhat.com>
To: Trond Myklebust <trondmy@hammerspace.com>,
"anna.schumaker@netapp.com" <anna.schumaker@netapp.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] SUNRPC: Ensure XPRT_CONNECTED is cleared while handling TCP RST
Date: Fri, 14 Dec 2018 08:48:50 -0500 [thread overview]
Message-ID: <1544795330.4709.3.camel@redhat.com> (raw)
In-Reply-To: <03049f893dfd265abb90fd2692bc41e1534f85d0.camel@hammerspace.com>
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@redhat.com>
> > ---
> > 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);
next prev parent reply other threads:[~2018-12-14 13:48 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-12 13:51 [PATCH 0/1] SUNRPC: Ensure XPRT_CONNECTED is cleared while handling TCP RST Dave Wysochanski
2018-12-12 13:51 ` [PATCH 1/1] " Dave Wysochanski
2018-12-12 16:56 ` Trond Myklebust
2018-12-12 17:47 ` Dave Wysochanski
2018-12-12 18:02 ` Trond Myklebust
2018-12-12 19:56 ` Dave Wysochanski
2019-01-08 12:46 ` Benjamin Coddington
2018-12-14 13:48 ` Dave Wysochanski [this message]
2018-12-14 18:29 ` Scott Mayhew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1544795330.4709.3.camel@redhat.com \
--to=dwysocha@redhat.com \
--cc=anna.schumaker@netapp.com \
--cc=linux-nfs@vger.kernel.org \
--cc=trondmy@hammerspace.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).