Re: [PATCH 3/3] SUNRPC: Destroy the back channel when we destroy the host transport

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2019-10-17 at 09:08 +1100, NeilBrown wrote:
> On Wed, Oct 16 2019, Trond Myklebust wrote:
> 
> > When we're destroying the host transport mechanism, we should
> > ensure
> > that we do not leak memory by failing to release any back channel
> > slots that might still exist.
> > 
> > Reported-by: Neil Brown <neilb@suse.de>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  net/sunrpc/xprt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index 8a45b3ccc313..41df4c507193 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1942,6 +1942,11 @@ static void xprt_destroy_cb(struct
> > work_struct *work)
> >  	rpc_destroy_wait_queue(&xprt->sending);
> >  	rpc_destroy_wait_queue(&xprt->backlog);
> >  	kfree(xprt->servername);
> > +	/*
> > +	 * Destroy any existing back channel
> > +	 */
> > +	xprt_destroy_backchannel(xprt, UINT_MAX);
> > +
> 
> This will cause xprt->bc_alloc_max to become meaningless.

Fixed in v2.

> That isn't really a problem as the xprt is about to be freed, but it
> still seems a little untidy - fragile maybe.
> How about another line in the comment:
> 
>    * Note: this corrupts ->bc_alloc_max, but it is too late for that
> to
>    * matter.
> ??
> 
> Also, possibly add
>  WARN_ON(atomic_read(&xprt->bc_slot_count);
> either before or after the xprt_destroy_backchannel - because there
> really shouldn't be any requests by this stage.

I considered it, but since RDMA doesn't use those variable, it wouldn't
really be a sufficient check.

Thanks
  Trond

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