On Thu, 2019-10-17 at 09:24 +1100, NeilBrown wrote:
> On Wed, Oct 16 2019, Trond Myklebust wrote:
>
> > If there are TCP back channel requests either being processed by
> > the
> > server threads, then we should hold a reference to the transport
> > to ensure it doesn't get freed from underneath us.
> >
> > Reported-by: Neil Brown <neilb@xxxxxxx>
> > Fixes: 2ea24497a1b3 ("SUNRPC: RPC callbacks may be split across
> > several..")
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > net/sunrpc/backchannel_rqst.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sunrpc/backchannel_rqst.c
> > b/net/sunrpc/backchannel_rqst.c
> > index 339e8c077c2d..7eb251372f94 100644
> > --- a/net/sunrpc/backchannel_rqst.c
> > +++ b/net/sunrpc/backchannel_rqst.c
> > @@ -307,8 +307,8 @@ void xprt_free_bc_rqst(struct rpc_rqst *req)
> > */
> > dprintk("RPC: Last session removed req=%p\n",
> > req);
> > xprt_free_allocation(req);
> > - return;
> > }
> > + xprt_put(xprt);
> > }
> >
> > /*
> > @@ -339,7 +339,7 @@ struct rpc_rqst *xprt_lookup_bc_request(struct
> > rpc_xprt *xprt, __be32 xid)
> > spin_unlock(&xprt->bc_pa_lock);
> > if (new) {
> > if (req != new)
> > - xprt_free_bc_rqst(new);
> > + xprt_free_allocation(new);
> > break;
> > } else if (req)
> > break;
> > @@ -368,6 +368,7 @@ void xprt_complete_bc_request(struct rpc_rqst
> > *req, uint32_t copied)
> > set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
> >
> > dprintk("RPC: add callback request to list\n");
> > + xprt_get(xprt);
> > spin_lock(&bc_serv->sv_cb_lock);
> > list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
> > wake_up(&bc_serv->sv_cb_waitq);
> > --
> > 2.21.0
>
> Looks good.
> This and the next two:
> Reviewed-by: NeilBrown <neilb@xxxxxxx>
>
> It would help me if you could add a Fixes: tag to at least the first
> two.
Neither have Cc:stable, but they both already have Fixes: tags. See
above.
>
> BTW, while reviewing I notices that bc_alloc_count and bc_slot_count
> are
> almost identical. The three places were that are changed separately
> are
> probably (minor) bugs.
> Do you recall why there were two different counters? Has the reason
> disappeared?
IIRC, the former contains the count of preallocated slots, and the
latter the count of preallocated+dynamic slots.
Thanks
Trond
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
