* [PATCH] knfsd: Correct reserved reply space for read requests.
@ 2006-03-30 5:53 ` NeilBrown
0 siblings, 0 replies; 8+ 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] 8+ messages in thread* Re: [PATCH] knfsd: Correct reserved reply space for read requests.
2006-03-30 5:53 ` NeilBrown
(?)
@ 2006-03-30 7:17 ` Trond Myklebust
-1 siblings, 0 replies; 8+ 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] 8+ messages in thread* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests.
2006-03-30 5:53 ` NeilBrown
(?)
(?)
@ 2006-03-30 17:58 ` Marc Eshel
2006-04-03 1:17 ` Neil Brown
-1 siblings, 1 reply; 8+ 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] 8+ 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 ` [NFS] " Marc Eshel
0 siblings, 1 reply; 8+ 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] 8+ messages in thread
* Re: [PATCH] knfsd: Correct reserved reply space for read requests.
2006-04-03 1:17 ` Neil Brown
@ 2006-04-03 3:28 ` Marc Eshel
0 siblings, 0 replies; 8+ 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
-------------------------------------------------------
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] 8+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests.
@ 2006-04-03 3:28 ` Marc Eshel
0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [NFS] [PATCH] knfsd: Correct reserved reply space for read requests.
2006-04-03 3:28 ` [NFS] " Marc Eshel
(?)
@ 2006-04-03 3:45 ` Neil Brown
-1 siblings, 0 replies; 8+ 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] 8+ messages in thread