The server-side sunrpc code currently calls pc_release before sending
the reply. A later nfsd patch will change some pc_release callbacks to
do extra work to clean the pagecache. There is no need to delay sending
the reply for this, however.
Change svc_process and svc_process_bc to call pc_release after sending
the reply instead of before.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
net/sunrpc/svc.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index b1fab3a6954437cf751e4725fa52cfc83eddf2ab..103bb6ba8e140fdccd6cab124e715caeb41bb445 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1426,8 +1426,6 @@ svc_process_common(struct svc_rqst *rqstp)
/* Call the function that processes the request. */
rc = process.dispatch(rqstp);
- if (procp->pc_release)
- procp->pc_release(rqstp);
xdr_finish_decode(xdr);
if (!rc)
@@ -1526,6 +1524,14 @@ static void svc_drop(struct svc_rqst *rqstp)
trace_svc_drop(rqstp);
}
+static void svc_release_rqst(struct svc_rqst *rqstp)
+{
+ const struct svc_procedure *procp = rqstp->rq_procinfo;
+
+ if (procp && procp->pc_release)
+ procp->pc_release(rqstp);
+}
+
/**
* svc_process - Execute one RPC transaction
* @rqstp: RPC transaction context
@@ -1533,7 +1539,7 @@ static void svc_drop(struct svc_rqst *rqstp)
*/
void svc_process(struct svc_rqst *rqstp)
{
- struct kvec *resv = &rqstp->rq_res.head[0];
+ struct kvec *resv = &rqstp->rq_res.head[0];
__be32 *p;
#if IS_ENABLED(CONFIG_FAIL_SUNRPC)
@@ -1565,9 +1571,12 @@ void svc_process(struct svc_rqst *rqstp)
if (unlikely(*p != rpc_call))
goto out_baddir;
- if (!svc_process_common(rqstp))
+ if (!svc_process_common(rqstp)) {
+ svc_release_rqst(rqstp);
goto out_drop;
+ }
svc_send(rqstp);
+ svc_release_rqst(rqstp);
return;
out_baddir:
@@ -1635,6 +1644,7 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
if (!proc_error) {
/* Processing error: drop the request */
xprt_free_bc_request(req);
+ svc_release_rqst(rqstp);
return;
}
/* Finally, send the reply synchronously */
@@ -1648,6 +1658,7 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp)
timeout.to_maxval = timeout.to_initval;
memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
task = rpc_run_bc_task(req, &timeout);
+ svc_release_rqst(rqstp);
if (IS_ERR(task))
return;
--
2.50.0On Fri, 04 Jul 2025, Jeff Layton wrote:
> The server-side sunrpc code currently calls pc_release before sending
> the reply. A later nfsd patch will change some pc_release callbacks to
> do extra work to clean the pagecache. There is no need to delay sending
> the reply for this, however.
>
> Change svc_process and svc_process_bc to call pc_release after sending
> the reply instead of before.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> net/sunrpc/svc.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index b1fab3a6954437cf751e4725fa52cfc83eddf2ab..103bb6ba8e140fdccd6cab124e715caeb41bb445 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1426,8 +1426,6 @@ svc_process_common(struct svc_rqst *rqstp)
>
> /* Call the function that processes the request. */
> rc = process.dispatch(rqstp);
> - if (procp->pc_release)
> - procp->pc_release(rqstp);
> xdr_finish_decode(xdr);
>
> if (!rc)
> @@ -1526,6 +1524,14 @@ static void svc_drop(struct svc_rqst *rqstp)
> trace_svc_drop(rqstp);
> }
>
> +static void svc_release_rqst(struct svc_rqst *rqstp)
> +{
> + const struct svc_procedure *procp = rqstp->rq_procinfo;
> +
> + if (procp && procp->pc_release)
> + procp->pc_release(rqstp);
> +}
> +
> /**
> * svc_process - Execute one RPC transaction
> * @rqstp: RPC transaction context
> @@ -1533,7 +1539,7 @@ static void svc_drop(struct svc_rqst *rqstp)
> */
> void svc_process(struct svc_rqst *rqstp)
> {
> - struct kvec *resv = &rqstp->rq_res.head[0];
> + struct kvec *resv = &rqstp->rq_res.head[0];
Commas and Tabs - you can never really have enough of them, can you?
> __be32 *p;
>
> #if IS_ENABLED(CONFIG_FAIL_SUNRPC)
> @@ -1565,9 +1571,12 @@ void svc_process(struct svc_rqst *rqstp)
> if (unlikely(*p != rpc_call))
> goto out_baddir;
>
> - if (!svc_process_common(rqstp))
> + if (!svc_process_common(rqstp)) {
> + svc_release_rqst(rqstp);
> goto out_drop;
> + }
> svc_send(rqstp);
> + svc_release_rqst(rqstp);
> return;
Should we, as a general rule, avoid calling any cleanup function more
than once? When tempted, we DEFINE_FREE() a cleanup function and
declare the variable appropriately.
Though in this case it might be easier to:
if (svc_process_common(rqstp))
svc_send(rqstp);
else
svc_drop(rqstp);
svc_rlease_rqst(rqstp);
return;
svc_process_bc() is a little more awkward.
But in general, delaying the release function until after the send seems
sound, and this patches appears to do it corretly.
Reviewed-by: NeilBrown <neil@brown.name>
NeilBrown
On Fri, 2025-07-04 at 09:33 +1000, NeilBrown wrote:
> On Fri, 04 Jul 2025, Jeff Layton wrote:
> > The server-side sunrpc code currently calls pc_release before sending
> > the reply. A later nfsd patch will change some pc_release callbacks to
> > do extra work to clean the pagecache. There is no need to delay sending
> > the reply for this, however.
> >
> > Change svc_process and svc_process_bc to call pc_release after sending
> > the reply instead of before.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > net/sunrpc/svc.c | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index b1fab3a6954437cf751e4725fa52cfc83eddf2ab..103bb6ba8e140fdccd6cab124e715caeb41bb445 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1426,8 +1426,6 @@ svc_process_common(struct svc_rqst *rqstp)
> >
> > /* Call the function that processes the request. */
> > rc = process.dispatch(rqstp);
> > - if (procp->pc_release)
> > - procp->pc_release(rqstp);
> > xdr_finish_decode(xdr);
> >
> > if (!rc)
> > @@ -1526,6 +1524,14 @@ static void svc_drop(struct svc_rqst *rqstp)
> > trace_svc_drop(rqstp);
> > }
> >
> > +static void svc_release_rqst(struct svc_rqst *rqstp)
> > +{
> > + const struct svc_procedure *procp = rqstp->rq_procinfo;
> > +
> > + if (procp && procp->pc_release)
> > + procp->pc_release(rqstp);
> > +}
> > +
> > /**
> > * svc_process - Execute one RPC transaction
> > * @rqstp: RPC transaction context
> > @@ -1533,7 +1539,7 @@ static void svc_drop(struct svc_rqst *rqstp)
> > */
> > void svc_process(struct svc_rqst *rqstp)
> > {
> > - struct kvec *resv = &rqstp->rq_res.head[0];
> > + struct kvec *resv = &rqstp->rq_res.head[0];
>
> Commas and Tabs - you can never really have enough of them, can you?
>
Not sure what happened there. I'll drop that hunk.
> > __be32 *p;
> >
> > #if IS_ENABLED(CONFIG_FAIL_SUNRPC)
> > @@ -1565,9 +1571,12 @@ void svc_process(struct svc_rqst *rqstp)
> > if (unlikely(*p != rpc_call))
> > goto out_baddir;
> >
> > - if (!svc_process_common(rqstp))
> > + if (!svc_process_common(rqstp)) {
> > + svc_release_rqst(rqstp);
> > goto out_drop;
> > + }
> > svc_send(rqstp);
> > + svc_release_rqst(rqstp);
> > return;
>
> Should we, as a general rule, avoid calling any cleanup function more
> than once? When tempted, we DEFINE_FREE() a cleanup function and
> declare the variable appropriately.
I'm not opposed to that. I think that change probably deserves a
separate patch.
> Though in this case it might be easier to:
>
> if (svc_process_common(rqstp))
> svc_send(rqstp);
> else
> svc_drop(rqstp);
> svc_rlease_rqst(rqstp);
> return;
>
There is another place that does a "goto out_drop in that function. I'm
not sure changing that would improve things, but I'll see how it looks.
> svc_process_bc() is a little more awkward.
>
Definitely.
> But in general, delaying the release function until after the send seems
> sound, and this patches appears to do it corretly.
>
> Reviewed-by: NeilBrown <neil@brown.name>
>
> NeilBrown
Thanks for the review!
--
Jeff Layton <jlayton@kernel.org>
© 2016 - 2025 Red Hat, Inc.