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 <[email protected]>
---
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 {
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 <[email protected]> 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 <[email protected]>
> ---
> 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 {
>
>
> On Jul 25, 2022, at 10:19 AM, Olga Kornievskaia <[email protected]> wrote:
>
> 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?
tl;dr: the latter. But it's not an issue of memory consumption, it's
that this whole struct is cleared with memset(0) for each incoming
RPC.
nfsd4_op is a union -- it takes the size of the largest args struct
in that union. Before copy offload was introduced, that was about
250 bytes. After, it is ~10x larger.
There are 8 nfsd4_op structs in an array in nfsd4_compoundargs.
The nfsd4_compoundargs structure is cleared by
svc_generic_init_request() before each NFSv4 RPC is executed.
So, in this case, the problem is 8 x 2KB = ~17KB to clear on
each RPC call for an operation that is quite infrequently
requested.
In NFSD, typically the execution context is amenable to GFP_KERNEL,
so it's generally OK to allocate dynamically for infrequently
requested operations.
--
Chuck Lever