[PATCH 5.10.y] nfsd: Don't call freezable_schedule_timeout() after each successful page allocation in svc_alloc_arg().

[PATCH 5.10.y] nfsd: Don't call freezable_schedule_timeout() after each successful page allocation in svc_alloc_arg().

[Kuniyuki Iwashima]

When commit 390390240145 ("nfsd: don't allow nfsd threads to be
signalled.") is backported to 5.10, it was adjusted considering commit
3feac2b55293 ("sunrpc: exclude from freezer when waiting for requests:").

However, 3feac2b55293 is based on commit f6e70aab9dfe ("SUNRPC: refresh
rq_pages using a bulk page allocator"), which converted page-by-page
allocation to a batch allocation, so schedule_timeout() is placed
un-nested.

As a result, the backported commit 7229200f6866 ("nfsd: don't allow nfsd
threads to be signalled.") placed freezable_schedule_timeout() in the wrong
place.

Now, freezable_schedule_timeout() is called after every successful page
allocation, and we see 30%+ performance regression on 5.10.220 in our
test suite.

Let's move it to the correct place so that freezable_schedule_timeout()
is called only when page allocation fails.

Fixes: 7229200f6866 ("nfsd: don't allow nfsd threads to be signalled.")
Reported-by: Hughdan Liu <hughliu@amazon.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/sunrpc/svc_xprt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index d1eacf3358b8..60782504ad3e 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -679,8 +679,8 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
 					set_current_state(TASK_RUNNING);
 					return -EINTR;
 				}
+				freezable_schedule_timeout(msecs_to_jiffies(500));
 			}
-			freezable_schedule_timeout(msecs_to_jiffies(500));
 			rqstp->rq_pages[i] = p;
 		}
 	rqstp->rq_page_end = &rqstp->rq_pages[i];
-- 
2.30.2

Re: [PATCH 5.10.y] nfsd: Don't call freezable_schedule_timeout() after each successful page allocation in svc_alloc_arg().

[Chuck Lever]

On Mon, Aug 19, 2024 at 10:05:51AM -0700, Kuniyuki Iwashima wrote:
> When commit 390390240145 ("nfsd: don't allow nfsd threads to be
> signalled.") is backported to 5.10, it was adjusted considering commit
> 3feac2b55293 ("sunrpc: exclude from freezer when waiting for requests:").
> 
> However, 3feac2b55293 is based on commit f6e70aab9dfe ("SUNRPC: refresh
> rq_pages using a bulk page allocator"), which converted page-by-page
> allocation to a batch allocation, so schedule_timeout() is placed
> un-nested.
> 
> As a result, the backported commit 7229200f6866 ("nfsd: don't allow nfsd
> threads to be signalled.") placed freezable_schedule_timeout() in the wrong
> place.
> 
> Now, freezable_schedule_timeout() is called after every successful page
> allocation, and we see 30%+ performance regression on 5.10.220 in our
> test suite.
> 
> Let's move it to the correct place so that freezable_schedule_timeout()
> is called only when page allocation fails.
> 
> Fixes: 7229200f6866 ("nfsd: don't allow nfsd threads to be signalled.")
> Reported-by: Hughdan Liu <hughliu@amazon.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/sunrpc/svc_xprt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index d1eacf3358b8..60782504ad3e 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -679,8 +679,8 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>  					set_current_state(TASK_RUNNING);
>  					return -EINTR;
>  				}
> +				freezable_schedule_timeout(msecs_to_jiffies(500));
>  			}
> -			freezable_schedule_timeout(msecs_to_jiffies(500));
>  			rqstp->rq_pages[i] = p;
>  		}
>  	rqstp->rq_page_end = &rqstp->rq_pages[i];
> -- 
> 2.30.2
> 

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

-- 
Chuck Lever

Re: [PATCH 5.10.y] nfsd: Don't call freezable_schedule_timeout() after each successful page allocation in svc_alloc_arg().

[Greg Kroah-Hartman]

On Mon, Aug 19, 2024 at 02:11:04PM -0400, Chuck Lever wrote:
> On Mon, Aug 19, 2024 at 10:05:51AM -0700, Kuniyuki Iwashima wrote:
> > When commit 390390240145 ("nfsd: don't allow nfsd threads to be
> > signalled.") is backported to 5.10, it was adjusted considering commit
> > 3feac2b55293 ("sunrpc: exclude from freezer when waiting for requests:").
> > 
> > However, 3feac2b55293 is based on commit f6e70aab9dfe ("SUNRPC: refresh
> > rq_pages using a bulk page allocator"), which converted page-by-page
> > allocation to a batch allocation, so schedule_timeout() is placed
> > un-nested.
> > 
> > As a result, the backported commit 7229200f6866 ("nfsd: don't allow nfsd
> > threads to be signalled.") placed freezable_schedule_timeout() in the wrong
> > place.
> > 
> > Now, freezable_schedule_timeout() is called after every successful page
> > allocation, and we see 30%+ performance regression on 5.10.220 in our
> > test suite.
> > 
> > Let's move it to the correct place so that freezable_schedule_timeout()
> > is called only when page allocation fails.
> > 
> > Fixes: 7229200f6866 ("nfsd: don't allow nfsd threads to be signalled.")
> > Reported-by: Hughdan Liu <hughliu@amazon.com>
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  net/sunrpc/svc_xprt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> > index d1eacf3358b8..60782504ad3e 100644
> > --- a/net/sunrpc/svc_xprt.c
> > +++ b/net/sunrpc/svc_xprt.c
> > @@ -679,8 +679,8 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
> >  					set_current_state(TASK_RUNNING);
> >  					return -EINTR;
> >  				}
> > +				freezable_schedule_timeout(msecs_to_jiffies(500));
> >  			}
> > -			freezable_schedule_timeout(msecs_to_jiffies(500));
> >  			rqstp->rq_pages[i] = p;
> >  		}
> >  	rqstp->rq_page_end = &rqstp->rq_pages[i];
> > -- 
> > 2.30.2
> > 
> 
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

Now queued up, thanks.

greg k-h