* fix encode_entryplus_baggage stack usage
@ 2014-01-09 21:33 J. Bruce Fields
2014-01-10 0:19 ` Jeff Layton
0 siblings, 1 reply; 4+ messages in thread
From: J. Bruce Fields @ 2014-01-09 21:33 UTC (permalink / raw)
To: linux-nfs; +Cc: sandeen, jlayton
Eric ran across an excessive stack with this (and a bunch of xfs code)
to blame. Untested. I don't know whether it's worth the minor extra
work here to avoid the alloc/free on each entry.
nfsd4_encode_fattr will need the same treatment.
--b.
commit 5b35a1f99f999555e82420e687e9d079ae796547
Author: J. Bruce Fields <bfields@redhat.com>
Date: Thu Jan 9 16:24:35 2014 -0500
nfsd: fix encode_entryplus_baggage stack usage
We stick an extra svc_fh in nfsd3_readdirres to save the need to
kmalloc, though maybe it would be fine to kmalloc instead.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 1ee6bae..de6e39e 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -842,21 +842,21 @@ out:
static __be32 *encode_entryplus_baggage(struct nfsd3_readdirres *cd, __be32 *p, const char *name, int namlen)
{
- struct svc_fh fh;
+ struct svc_fh *fh = &cd->scratch;
__be32 err;
- fh_init(&fh, NFS3_FHSIZE);
- err = compose_entry_fh(cd, &fh, name, namlen);
+ fh_init(fh, NFS3_FHSIZE);
+ err = compose_entry_fh(cd, fh, name, namlen);
if (err) {
*p++ = 0;
*p++ = 0;
goto out;
}
- p = encode_post_op_attr(cd->rqstp, p, &fh);
+ p = encode_post_op_attr(cd->rqstp, p, fh);
*p++ = xdr_one; /* yes, a file handle follows */
- p = encode_fh(p, &fh);
+ p = encode_fh(p, fh);
out:
- fh_put(&fh);
+ fh_put(fh);
return p;
}
diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
index b6d5542..335e04a 100644
--- a/fs/nfsd/xdr3.h
+++ b/fs/nfsd/xdr3.h
@@ -174,6 +174,9 @@ struct nfsd3_linkres {
struct nfsd3_readdirres {
__be32 status;
struct svc_fh fh;
+ /* Just to save kmalloc on every readdirplus entry (svc_fh is a
+ * little large for the stack): */
+ struct svc_fh scratch;
int count;
__be32 verf[2];
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: fix encode_entryplus_baggage stack usage
2014-01-09 21:33 fix encode_entryplus_baggage stack usage J. Bruce Fields
@ 2014-01-10 0:19 ` Jeff Layton
2014-01-23 18:53 ` [PATCH] nfsd4: decrease nfsd4_encode_fattr " J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Layton @ 2014-01-10 0:19 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, sandeen
On Thu, 9 Jan 2014 16:33:35 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:
> Eric ran across an excessive stack with this (and a bunch of xfs code)
> to blame. Untested. I don't know whether it's worth the minor extra
> work here to avoid the alloc/free on each entry.
>
> nfsd4_encode_fattr will need the same treatment.
>
> --b.
>
> commit 5b35a1f99f999555e82420e687e9d079ae796547
> Author: J. Bruce Fields <bfields@redhat.com>
> Date: Thu Jan 9 16:24:35 2014 -0500
>
> nfsd: fix encode_entryplus_baggage stack usage
>
> We stick an extra svc_fh in nfsd3_readdirres to save the need to
> kmalloc, though maybe it would be fine to kmalloc instead.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index 1ee6bae..de6e39e 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -842,21 +842,21 @@ out:
>
> static __be32 *encode_entryplus_baggage(struct nfsd3_readdirres *cd, __be32 *p, const char *name, int namlen)
> {
> - struct svc_fh fh;
> + struct svc_fh *fh = &cd->scratch;
> __be32 err;
>
> - fh_init(&fh, NFS3_FHSIZE);
> - err = compose_entry_fh(cd, &fh, name, namlen);
> + fh_init(fh, NFS3_FHSIZE);
> + err = compose_entry_fh(cd, fh, name, namlen);
> if (err) {
> *p++ = 0;
> *p++ = 0;
> goto out;
> }
> - p = encode_post_op_attr(cd->rqstp, p, &fh);
> + p = encode_post_op_attr(cd->rqstp, p, fh);
> *p++ = xdr_one; /* yes, a file handle follows */
> - p = encode_fh(p, &fh);
> + p = encode_fh(p, fh);
> out:
> - fh_put(&fh);
> + fh_put(fh);
> return p;
> }
>
> diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
> index b6d5542..335e04a 100644
> --- a/fs/nfsd/xdr3.h
> +++ b/fs/nfsd/xdr3.h
> @@ -174,6 +174,9 @@ struct nfsd3_linkres {
> struct nfsd3_readdirres {
> __be32 status;
> struct svc_fh fh;
> + /* Just to save kmalloc on every readdirplus entry (svc_fh is a
> + * little large for the stack): */
> + struct svc_fh scratch;
> int count;
> __be32 verf[2];
>
I think this is probably the best solution...
Acked-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] nfsd4: decrease nfsd4_encode_fattr stack usage
2014-01-10 0:19 ` Jeff Layton
@ 2014-01-23 18:53 ` J. Bruce Fields
2014-01-23 23:07 ` Jeff Layton
0 siblings, 1 reply; 4+ messages in thread
From: J. Bruce Fields @ 2014-01-23 18:53 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs, sandeen
From: "J. Bruce Fields" <bfields@redhat.com>
A struct svc_fh is 320 bytes on x86_64, it'd be better not to have these
on the stack.
kmalloc'ing them probably isn't ideal either, but this is the simplest
thing to do. If it turns out to be a problem in the readdir case then
we could add a svc_fh to nfsd4_readdir and pass that in.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4xdr.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
On Thu, Jan 09, 2014 at 07:19:34PM -0500, Jeff Layton wrote:
> On Thu, 9 Jan 2014 16:33:35 -0500
> > nfsd: fix encode_entryplus_baggage stack usage
...
> I think this is probably the best solution...
>
> Acked-by: Jeff Layton <jlayton@redhat.com>
Thanks. While we're at it, here's the v4 case.
--b.
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 8198ecf..63f2395 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2058,7 +2058,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
u32 bmval1 = bmval[1];
u32 bmval2 = bmval[2];
struct kstat stat;
- struct svc_fh tempfh;
+ struct svc_fh *tempfh = NULL;
struct kstatfs statfs;
int buflen = count << 2;
__be32 *attrlenp;
@@ -2105,11 +2105,15 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
goto out_nfserr;
}
if ((bmval0 & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) && !fhp) {
- fh_init(&tempfh, NFS4_FHSIZE);
- status = fh_compose(&tempfh, exp, dentry, NULL);
+ tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+ status = nfserr_jukebox;
+ if (!tempfh)
+ goto out;
+ fh_init(tempfh, NFS4_FHSIZE);
+ status = fh_compose(tempfh, exp, dentry, NULL);
if (status)
goto out;
- fhp = &tempfh;
+ fhp = tempfh;
}
if (bmval0 & (FATTR4_WORD0_ACL | FATTR4_WORD0_ACLSUPPORT
| FATTR4_WORD0_SUPPORTED_ATTRS)) {
@@ -2495,8 +2499,8 @@ out:
security_release_secctx(context, contextlen);
#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
kfree(acl);
- if (fhp == &tempfh)
- fh_put(&tempfh);
+ if (tempfh)
+ fh_put(tempfh);
return status;
out_nfserr:
status = nfserrno(err);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nfsd4: decrease nfsd4_encode_fattr stack usage
2014-01-23 18:53 ` [PATCH] nfsd4: decrease nfsd4_encode_fattr " J. Bruce Fields
@ 2014-01-23 23:07 ` Jeff Layton
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Layton @ 2014-01-23 23:07 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, sandeen
On Thu, 23 Jan 2014 13:53:05 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> A struct svc_fh is 320 bytes on x86_64, it'd be better not to have these
> on the stack.
>
> kmalloc'ing them probably isn't ideal either, but this is the simplest
> thing to do. If it turns out to be a problem in the readdir case then
> we could add a svc_fh to nfsd4_readdir and pass that in.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/nfsd/nfs4xdr.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> On Thu, Jan 09, 2014 at 07:19:34PM -0500, Jeff Layton wrote:
> > On Thu, 9 Jan 2014 16:33:35 -0500
> > > nfsd: fix encode_entryplus_baggage stack usage
> ...
> > I think this is probably the best solution...
> >
> > Acked-by: Jeff Layton <jlayton@redhat.com>
>
> Thanks. While we're at it, here's the v4 case.
>
> --b.
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 8198ecf..63f2395 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2058,7 +2058,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
> u32 bmval1 = bmval[1];
> u32 bmval2 = bmval[2];
> struct kstat stat;
> - struct svc_fh tempfh;
> + struct svc_fh *tempfh = NULL;
> struct kstatfs statfs;
> int buflen = count << 2;
> __be32 *attrlenp;
> @@ -2105,11 +2105,15 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
> goto out_nfserr;
> }
> if ((bmval0 & (FATTR4_WORD0_FILEHANDLE | FATTR4_WORD0_FSID)) && !fhp) {
> - fh_init(&tempfh, NFS4_FHSIZE);
> - status = fh_compose(&tempfh, exp, dentry, NULL);
> + tempfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
> + status = nfserr_jukebox;
> + if (!tempfh)
> + goto out;
> + fh_init(tempfh, NFS4_FHSIZE);
> + status = fh_compose(tempfh, exp, dentry, NULL);
> if (status)
> goto out;
> - fhp = &tempfh;
> + fhp = tempfh;
> }
> if (bmval0 & (FATTR4_WORD0_ACL | FATTR4_WORD0_ACLSUPPORT
> | FATTR4_WORD0_SUPPORTED_ATTRS)) {
> @@ -2495,8 +2499,8 @@ out:
> security_release_secctx(context, contextlen);
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> - if (fhp == &tempfh)
> - fh_put(&tempfh);
> + if (tempfh)
> + fh_put(tempfh);
> return status;
> out_nfserr:
> status = nfserrno(err);
Ok, different approach here. You're kmalloc'ing it separately instead
of making it part of the nfsd4_readdir struct. Should be fine though...
Acked-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-23 23:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-09 21:33 fix encode_entryplus_baggage stack usage J. Bruce Fields
2014-01-10 0:19 ` Jeff Layton
2014-01-23 18:53 ` [PATCH] nfsd4: decrease nfsd4_encode_fattr " J. Bruce Fields
2014-01-23 23:07 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox