| Message ID | 20191016141546.32277-3-trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding | expand |
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. 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. Thanks, NeilBrown > /* > * Tear down transport state and free the rpc_xprt > */ > -- > 2.21.0
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
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); + /* * Tear down transport state and free the rpc_xprt */
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(+)