diff mbox series

[v2,4/5] sunrpc: Prepare xs_connect() for taking NULL tasks

Message ID 20210202184244.288898-5-Anna.Schumaker@Netapp.com (mailing list archive)
State New
Headers show
Series SUNRPC: Create sysfs files for changing IP | expand

Commit Message

Anna Schumaker Feb. 2, 2021, 6:42 p.m. UTC
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(-)

Comments

Trond Myklebust Feb. 2, 2021, 9:49 p.m. UTC | #1
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 "
Trond Myklebust Feb. 2, 2021, 9:59 p.m. UTC | #2
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().
Anna Schumaker Feb. 5, 2021, 6:33 p.m. UTC | #3
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 mbox series

Patch

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 "