| Message ID | 20210202184244.288898-5-Anna.Schumaker@Netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: Create sysfs files for changing IP | expand |
On Tue, 2021-02-02 at 13:42 -0500, schumaker.anna@gmail.com wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > We won't have a task structure when we go to change IP addresses, so > check for one before calling the WARN_ON() to avoid crashing. > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > net/sunrpc/xprtsock.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > index c56a66cdf4ac..250abf1aa018 100644 > --- a/net/sunrpc/xprtsock.c > +++ b/net/sunrpc/xprtsock.c > @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt, > struct rpc_task *task) > struct sock_xprt *transport = container_of(xprt, struct > sock_xprt, xprt); > unsigned long delay = 0; > > - WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport)); > + if (task) > + WARN_ON_ONCE(!xprt_lock_connect(xprt, task, > transport)); Nit: That's the same as WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport)); > > if (transport->sock != NULL) { > dprintk("RPC: xs_connect delayed xprt %p for > %lu "
On Tue, 2021-02-02 at 16:49 -0500, Trond Myklebust wrote: > On Tue, 2021-02-02 at 13:42 -0500, schumaker.anna@gmail.com wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > We won't have a task structure when we go to change IP addresses, > > so > > check for one before calling the WARN_ON() to avoid crashing. > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > net/sunrpc/xprtsock.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > index c56a66cdf4ac..250abf1aa018 100644 > > --- a/net/sunrpc/xprtsock.c > > +++ b/net/sunrpc/xprtsock.c > > @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt, > > struct rpc_task *task) > > struct sock_xprt *transport = container_of(xprt, struct > > sock_xprt, xprt); > > unsigned long delay = 0; > > > > - WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport)); > > + if (task) > > + WARN_ON_ONCE(!xprt_lock_connect(xprt, task, > > transport)); > > Nit: That's the same as > WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport)); > > > > > if (transport->sock != NULL) { > > dprintk("RPC: xs_connect delayed xprt %p for > > %lu " > So, the problem with this patch is that you're deliberately circumventing the process of locking the socket. That's not going to work. What you could do is follow the example set by xprt_destroy() and xs_enable_swap(), to call wait_on_bit_lock() when there is no task. That should work, but you'd better make sure that your process holds a reference to the xprt->kref before doing that, or else you could easily end up deadlocking with xprt_destroy().
On Tue, Feb 2, 2021 at 4:59 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Tue, 2021-02-02 at 16:49 -0500, Trond Myklebust wrote: > > On Tue, 2021-02-02 at 13:42 -0500, schumaker.anna@gmail.com wrote: > > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > > > We won't have a task structure when we go to change IP addresses, > > > so > > > check for one before calling the WARN_ON() to avoid crashing. > > > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > --- > > > net/sunrpc/xprtsock.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c > > > index c56a66cdf4ac..250abf1aa018 100644 > > > --- a/net/sunrpc/xprtsock.c > > > +++ b/net/sunrpc/xprtsock.c > > > @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt, > > > struct rpc_task *task) > > > struct sock_xprt *transport = container_of(xprt, struct > > > sock_xprt, xprt); > > > unsigned long delay = 0; > > > > > > - WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport)); > > > + if (task) > > > + WARN_ON_ONCE(!xprt_lock_connect(xprt, task, > > > transport)); > > > > Nit: That's the same as > > WARN_ON_ONCE(task && !xprt_lock_connect(xprt, task, transport)); > > > > > > > > if (transport->sock != NULL) { > > > dprintk("RPC: xs_connect delayed xprt %p for > > > %lu " > > > > So, the problem with this patch is that you're deliberately > circumventing the process of locking the socket. That's not going to > work. > What you could do is follow the example set by xprt_destroy() and > xs_enable_swap(), to call wait_on_bit_lock() when there is no task. > That should work, but you'd better make sure that your process holds a > reference to the xprt->kref before doing that, or else you could easily > end up deadlocking with xprt_destroy(). I've tried this, and the kref isn't a problem but no matter where I put the wait_on_bit_lock() call it ends up deadlocking. I think it's happening in the xs_tcp_setup_socket() function, but I'm still trying to figure out exactly where. Anna > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index c56a66cdf4ac..250abf1aa018 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -2311,7 +2311,8 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task) struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt); unsigned long delay = 0; - WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport)); + if (task) + WARN_ON_ONCE(!xprt_lock_connect(xprt, task, transport)); if (transport->sock != NULL) { dprintk("RPC: xs_connect delayed xprt %p for %lu "