On Thu, 24 Apr 2025, cel@xxxxxxxxxx wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> 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.
>
> 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?
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...
NeilBrown
>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> 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
>
>
