Re: NFS server regression in kernel 5.13 (tested w/ 5.13.9)

[Chuck Lever III <chuck.lever@oracle.com>]

> On Aug 27, 2021, at 3:14 AM, NeilBrown <neilb@suse.de> wrote:
> 
> Subject: [PATCH] SUNRPC: don't pause on incomplete allocation
> 
> alloc_pages_bulk_array() attempts to allocate at least one page based on
> the provided pages, and then opportunistically allocates more if that
> can be done without dropping the spinlock.
> 
> So if it returns fewer than requested, that could just mean that it
> needed to drop the lock.  In that case, try again immediately.
> 
> Only pause for a time if no progress could be made.

The case I was worried about was "no pages available on the
pcplist", in which case, alloc_pages_bulk_array() resorts
to calling __alloc_pages() and returns only one new page.

"No progess" would mean even __alloc_pages() failed.

So this patch would behave essentially like the
pre-alloc_pages_bulk_array() code: call alloc_page() for
each empty struct_page in the array without pausing. That
seems correct to me.


I would add

Fixes: f6e70aab9dfe ("SUNRPC: refresh rq_pages using a bulk page allocator")


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> net/sunrpc/svc_xprt.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index d66a8e44a1ae..99268dd95519 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -662,7 +662,7 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
> {
> 	struct svc_serv *serv = rqstp->rq_server;
> 	struct xdr_buf *arg = &rqstp->rq_arg;
> -	unsigned long pages, filled;
> +	unsigned long pages, filled, prev;
> 
> 	pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
> 	if (pages > RPCSVC_MAXPAGES) {
> @@ -672,11 +672,14 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
> 		pages = RPCSVC_MAXPAGES;
> 	}
> 
> -	for (;;) {
> +	for (prev = 0;; prev = filled) {
> 		filled = alloc_pages_bulk_array(GFP_KERNEL, pages,
> 						rqstp->rq_pages);
> 		if (filled == pages)
> 			break;
> +		if (filled > prev)
> +			/* Made progress, don't sleep yet */
> +			continue;
> 
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		if (signalled() || kthread_should_stop()) {

--
Chuck Lever