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

[Trond Myklebust <trondmy@hammerspace.com>]

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().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com