Re: [PATCH v1 01/11] NFSD: Shrink size of struct nfsd4_copy_notify

[Olga Kornievskaia <aglo@umich.edu>]

Hi Chuck,

A general question: what's the rule for deciding whether to allocate
statically or dynamically? I thought that "small" structures it's
better to preallocate (statically) for performance reasons. Or is the
idea here that copy_notify/copy are rare operations that instead they
should be allocated dynamically and so that other operations doesn't
consume more memory in nfsd4_op structure?

On Fri, Jul 22, 2022 at 4:35 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> struct nfsd4_copy_notify is part of struct nfsd4_op, which resides
> in an 8-element array.
>
> sizeof(struct nfsd4_op):
> Before: /* size: 2208, cachelines: 35, members: 5 */
> After:  /* size: 1696, cachelines: 27, members: 5 */
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c |    4 ++--
>  fs/nfsd/nfs4xdr.c  |   12 ++++++++++--
>  fs/nfsd/xdr4.h     |    4 ++--
>  3 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3895eb52d2b1..22c5ccb83d20 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1953,9 +1953,9 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         /* For now, only return one server address in cpn_src, the
>          * address used by the client to connect to this server.
>          */
> -       cn->cpn_src.nl4_type = NL4_NETADDR;
> +       cn->cpn_src->nl4_type = NL4_NETADDR;
>         status = nfsd4_set_netaddr((struct sockaddr *)&rqstp->rq_daddr,
> -                                &cn->cpn_src.u.nl4_addr);
> +                                &cn->cpn_src->u.nl4_addr);
>         WARN_ON_ONCE(status);
>         if (status) {
>                 nfs4_put_cpntf_state(nn, cps);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 358b3338c4cc..335431199077 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1952,10 +1952,17 @@ nfsd4_decode_copy_notify(struct nfsd4_compoundargs *argp,
>  {
>         __be32 status;
>
> +       cn->cpn_src = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_src));
> +       if (cn->cpn_src == NULL)
> +               return nfserrno(-ENOMEM);       /* XXX: jukebox? */
> +       cn->cpn_dst = svcxdr_tmpalloc(argp, sizeof(*cn->cpn_dst));
> +       if (cn->cpn_dst == NULL)
> +               return nfserrno(-ENOMEM);       /* XXX: jukebox? */
> +
>         status = nfsd4_decode_stateid4(argp, &cn->cpn_src_stateid);
>         if (status)
>                 return status;
> -       return nfsd4_decode_nl4_server(argp, &cn->cpn_dst);
> +       return nfsd4_decode_nl4_server(argp, cn->cpn_dst);
>  }
>
>  static __be32
> @@ -4898,7 +4905,8 @@ nfsd4_encode_copy_notify(struct nfsd4_compoundres *resp, __be32 nfserr,
>
>         *p++ = cpu_to_be32(1);
>
> -       return nfsd42_encode_nl4_server(resp, &cn->cpn_src);
> +       nfserr = nfsd42_encode_nl4_server(resp, cn->cpn_src);
> +       return nfserr;
>  }
>
>  static __be32
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 6e6a89008ce1..f253fc3f4708 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -595,13 +595,13 @@ struct nfsd4_offload_status {
>  struct nfsd4_copy_notify {
>         /* request */
>         stateid_t               cpn_src_stateid;
> -       struct nl4_server       cpn_dst;
> +       struct nl4_server       *cpn_dst;
>
>         /* response */
>         stateid_t               cpn_cnr_stateid;
>         u64                     cpn_sec;
>         u32                     cpn_nsec;
> -       struct nl4_server       cpn_src;
> +       struct nl4_server       *cpn_src;
>  };
>
>  struct nfsd4_op {
>
>