* [PATCH] knfsd: Correct reserved reply space for read requests. [not found] <20060330165307.25307.patches@notabene> @ 2006-03-30 5:53 ` NeilBrown 2006-03-30 7:17 ` Trond Myklebust 2006-03-30 17:58 ` [NFS] " Marc Eshel 0 siblings, 2 replies; 6+ messages in thread From: NeilBrown @ 2006-03-30 5:53 UTC (permalink / raw) To: Andrew Morton; +Cc: nfs, linux-kernel Single patch for nfsd in 2.6.16. As the comment say, it is sensible for this to sit in -mm for a while just in case. ### Comments for Changeset NFSd makes sure there is enough space to hold the maximum possible reply before accepting a request. The units for this maximum is (4byte) words. However in three places, particularly for read request, the number given is a number of bytes. This means too much space is reserved which is slightly wasteful. This is the sort of patch that could uncover a deeper bug, and it is not critical, so it would be best for it to spend a while in -mm before going in to mainline. Discovered-by: "Eivind Sarto" <ivan@kasenna.com> Cc: "Eivind Sarto" <ivan@kasenna.com> Signed-off-by: Neil Brown <neilb@suse.de> ### Diffstat output ./fs/nfsd/nfs3proc.c | 2 +- ./fs/nfsd/nfs4proc.c | 2 +- ./fs/nfsd/nfsproc.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff ./fs/nfsd/nfs3proc.c~current~ ./fs/nfsd/nfs3proc.c --- ./fs/nfsd/nfs3proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 +++ ./fs/nfsd/nfs3proc.c 2006-03-30 16:48:58.000000000 +1100 @@ -682,7 +682,7 @@ static struct svc_procedure nfsd_proced PROC(lookup, dirop, dirop, fhandle2, RC_NOCACHE, ST+FH+pAT+pAT), PROC(access, access, access, fhandle, RC_NOCACHE, ST+pAT+1), PROC(readlink, readlink, readlink, fhandle, RC_NOCACHE, ST+pAT+1+NFS3_MAXPATHLEN/4), - PROC(read, read, read, fhandle, RC_NOCACHE, ST+pAT+4+NFSSVC_MAXBLKSIZE), + PROC(read, read, read, fhandle, RC_NOCACHE, ST+pAT+4+NFSSVC_MAXBLKSIZE/4), PROC(write, write, write, fhandle, RC_REPLBUFF, ST+WC+4), PROC(create, create, create, fhandle2, RC_REPLBUFF, ST+(1+FH+pAT)+WC), PROC(mkdir, mkdir, create, fhandle2, RC_REPLBUFF, ST+(1+FH+pAT)+WC), diff ./fs/nfsd/nfs4proc.c~current~ ./fs/nfsd/nfs4proc.c --- ./fs/nfsd/nfs4proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 +++ ./fs/nfsd/nfs4proc.c 2006-03-30 16:48:58.000000000 +1100 @@ -973,7 +973,7 @@ struct nfsd4_voidargs { int dummy; }; */ static struct svc_procedure nfsd_procedures4[2] = { PROC(null, void, void, void, RC_NOCACHE, 1), - PROC(compound, compound, compound, compound, RC_NOCACHE, NFSD_BUFSIZE) + PROC(compound, compound, compound, compound, RC_NOCACHE, NFSD_BUFSIZE/4) }; struct svc_version nfsd_version4 = { diff ./fs/nfsd/nfsproc.c~current~ ./fs/nfsd/nfsproc.c --- ./fs/nfsd/nfsproc.c~current~ 2006-03-30 16:48:30.000000000 +1100 +++ ./fs/nfsd/nfsproc.c 2006-03-30 16:48:58.000000000 +1100 @@ -553,7 +553,7 @@ static struct svc_procedure nfsd_proced PROC(none, void, void, none, RC_NOCACHE, ST), PROC(lookup, diropargs, diropres, fhandle, RC_NOCACHE, ST+FH+AT), PROC(readlink, readlinkargs, readlinkres, none, RC_NOCACHE, ST+1+NFS_MAXPATHLEN/4), - PROC(read, readargs, readres, fhandle, RC_NOCACHE, ST+AT+1+NFSSVC_MAXBLKSIZE), + PROC(read, readargs, readres, fhandle, RC_NOCACHE, ST+AT+1+NFSSVC_MAXBLKSIZE/4), PROC(none, void, void, none, RC_NOCACHE, ST), PROC(write, writeargs, attrstat, fhandle, RC_REPLBUFF, ST+AT), PROC(create, createargs, diropres, fhandle, RC_REPLBUFF, ST+FH+AT), ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] knfsd: Correct reserved reply space for read requests. 2006-03-30 5:53 ` [PATCH] knfsd: Correct reserved reply space for read requests NeilBrown @ 2006-03-30 7:17 ` Trond Myklebust 2006-03-30 17:58 ` [NFS] " Marc Eshel 1 sibling, 0 replies; 6+ messages in thread From: Trond Myklebust @ 2006-03-30 7:17 UTC (permalink / raw) To: NeilBrown; +Cc: Andrew Morton, nfs, linux-kernel On Thu, 2006-03-30 at 16:53 +1100, NeilBrown wrote: > Single patch for nfsd in 2.6.16. As the comment say, it is sensible > for this to sit in -mm for a while just in case. > > ### Comments for Changeset > > NFSd makes sure there is enough space to hold the maximum possible > reply before accepting a request. The units for this maximum is > (4byte) words. However in three places, particularly for read > request, the number given is a number of bytes. > > This means too much space is reserved which is slightly wasteful. > > This is the sort of patch that could uncover a deeper bug, and it is > not critical, so it would be best for it to spend a while in -mm before going in to mainline. > > Discovered-by: "Eivind Sarto" <ivan@kasenna.com> > > Cc: "Eivind Sarto" <ivan@kasenna.com> > > Signed-off-by: Neil Brown <neilb@suse.de> > > ### Diffstat output > ./fs/nfsd/nfs3proc.c | 2 +- > ./fs/nfsd/nfs4proc.c | 2 +- > ./fs/nfsd/nfsproc.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff ./fs/nfsd/nfs3proc.c~current~ ./fs/nfsd/nfs3proc.c > --- ./fs/nfsd/nfs3proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfs3proc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -682,7 +682,7 @@ static struct svc_procedure nfsd_proced > PROC(lookup, dirop, dirop, fhandle2, RC_NOCACHE, ST+FH+pAT+pAT), > PROC(access, access, access, fhandle, RC_NOCACHE, ST+pAT+1), > PROC(readlink, readlink, readlink, fhandle, RC_NOCACHE, ST+pAT+1+NFS3_MAXPATHLEN/4), > - PROC(read, read, read, fhandle, RC_NOCACHE, ST+pAT+4+NFSSVC_MAXBLKSIZE), > + PROC(read, read, read, fhandle, RC_NOCACHE, ST+pAT+4+NFSSVC_MAXBLKSIZE/4), Hmm... Wouldn't it be safer to use XDR_QUADLEN()? I doubt that we will ever set a NFSSVC_MAXBLKSIZE that is not divisible by 4, but... > PROC(write, write, write, fhandle, RC_REPLBUFF, ST+WC+4), > PROC(create, create, create, fhandle2, RC_REPLBUFF, ST+(1+FH+pAT)+WC), > PROC(mkdir, mkdir, create, fhandle2, RC_REPLBUFF, ST+(1+FH+pAT)+WC), > > diff ./fs/nfsd/nfs4proc.c~current~ ./fs/nfsd/nfs4proc.c > --- ./fs/nfsd/nfs4proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfs4proc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -973,7 +973,7 @@ struct nfsd4_voidargs { int dummy; }; > */ > static struct svc_procedure nfsd_procedures4[2] = { > PROC(null, void, void, void, RC_NOCACHE, 1), > - PROC(compound, compound, compound, compound, RC_NOCACHE, NFSD_BUFSIZE) > + PROC(compound, compound, compound, compound, RC_NOCACHE, NFSD_BUFSIZE/4) Ditto... > }; > > struct svc_version nfsd_version4 = { > > diff ./fs/nfsd/nfsproc.c~current~ ./fs/nfsd/nfsproc.c > --- ./fs/nfsd/nfsproc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfsproc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -553,7 +553,7 @@ static struct svc_procedure nfsd_proced > PROC(none, void, void, none, RC_NOCACHE, ST), > PROC(lookup, diropargs, diropres, fhandle, RC_NOCACHE, ST+FH+AT), > PROC(readlink, readlinkargs, readlinkres, none, RC_NOCACHE, ST+1+NFS_MAXPATHLEN/4), > - PROC(read, readargs, readres, fhandle, RC_NOCACHE, ST+AT+1+NFSSVC_MAXBLKSIZE), > + PROC(read, readargs, readres, fhandle, RC_NOCACHE, ST+AT+1+NFSSVC_MAXBLKSIZE/4), > PROC(none, void, void, none, RC_NOCACHE, ST), > PROC(write, writeargs, attrstat, fhandle, RC_REPLBUFF, ST+AT), > PROC(create, createargs, diropres, fhandle, RC_REPLBUFF, ST+FH+AT), Cheers, Trond ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests. 2006-03-30 5:53 ` [PATCH] knfsd: Correct reserved reply space for read requests NeilBrown 2006-03-30 7:17 ` Trond Myklebust @ 2006-03-30 17:58 ` Marc Eshel 2006-04-03 1:17 ` Neil Brown 1 sibling, 1 reply; 6+ messages in thread From: Marc Eshel @ 2006-03-30 17:58 UTC (permalink / raw) To: NeilBrown; +Cc: Andrew Morton, linux-kernel, nfs, nfs-admin Hi Neil Can we use this opportunity to change NFSSVC_MAXBLKSIZE from 32K to 64K to match RPCSVC_MAXPAYLOAD. It makes real difference in I/O performance (we will still be saving half the space we used to allocate :). Thanks, Marc. NeilBrown wrote on 03/29/2006 09:53:50 PM: > Single patch for nfsd in 2.6.16. As the comment say, it is sensible > for this to sit in -mm for a while just in case. > > ### Comments for Changeset > > NFSd makes sure there is enough space to hold the maximum possible > reply before accepting a request. The units for this maximum is > (4byte) words. However in three places, particularly for read > request, the number given is a number of bytes. > > This means too much space is reserved which is slightly wasteful. > > This is the sort of patch that could uncover a deeper bug, and it is > not critical, so it would be best for it to spend a while in -mm > before going in to mainline. > > Discovered-by: "Eivind Sarto" <ivan@kasenna.com> > > Cc: "Eivind Sarto" <ivan@kasenna.com> > > Signed-off-by: Neil Brown <neilb@suse.de> > > ### Diffstat output > ./fs/nfsd/nfs3proc.c | 2 +- > ./fs/nfsd/nfs4proc.c | 2 +- > ./fs/nfsd/nfsproc.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff ./fs/nfsd/nfs3proc.c~current~ ./fs/nfsd/nfs3proc.c > --- ./fs/nfsd/nfs3proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfs3proc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -682,7 +682,7 @@ static struct svc_procedure nfsd_proced > PROC(lookup, dirop, dirop, fhandle2, RC_NOCACHE, > ST+FH+pAT+pAT), > PROC(access, access, access, fhandle, RC_NOCACHE, ST+pAT+1), > PROC(readlink, readlink, readlink, fhandle, RC_NOCACHE, > ST+pAT+1+NFS3_MAXPATHLEN/4), > - PROC(read, read, read, fhandle, RC_NOCACHE, > ST+pAT+4+NFSSVC_MAXBLKSIZE), > + PROC(read, read, read, fhandle, RC_NOCACHE, > ST+pAT+4+NFSSVC_MAXBLKSIZE/4), > PROC(write, write, write, fhandle, RC_REPLBUFF, ST+WC+4), > PROC(create, create, create, fhandle2, RC_REPLBUFF, > ST+(1+FH+pAT)+WC), > PROC(mkdir, mkdir, create, fhandle2, RC_REPLBUFF, > ST+(1+FH+pAT)+WC), > > diff ./fs/nfsd/nfs4proc.c~current~ ./fs/nfsd/nfs4proc.c > --- ./fs/nfsd/nfs4proc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfs4proc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -973,7 +973,7 @@ struct nfsd4_voidargs { int dummy; }; > */ > static struct svc_procedure nfsd_procedures4[2] = { > PROC(null, void, void, void, RC_NOCACHE, 1), > - PROC(compound, compound, compound, compound, RC_NOCACHE, NFSD_BUFSIZE) > + PROC(compound, compound, compound, compound, RC_NOCACHE, > NFSD_BUFSIZE/4) > }; > > struct svc_version nfsd_version4 = { > > diff ./fs/nfsd/nfsproc.c~current~ ./fs/nfsd/nfsproc.c > --- ./fs/nfsd/nfsproc.c~current~ 2006-03-30 16:48:30.000000000 +1100 > +++ ./fs/nfsd/nfsproc.c 2006-03-30 16:48:58.000000000 +1100 > @@ -553,7 +553,7 @@ static struct svc_procedure nfsd_proced > PROC(none, void, void, none, RC_NOCACHE, ST), > PROC(lookup, diropargs, diropres, fhandle, RC_NOCACHE, ST+FH+AT), > PROC(readlink, readlinkargs, readlinkres, none, > RC_NOCACHE, ST+1+NFS_MAXPATHLEN/4), > - PROC(read, readargs, readres, fhandle, RC_NOCACHE, > ST+AT+1+NFSSVC_MAXBLKSIZE), > + PROC(read, readargs, readres, fhandle, RC_NOCACHE, > ST+AT+1+NFSSVC_MAXBLKSIZE/4), > PROC(none, void, void, none, RC_NOCACHE, ST), > PROC(write, writeargs, attrstat, fhandle, RC_REPLBUFF, ST+AT), > PROC(create, createargs, diropres, fhandle, RC_REPLBUFF,ST+FH+AT), > > > ------------------------------------------------------- > This SF.Net email is sponsored by xPML, a groundbreaking scripting language > that extends applications into web and mobile media. Attend the live webcast > and join the prime developer group breaking into this new coding territory! > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642 > _______________________________________________ > NFS maillist - NFS@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests. 2006-03-30 17:58 ` [NFS] " Marc Eshel @ 2006-04-03 1:17 ` Neil Brown 2006-04-03 3:28 ` Marc Eshel 0 siblings, 1 reply; 6+ messages in thread From: Neil Brown @ 2006-04-03 1:17 UTC (permalink / raw) To: Marc Eshel; +Cc: Andrew Morton, linux-kernel, nfs On Thursday March 30, eshel@almaden.ibm.com wrote: > Hi Neil > Can we use this opportunity to change NFSSVC_MAXBLKSIZE from 32K to 64K to > match RPCSVC_MAXPAYLOAD. It makes real difference in I/O performance (we > will still be saving half the space we used to allocate :). > Thanks, Marc. Maybe... but why not 128K ?? There is certainly room to increase NFSSVC_MAXBLKSIZE, but I feel that it needs to be done together with a more detailed look at consequences in the network layer, particularly TCP window sizes. I wouldn't mind making a CONFIG_ tunable without that detailed look, but making it a fixed change I feel less comfortable about. NeilBrown ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests. 2006-04-03 1:17 ` Neil Brown @ 2006-04-03 3:28 ` Marc Eshel 2006-04-03 3:45 ` Neil Brown 0 siblings, 1 reply; 6+ messages in thread From: Marc Eshel @ 2006-04-03 3:28 UTC (permalink / raw) To: Neil Brown; +Cc: Andrew Morton, linux-kernel, nfs, nfs-admin nfs-admin@lists.sourceforge.net wrote on 04/02/2006 06:17:17 PM: > On Thursday March 30, eshel@almaden.ibm.com wrote: > > Hi Neil > > Can we use this opportunity to change NFSSVC_MAXBLKSIZE from 32K to 64K to > > match RPCSVC_MAXPAYLOAD. It makes real difference in I/O performance (we > > will still be saving half the space we used to allocate :). > > Thanks, Marc. > > Maybe... but why not 128K ?? Yes, It would be nice to be able to match the Linux client side that can go to 1MB. > There is certainly room to increase NFSSVC_MAXBLKSIZE, but I feel that > it needs to be done together with a more detailed look at consequences > in the network layer, particularly TCP window sizes. I wouldn't mind > making a CONFIG_ tunable without that detailed look, but making it a > fixed change I feel less comfortable about. Like you said it will need match more work to do it right and also not waste space for all RPC request to which the maximum possible size is allocated up front. But until than way not take advantage of this minor change that can give us significant performance improvement. I run with NFSSVC_MAXBLKSIZE set to 64K (the only change I made) and saw 10%-15% improvement for NFS reads. Is there anyone out there that is looking at making this improvement ? Marc. > > NeilBrown > > > ------------------------------------------------------- > This SF.Net email is sponsored by xPML, a groundbreaking scripting language > that extends applications into web and mobile media. Attend the live webcast > and join the prime developer group breaking into this new coding territory! > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642 > _______________________________________________ > NFS maillist - NFS@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/nfs ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests. 2006-04-03 3:28 ` Marc Eshel @ 2006-04-03 3:45 ` Neil Brown 0 siblings, 0 replies; 6+ messages in thread From: Neil Brown @ 2006-04-03 3:45 UTC (permalink / raw) To: Marc Eshel; +Cc: Andrew Morton, linux-kernel, nfs On Sunday April 2, eshel@almaden.ibm.com wrote: > nfs-admin@lists.sourceforge.net wrote on 04/02/2006 06:17:17 PM: > > > On Thursday March 30, eshel@almaden.ibm.com wrote: > > > Hi Neil > > > Can we use this opportunity to change NFSSVC_MAXBLKSIZE from 32K to > 64K to > > > match RPCSVC_MAXPAYLOAD. It makes real difference in I/O performance > (we > > > will still be saving half the space we used to allocate :). > > > Thanks, Marc. > > > > Maybe... but why not 128K ?? > > Yes, It would be nice to be able to match the Linux client side that can > go to 1MB. > > > There is certainly room to increase NFSSVC_MAXBLKSIZE, but I feel that > > it needs to be done together with a more detailed look at consequences > > in the network layer, particularly TCP window sizes. I wouldn't mind > > making a CONFIG_ tunable without that detailed look, but making it a > > fixed change I feel less comfortable about. > > Like you said it will need match more work to do it right and also not > waste space for all RPC request to which the maximum possible size is > allocated up front. But until than way not take advantage of this minor > change that can give us significant performance improvement. I run with > NFSSVC_MAXBLKSIZE set to 64K (the only change I made) and saw 10%-15% > improvement for NFS reads. Is there anyone out there that is looking at > making this improvement ? It was a fairly soft reservation of space, and we never actually tried to use it all. I don't see a clear problem with just making the change you mention, but until I have thought through the implications (or until someone else has and convinces me) I don't feel comfortable making the change. It's on my todo list, but that doesn't promise a lot :-( If someone else wants to try to convince me it is safe, and mention tcp window sizes a couple of time, I'll probably be happy with it. NeilBrown ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-04-03 3:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20060330165307.25307.patches@notabene>
2006-03-30 5:53 ` [PATCH] knfsd: Correct reserved reply space for read requests NeilBrown
2006-03-30 7:17 ` Trond Myklebust
2006-03-30 17:58 ` [NFS] " Marc Eshel
2006-04-03 1:17 ` Neil Brown
2006-04-03 3:28 ` Marc Eshel
2006-04-03 3:45 ` Neil Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox