| Message ID | 20220420182121.87341-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] SUNRPC release the transport of a relocated task with an assigned transport | expand |
On Wed, 2022-04-20 at 14:21 -0400, Olga Kornievskaia wrote: > From: Olga Kornievskaia <kolga@netapp.com> > > A relocated task must release its previous transport. > > Fixes: 82ee41b85cef1 ("SUNRPC don't resend a task on an offlined > transport") > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > net/sunrpc/clnt.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > index af0174d7ce5a..95de454a858b 100644 > --- a/net/sunrpc/clnt.c > +++ b/net/sunrpc/clnt.c > @@ -1067,8 +1067,10 @@ void rpc_task_set_transport(struct rpc_task > *task, struct rpc_clnt *clnt) > { > if (task->tk_xprt && > !(test_bit(XPRT_OFFLINE, &task->tk_xprt- > >state) && > - (task->tk_flags & RPC_TASK_MOVEABLE))) > + (task->tk_flags & RPC_TASK_MOVEABLE))) { > + xprt_release_write(task->tk_xprt, task); > return; > + } > if (task->tk_flags & RPC_TASK_NO_ROUND_ROBIN) > task->tk_xprt = rpc_task_get_first_xprt(clnt); > else I don't think this patch is correct. It is changing the case where we're not changing the value of task->tk_xprt, which was never a problem before commit 82ee41b85cef. What needs to change is the case where task->tk_xprt is set, but we're going to change its value anyway because of the tests for XPRT_OFFLINE and RPC_TASK_MOVEABLE. In that case we need to call xprt_release(), followed by xprt_put(). Otherwise we're leaking slots, locks and references to the old task- >tk_xprt.
On Wed, Apr 20, 2022 at 2:41 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2022-04-20 at 14:21 -0400, Olga Kornievskaia wrote: > > From: Olga Kornievskaia <kolga@netapp.com> > > > > A relocated task must release its previous transport. > > > > Fixes: 82ee41b85cef1 ("SUNRPC don't resend a task on an offlined > > transport") > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > --- > > net/sunrpc/clnt.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c > > index af0174d7ce5a..95de454a858b 100644 > > --- a/net/sunrpc/clnt.c > > +++ b/net/sunrpc/clnt.c > > @@ -1067,8 +1067,10 @@ void rpc_task_set_transport(struct rpc_task > > *task, struct rpc_clnt *clnt) > > { > > if (task->tk_xprt && > > !(test_bit(XPRT_OFFLINE, &task->tk_xprt- > > >state) && > > - (task->tk_flags & RPC_TASK_MOVEABLE))) > > + (task->tk_flags & RPC_TASK_MOVEABLE))) { > > + xprt_release_write(task->tk_xprt, task); > > return; > > + } > > if (task->tk_flags & RPC_TASK_NO_ROUND_ROBIN) > > task->tk_xprt = rpc_task_get_first_xprt(clnt); > > else > > > I don't think this patch is correct. It is changing the case where > we're not changing the value of task->tk_xprt, which was never a > problem before commit 82ee41b85cef. > > What needs to change is the case where task->tk_xprt is set, but we're > going to change its value anyway because of the tests for XPRT_OFFLINE > and RPC_TASK_MOVEABLE. > In that case we need to call xprt_release(), followed by xprt_put(). > Otherwise we're leaking slots, locks and references to the old task- > >tk_xprt. Thank you Trond. It is indeed incorrect. I've sent v2. > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index af0174d7ce5a..95de454a858b 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -1067,8 +1067,10 @@ void rpc_task_set_transport(struct rpc_task *task, struct rpc_clnt *clnt) { if (task->tk_xprt && !(test_bit(XPRT_OFFLINE, &task->tk_xprt->state) && - (task->tk_flags & RPC_TASK_MOVEABLE))) + (task->tk_flags & RPC_TASK_MOVEABLE))) { + xprt_release_write(task->tk_xprt, task); return; + } if (task->tk_flags & RPC_TASK_NO_ROUND_ROBIN) task->tk_xprt = rpc_task_get_first_xprt(clnt); else