Re: [PATCH RFC 1/2] sunrpc: delay pc_release callback until after sending a reply

[Jeff Layton <jlayton@kernel.org>]

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>