Re: [PATCH v3 11/11] NFSD: Remove NFSSVC_MAXBLKSIZE from .pc_xdrressize

[Chuck Lever <cel@kernel.org>]

On 4/26/25 12:31 AM, NeilBrown wrote:
> On Thu, 24 Apr 2025, cel@kernel.org wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> The value in the .pc_xdrressize field is used to "reserve space in
>> the output queue". Relevant only to UDP transports, AFAICT.
>>
>> The fixed value of NFSSVC_MAXBLKSIZE is added to that field for
>> NFSv2 and NFSv3 read requests, even though nfsd_proc_read() is
>> already careful to reserve the actual size of the read payload.
>> Adding the maximum payload size to .pc_xdrressize seems to be
>> unnecessary.
> 
> I believe it is necessary.
> 
> svc_reserve() (and svc_reserve_auth) only ever reduces the size of the
> reservation.
> ->rq_reserved is initialised to serv->sv_max_mesg, then reduced to
> .pc_xdrressize once the proc is known, then possibly reduced further by
> code in the proc.
> So .pc_xdrressize must be (at least) the largest possible size.

Hrm. I instrumented this code path. It seemed to be doing exactly
what I expected. The behavior of xdr_reserve_space() is to /increase/
buffer space reservation, so svc_reserve() seems to behave in the
opposite way, then?

So if the maximum payload size is no longer a constant, these
pc_xdrressize values still have to add the largest payload size that
NFSD can support (probably will be 8MB), not the max-payload-size
setting in effect for that nfsd thread pool.


>> Also, instead of adding a constant 4 bytes for each payload's
>> XDR pad, add the actual size of the pad for better accuracy of
>> the reservation size.
> 
> Could we instead change svc_reserve() to add the pad, and remove all the
> manual padding?

The padding is needed only for a few certain operations; READ and
READLINK, I think that's it. NFSv4 GETATTR might need it too.

IOW the common case is that no padding is needed.

The largest payload, even if it needs an XDR pad, will be
NFSSVC_MAXBLKSIZE.


> But pc_xdrressize is in xdr units - it is multiplied by 4 before passing
> to svc_reserve.  So these changes don't do what you think they do...

Fair enough. I can drop "NFSD: Remove NFSSVC_MAXBLKSIZE from
.pc_xdrressize" and "NFSD: Remove NFSD_BUFSIZE".

I find the name NFSSVC_MAXBLKSIZE confusing, though. NFS is a file
protocol, so I'm not clear what a "block" is in this context.

Also, am I correct that the only transport that cares about this
send buffer reservation is UDP?


> NeilBrown
> 
> 
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfsd/nfs3proc.c | 4 ++--
>>  fs/nfsd/nfsproc.c  | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 372bdcf5e07a..dbb750a7b5db 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -202,7 +202,7 @@ nfsd3_proc_read(struct svc_rqst *rqstp)
>>  	 */
>>  	resp->count = argp->count;
>>  	svc_reserve_auth(rqstp, ((1 + NFS3_POST_OP_ATTR_WORDS + 3) << 2) +
>> -			 resp->count + 4);
>> +			 xdr_align_size(resp->count));
>>  
>>  	fh_copy(&resp->fh, &argp->fh);
>>  	resp->status = nfsd_read(rqstp, &resp->fh, argp->offset,
>> @@ -921,7 +921,7 @@ static const struct svc_procedure nfsd_procedures3[22] = {
>>  		.pc_argzero = sizeof(struct nfsd3_readargs),
>>  		.pc_ressize = sizeof(struct nfsd3_readres),
>>  		.pc_cachetype = RC_NOCACHE,
>> -		.pc_xdrressize = ST+pAT+4+NFSSVC_MAXBLKSIZE/4,
>> +		.pc_xdrressize = ST+pAT+3,
>>  		.pc_name = "READ",
>>  	},
>>  	[NFS3PROC_WRITE] = {
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index 6dda081eb24c..a95faf726e58 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -219,7 +219,7 @@ nfsd_proc_read(struct svc_rqst *rqstp)
>>  	/* Obtain buffer pointer for payload. 19 is 1 word for
>>  	 * status, 17 words for fattr, and 1 word for the byte count.
>>  	 */
>> -	svc_reserve_auth(rqstp, (19<<2) + argp->count + 4);
>> +	svc_reserve_auth(rqstp, (19<<2) + xdr_align_size(argp->count));
>>  
>>  	resp->count = argp->count;
>>  	fh_copy(&resp->fh, &argp->fh);
>> @@ -739,7 +739,7 @@ static const struct svc_procedure nfsd_procedures2[18] = {
>>  		.pc_argzero = sizeof(struct nfsd_readargs),
>>  		.pc_ressize = sizeof(struct nfsd_readres),
>>  		.pc_cachetype = RC_NOCACHE,
>> -		.pc_xdrressize = ST+AT+1+NFSSVC_MAXBLKSIZE_V2/4,
>> +		.pc_xdrressize = ST+AT+1,
>>  		.pc_name = "READ",
>>  	},
>>  	[NFSPROC_WRITECACHE] = {
>> -- 
>> 2.49.0
>>
>>
> 
> 


-- 
Chuck Lever