* linux-next: nfs tree build warning
@ 2009-05-01 3:15 Stephen Rothwell
2009-05-01 3:22 ` Trond Myklebust
2009-06-09 9:13 ` linux-next: nfs tree build warning Stephen Rothwell
0 siblings, 2 replies; 22+ messages in thread
From: Stephen Rothwell @ 2009-05-01 3:15 UTC (permalink / raw)
To: Trond Myklebust; +Cc: linux-next, Benny Halevy, Andy Adamson
[-- Attachment #1: Type: text/plain, Size: 481 bytes --]
Hi Trond,
Today's linux-next build (x86_64 allmodconfig) produced this warning:
fs/nfs/nfs4proc.c: In function 'nfs4_proc_exchange_id':
fs/nfs/nfs4proc.c:4279: warning: the frame size of 2288 bytes is larger than 2048 bytes
Introduced by commit 63a93b4af49220c74757beb17b5617b72d912b6b ("nfs41:
exchange_id operation"). This commit has been around for a while.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: linux-next: nfs tree build warning 2009-05-01 3:15 linux-next: nfs tree build warning Stephen Rothwell @ 2009-05-01 3:22 ` Trond Myklebust 2009-05-01 12:19 ` Benny Halevy 2009-06-09 9:13 ` linux-next: nfs tree build warning Stephen Rothwell 1 sibling, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-01 3:22 UTC (permalink / raw) To: Stephen Rothwell; +Cc: linux-next, Benny Halevy, Andy Adamson On Fri, 2009-05-01 at 13:15 +1000, Stephen Rothwell wrote: > Hi Trond, > > Today's linux-next build (x86_64 allmodconfig) produced this warning: > > fs/nfs/nfs4proc.c: In function 'nfs4_proc_exchange_id': > fs/nfs/nfs4proc.c:4279: warning: the frame size of 2288 bytes is larger than 2048 bytes > > Introduced by commit 63a93b4af49220c74757beb17b5617b72d912b6b ("nfs41: > exchange_id operation"). This commit has been around for a while. Benny, Andy, Why are we preallocating 1k buffers on the stack for these things? That's an insane amount of free space... If this is truly a realistic value (which I sincerely doubt), then the right thing to do is to preallocate a page in which to store them. Putting 1k arrays on the stack is just _wrong_. Trond ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: nfs tree build warning 2009-05-01 3:22 ` Trond Myklebust @ 2009-05-01 12:19 ` Benny Halevy 2009-05-01 14:56 ` William A. (Andy) Adamson 0 siblings, 1 reply; 22+ messages in thread From: Benny Halevy @ 2009-05-01 12:19 UTC (permalink / raw) To: Trond Myklebust, Andy Adamson; +Cc: Stephen Rothwell, linux-next On May. 01, 2009, 6:22 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Fri, 2009-05-01 at 13:15 +1000, Stephen Rothwell wrote: >> Hi Trond, >> >> Today's linux-next build (x86_64 allmodconfig) produced this warning: >> >> fs/nfs/nfs4proc.c: In function 'nfs4_proc_exchange_id': >> fs/nfs/nfs4proc.c:4279: warning: the frame size of 2288 bytes is larger than 2048 bytes >> >> Introduced by commit 63a93b4af49220c74757beb17b5617b72d912b6b ("nfs41: >> exchange_id operation"). This commit has been around for a while. > > Benny, Andy, > > Why are we preallocating 1k buffers on the stack for these things? > That's an insane amount of free space... > > If this is truly a realistic value (which I sincerely doubt), then the > right thing to do is to preallocate a page in which to store them. > Putting 1k arrays on the stack is just _wrong_. > > Trond > Ouch, struct nfs41_exchange_id_res contains struct server_owner server_owner; struct server_scope server_scope; each embedding a char [NFS4_OPAQUE_LIMIT] array which is 1K in length. Not only we should have preallocated these arrays dynamically, we actually throw them away. Therefore I suggest that until they are put to use we should just skip their xdr decoding, like we do for the implementation ID. Benny ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: nfs tree build warning 2009-05-01 12:19 ` Benny Halevy @ 2009-05-01 14:56 ` William A. (Andy) Adamson 2009-05-01 20:14 ` [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy 0 siblings, 1 reply; 22+ messages in thread From: William A. (Andy) Adamson @ 2009-05-01 14:56 UTC (permalink / raw) To: Benny Halevy; +Cc: Trond Myklebust, Andy Adamson, Stephen Rothwell, linux-next On Fri, May 1, 2009 at 8:19 AM, Benny Halevy <bhalevy@panasas.com> wrote: > On May. 01, 2009, 6:22 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: >> On Fri, 2009-05-01 at 13:15 +1000, Stephen Rothwell wrote: >>> Hi Trond, >>> >>> Today's linux-next build (x86_64 allmodconfig) produced this warning: >>> >>> fs/nfs/nfs4proc.c: In function 'nfs4_proc_exchange_id': >>> fs/nfs/nfs4proc.c:4279: warning: the frame size of 2288 bytes is larger than 2048 bytes >>> >>> Introduced by commit 63a93b4af49220c74757beb17b5617b72d912b6b ("nfs41: >>> exchange_id operation"). This commit has been around for a while. >> >> Benny, Andy, >> >> Why are we preallocating 1k buffers on the stack for these things? >> That's an insane amount of free space... >> >> If this is truly a realistic value (which I sincerely doubt), then the >> right thing to do is to preallocate a page in which to store them. >> Putting 1k arrays on the stack is just _wrong_. >> >> Trond >> > > Ouch, struct nfs41_exchange_id_res contains > struct server_owner server_owner; > struct server_scope server_scope; > each embedding a char [NFS4_OPAQUE_LIMIT] array > which is 1K in length. > Not only we should have preallocated these arrays dynamically, > we actually throw them away. > Therefore I suggest that until they are put to use > we should just skip their xdr decoding, like we do > for the implementation ID. Yes, I agree. That is just wrong.... -->Andy > > Benny > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-01 14:56 ` William A. (Andy) Adamson @ 2009-05-01 20:14 ` Benny Halevy 2009-05-05 19:34 ` Trond Myklebust 0 siblings, 1 reply; 22+ messages in thread From: Benny Halevy @ 2009-05-01 20:14 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next, Benny Halevy struct nfs41_exchange_id_res is currently allocated on the stack insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( byte arrays embedded in server_owner and server_scope. Since these are not in use yet, this patch gets rid of them for the time being. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- include/linux/nfs_xdr.h | 3 --- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index 80af0ae..3350d19 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) static int decode_exchange_id(struct xdr_stream *xdr, struct nfs41_exchange_id_res *res) { - uint32_t *p; - int status, dummy; + uint32_t *p, dummy; + int status; struct nfs_client *clp = res->client; status = decode_op_hdr(xdr, OP_EXCHANGE_ID); @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, if (dummy != SP4_NONE) return -EIO; - /* minor_id */ + /* Throw away minor_id */ READ_BUF(8); - READ64(res->server_owner.minor_id); + p += 8; - /* Major id */ + /* Throw away Major id */ READ_BUF(4); - READ32(res->server_owner.major_id_sz); - READ_BUF(res->server_owner.major_id_sz); - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); + READ32(dummy); + READ_BUF(dummy); + p += XDR_QUADLEN(dummy); - /* server_scope */ + /* Throw away server_scope */ READ_BUF(4); - READ32(res->server_scope.server_scope_sz); - READ_BUF(res->server_scope.server_scope_sz); - COPYMEM(res->server_scope.server_scope, - res->server_scope.server_scope_sz); + READ32(dummy); + READ_BUF(dummy); + p += XDR_QUADLEN(dummy); + /* Throw away Implementation id array */ READ_BUF(4); READ32(dummy); + READ_BUF(dummy); p += XDR_QUADLEN(dummy); return 0; diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 071a6d1..62f63fb 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -925,9 +925,6 @@ struct server_scope { struct nfs41_exchange_id_res { struct nfs_client *client; u32 flags; - struct server_owner server_owner; - struct server_scope server_scope; - struct nfs_impl_id4 impl_id; }; struct nfs41_create_session_args { -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-01 20:14 ` [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy @ 2009-05-05 19:34 ` Trond Myklebust 2009-05-05 19:39 ` Trond Myklebust 0 siblings, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-05 19:34 UTC (permalink / raw) To: Benny Halevy; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: > struct nfs41_exchange_id_res is currently allocated on the stack > insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( > byte arrays embedded in server_owner and server_scope. > Since these are not in use yet, this patch gets rid of them for the > time being. > > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > --- > fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- > include/linux/nfs_xdr.h | 3 --- > 2 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index 80af0ae..3350d19 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) > static int decode_exchange_id(struct xdr_stream *xdr, > struct nfs41_exchange_id_res *res) > { > - uint32_t *p; > - int status, dummy; > + uint32_t *p, dummy; > + int status; > struct nfs_client *clp = res->client; > > status = decode_op_hdr(xdr, OP_EXCHANGE_ID); > @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, > if (dummy != SP4_NONE) > return -EIO; > > - /* minor_id */ > + /* Throw away minor_id */ > READ_BUF(8); > - READ64(res->server_owner.minor_id); > + p += 8; ^^^^^^^^ Err... This isn't the same thing at all! You're suddenly skipping 10=words instead of the original 2. READ_BUF() will already take care of updating the 'p' pointer. > > - /* Major id */ > + /* Throw away Major id */ > READ_BUF(4); > - READ32(res->server_owner.major_id_sz); > - READ_BUF(res->server_owner.major_id_sz); > - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); > + READ32(dummy); > + READ_BUF(dummy); > + p += XDR_QUADLEN(dummy); ^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. You're skipping 2*dummy words. > > - /* server_scope */ > + /* Throw away server_scope */ > READ_BUF(4); > - READ32(res->server_scope.server_scope_sz); > - READ_BUF(res->server_scope.server_scope_sz); > - COPYMEM(res->server_scope.server_scope, > - res->server_scope.server_scope_sz); > + READ32(dummy); > + READ_BUF(dummy); > + p += XDR_QUADLEN(dummy); ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > + > /* Throw away Implementation id array */ > READ_BUF(4); > READ32(dummy); > + READ_BUF(dummy); > p += XDR_QUADLEN(dummy); ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > return 0; > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index 071a6d1..62f63fb 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -925,9 +925,6 @@ struct server_scope { > struct nfs41_exchange_id_res { > struct nfs_client *client; > u32 flags; > - struct server_owner server_owner; > - struct server_scope server_scope; > - struct nfs_impl_id4 impl_id; > }; > > struct nfs41_create_session_args { ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 19:34 ` Trond Myklebust @ 2009-05-05 19:39 ` Trond Myklebust 2009-05-05 19:41 ` Trond Myklebust 0 siblings, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-05 19:39 UTC (permalink / raw) To: Benny Halevy; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: > On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: > > struct nfs41_exchange_id_res is currently allocated on the stack > > insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( > > byte arrays embedded in server_owner and server_scope. > > Since these are not in use yet, this patch gets rid of them for the > > time being. > > > > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > > --- > > fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- > > include/linux/nfs_xdr.h | 3 --- > > 2 files changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > index 80af0ae..3350d19 100644 > > --- a/fs/nfs/nfs4xdr.c > > +++ b/fs/nfs/nfs4xdr.c > > @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) > > static int decode_exchange_id(struct xdr_stream *xdr, > > struct nfs41_exchange_id_res *res) > > { > > - uint32_t *p; > > - int status, dummy; > > + uint32_t *p, dummy; > > + int status; > > struct nfs_client *clp = res->client; > > > > status = decode_op_hdr(xdr, OP_EXCHANGE_ID); > > @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, > > if (dummy != SP4_NONE) > > return -EIO; > > > > - /* minor_id */ > > + /* Throw away minor_id */ > > READ_BUF(8); > > - READ64(res->server_owner.minor_id); > > + p += 8; > ^^^^^^^^ Err... This isn't the same thing at all! > > You're suddenly skipping 10=words instead of the original 2. READ_BUF() > will already take care of updating the 'p' pointer. BTW, this is exactly why the whole READ_BUF(), READ*(), WRITE*() macro crap is so utterly broken. The magic that happens to the 'p' pointer is completely opaque to someone unfamiliar with the code. > > > > - /* Major id */ > > + /* Throw away Major id */ > > READ_BUF(4); > > - READ32(res->server_owner.major_id_sz); > > - READ_BUF(res->server_owner.major_id_sz); > > - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); > > + READ32(dummy); > > + READ_BUF(dummy); > > + p += XDR_QUADLEN(dummy); > ^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. You're skipping 2*dummy words. > > > > > - /* server_scope */ > > + /* Throw away server_scope */ > > READ_BUF(4); > > - READ32(res->server_scope.server_scope_sz); > > - READ_BUF(res->server_scope.server_scope_sz); > > - COPYMEM(res->server_scope.server_scope, > > - res->server_scope.server_scope_sz); > > + READ32(dummy); > > + READ_BUF(dummy); > > + p += XDR_QUADLEN(dummy); > ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > > + > > /* Throw away Implementation id array */ > > READ_BUF(4); > > READ32(dummy); > > + READ_BUF(dummy); > > p += XDR_QUADLEN(dummy); > ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > > > > return 0; > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > > index 071a6d1..62f63fb 100644 > > --- a/include/linux/nfs_xdr.h > > +++ b/include/linux/nfs_xdr.h > > @@ -925,9 +925,6 @@ struct server_scope { > > struct nfs41_exchange_id_res { > > struct nfs_client *client; > > u32 flags; > > - struct server_owner server_owner; > > - struct server_scope server_scope; > > - struct nfs_impl_id4 impl_id; > > }; > > > > struct nfs41_create_session_args { > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 19:39 ` Trond Myklebust @ 2009-05-05 19:41 ` Trond Myklebust 2009-05-05 20:28 ` Benny Halevy 0 siblings, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-05 19:41 UTC (permalink / raw) To: Benny Halevy; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: > On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: > > On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: > > > struct nfs41_exchange_id_res is currently allocated on the stack > > > insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( > > > byte arrays embedded in server_owner and server_scope. > > > Since these are not in use yet, this patch gets rid of them for the > > > time being. > > > > > > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > > > --- > > > fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- > > > include/linux/nfs_xdr.h | 3 --- > > > 2 files changed, 14 insertions(+), 16 deletions(-) > > > > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > > > index 80af0ae..3350d19 100644 > > > --- a/fs/nfs/nfs4xdr.c > > > +++ b/fs/nfs/nfs4xdr.c > > > @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) > > > static int decode_exchange_id(struct xdr_stream *xdr, > > > struct nfs41_exchange_id_res *res) > > > { > > > - uint32_t *p; > > > - int status, dummy; > > > + uint32_t *p, dummy; Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' checker will yell at you. > > > + int status; > > > struct nfs_client *clp = res->client; > > > > > > status = decode_op_hdr(xdr, OP_EXCHANGE_ID); > > > @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, > > > if (dummy != SP4_NONE) > > > return -EIO; > > > > > > - /* minor_id */ > > > + /* Throw away minor_id */ > > > READ_BUF(8); > > > - READ64(res->server_owner.minor_id); > > > + p += 8; > > ^^^^^^^^ Err... This isn't the same thing at all! > > > > You're suddenly skipping 10=words instead of the original 2. READ_BUF() > > will already take care of updating the 'p' pointer. > > BTW, this is exactly why the whole READ_BUF(), READ*(), WRITE*() macro > crap is so utterly broken. The magic that happens to the 'p' pointer is > completely opaque to someone unfamiliar with the code. > > > > > > > - /* Major id */ > > > + /* Throw away Major id */ > > > READ_BUF(4); > > > - READ32(res->server_owner.major_id_sz); > > > - READ_BUF(res->server_owner.major_id_sz); > > > - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); > > > + READ32(dummy); > > > + READ_BUF(dummy); > > > + p += XDR_QUADLEN(dummy); > > ^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. You're skipping 2*dummy words. > > > > > > > > - /* server_scope */ > > > + /* Throw away server_scope */ > > > READ_BUF(4); > > > - READ32(res->server_scope.server_scope_sz); > > > - READ_BUF(res->server_scope.server_scope_sz); > > > - COPYMEM(res->server_scope.server_scope, > > > - res->server_scope.server_scope_sz); > > > + READ32(dummy); > > > + READ_BUF(dummy); > > > + p += XDR_QUADLEN(dummy); > > ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > > > > + > > > /* Throw away Implementation id array */ > > > READ_BUF(4); > > > READ32(dummy); > > > + READ_BUF(dummy); > > > p += XDR_QUADLEN(dummy); > > ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > > > > > > > return 0; > > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > > > index 071a6d1..62f63fb 100644 > > > --- a/include/linux/nfs_xdr.h > > > +++ b/include/linux/nfs_xdr.h > > > @@ -925,9 +925,6 @@ struct server_scope { > > > struct nfs41_exchange_id_res { > > > struct nfs_client *client; > > > u32 flags; > > > - struct server_owner server_owner; > > > - struct server_scope server_scope; > > > - struct nfs_impl_id4 impl_id; > > > }; > > > > > > struct nfs41_create_session_args { > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 19:41 ` Trond Myklebust @ 2009-05-05 20:28 ` Benny Halevy 2009-05-05 20:35 ` Benny Halevy 0 siblings, 1 reply; 22+ messages in thread From: Benny Halevy @ 2009-05-05 20:28 UTC (permalink / raw) To: Trond Myklebust; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On May. 05, 2009, 22:41 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: >> On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: >>> On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: >>>> struct nfs41_exchange_id_res is currently allocated on the stack >>>> insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( >>>> byte arrays embedded in server_owner and server_scope. >>>> Since these are not in use yet, this patch gets rid of them for the >>>> time being. >>>> >>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >>>> --- >>>> fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- >>>> include/linux/nfs_xdr.h | 3 --- >>>> 2 files changed, 14 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>> index 80af0ae..3350d19 100644 >>>> --- a/fs/nfs/nfs4xdr.c >>>> +++ b/fs/nfs/nfs4xdr.c >>>> @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) >>>> static int decode_exchange_id(struct xdr_stream *xdr, >>>> struct nfs41_exchange_id_res *res) >>>> { >>>> - uint32_t *p; >>>> - int status, dummy; >>>> + uint32_t *p, dummy; > > Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' > checker will yell at you. Thanks! I'll send a fixed version of this patch and also look into the rest of the xdr code. > >>>> + int status; >>>> struct nfs_client *clp = res->client; >>>> >>>> status = decode_op_hdr(xdr, OP_EXCHANGE_ID); >>>> @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, >>>> if (dummy != SP4_NONE) >>>> return -EIO; >>>> >>>> - /* minor_id */ >>>> + /* Throw away minor_id */ >>>> READ_BUF(8); >>>> - READ64(res->server_owner.minor_id); >>>> + p += 8; >>> ^^^^^^^^ Err... This isn't the same thing at all! Ouch, of course. What did I smoke that day? >>> >>> You're suddenly skipping 10=words instead of the original 2. READ_BUF() >>> will already take care of updating the 'p' pointer. Which p? It takes care of argp->p, not the local 'p' variable, doesn't it. p += 2 has an equivalent side effect on 'p' as doing READ64. I can do "p = argp;" instead though to reset 'p' onto the current xdr stream "head". >> BTW, this is exactly why the whole READ_BUF(), READ*(), WRITE*() macro >> crap is so utterly broken. The magic that happens to the 'p' pointer is >> completely opaque to someone unfamiliar with the code. I completely agree. We're dealing with bits and bytes (or 32 bit words actually) at the wrong abstraction layer. >> >>>> >>>> - /* Major id */ >>>> + /* Throw away Major id */ >>>> READ_BUF(4); >>>> - READ32(res->server_owner.major_id_sz); >>>> - READ_BUF(res->server_owner.major_id_sz); >>>> - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); >>>> + READ32(dummy); >>>> + READ_BUF(dummy); >>>> + p += XDR_QUADLEN(dummy); >>> ^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. You're skipping 2*dummy words. Why? READ_BUF increments argp->p in XDR_QUADLEN(dummy) words and the local p should be adjusted correspondingly. This used to happen as the side effect of COPY_MEM. >>> >>>> >>>> - /* server_scope */ >>>> + /* Throw away server_scope */ >>>> READ_BUF(4); >>>> - READ32(res->server_scope.server_scope_sz); >>>> - READ_BUF(res->server_scope.server_scope_sz); >>>> - COPYMEM(res->server_scope.server_scope, >>>> - res->server_scope.server_scope_sz); >>>> + READ32(dummy); >>>> + READ_BUF(dummy); >>>> + p += XDR_QUADLEN(dummy); >>> ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto Ditto >>> >>>> + >>>> /* Throw away Implementation id array */ >>>> READ_BUF(4); >>>> READ32(dummy); >>>> + READ_BUF(dummy); >>>> p += XDR_QUADLEN(dummy); >>> ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto >>> and here too... BTW, Calling READ_BUF here was missing before, so this could be put in a patch of its own, and adjusting p is not really necessary since we're about to exit the function... In any case, are we going to squash these fixes into the respective queued patch, or would like to start accumulating the patches for 2.6.31 without rebasing? Benny >>>> >>>> return 0; >>>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h >>>> index 071a6d1..62f63fb 100644 >>>> --- a/include/linux/nfs_xdr.h >>>> +++ b/include/linux/nfs_xdr.h >>>> @@ -925,9 +925,6 @@ struct server_scope { >>>> struct nfs41_exchange_id_res { >>>> struct nfs_client *client; >>>> u32 flags; >>>> - struct server_owner server_owner; >>>> - struct server_scope server_scope; >>>> - struct nfs_impl_id4 impl_id; >>>> }; >>>> >>>> struct nfs41_create_session_args { > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 20:28 ` Benny Halevy @ 2009-05-05 20:35 ` Benny Halevy 2009-05-05 22:12 ` Trond Myklebust 0 siblings, 1 reply; 22+ messages in thread From: Benny Halevy @ 2009-05-05 20:35 UTC (permalink / raw) To: Trond Myklebust; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On 2009-05-05 23:28, Benny Halevy wrote: > On May. 05, 2009, 22:41 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: >> On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: >>> On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: >>>> On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: >>>>> struct nfs41_exchange_id_res is currently allocated on the stack >>>>> insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( >>>>> byte arrays embedded in server_owner and server_scope. >>>>> Since these are not in use yet, this patch gets rid of them for the >>>>> time being. >>>>> >>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >>>>> --- >>>>> fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- >>>>> include/linux/nfs_xdr.h | 3 --- >>>>> 2 files changed, 14 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>>> index 80af0ae..3350d19 100644 >>>>> --- a/fs/nfs/nfs4xdr.c >>>>> +++ b/fs/nfs/nfs4xdr.c >>>>> @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) >>>>> static int decode_exchange_id(struct xdr_stream *xdr, >>>>> struct nfs41_exchange_id_res *res) >>>>> { >>>>> - uint32_t *p; >>>>> - int status, dummy; >>>>> + uint32_t *p, dummy; >> Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' >> checker will yell at you. > > Thanks! I'll send a fixed version > of this patch and also look into the rest of the xdr code. > >>>>> + int status; >>>>> struct nfs_client *clp = res->client; >>>>> >>>>> status = decode_op_hdr(xdr, OP_EXCHANGE_ID); >>>>> @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, >>>>> if (dummy != SP4_NONE) >>>>> return -EIO; >>>>> >>>>> - /* minor_id */ >>>>> + /* Throw away minor_id */ >>>>> READ_BUF(8); >>>>> - READ64(res->server_owner.minor_id); >>>>> + p += 8; >>>> ^^^^^^^^ Err... This isn't the same thing at all! > > Ouch, of course. What did I smoke that day? > >>>> You're suddenly skipping 10=words instead of the original 2. READ_BUF() >>>> will already take care of updating the 'p' pointer. > > Which p? > It takes care of argp->p, not the local 'p' variable, doesn't it. Grr, I meant "xdr->p" of course, via xdr_inline_decode (argp is the server's READ_BUF, sigh) > p += 2 has an equivalent side effect on 'p' as doing READ64. > I can do "p = argp;" instead though to reset 'p' onto p = xdr->p; ... Benny > the current xdr stream "head". > >>> BTW, this is exactly why the whole READ_BUF(), READ*(), WRITE*() macro >>> crap is so utterly broken. The magic that happens to the 'p' pointer is >>> completely opaque to someone unfamiliar with the code. > > I completely agree. We're dealing with bits and bytes (or 32 bit words > actually) at the wrong abstraction layer. > >>>>> >>>>> - /* Major id */ >>>>> + /* Throw away Major id */ >>>>> READ_BUF(4); >>>>> - READ32(res->server_owner.major_id_sz); >>>>> - READ_BUF(res->server_owner.major_id_sz); >>>>> - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); >>>>> + READ32(dummy); >>>>> + READ_BUF(dummy); >>>>> + p += XDR_QUADLEN(dummy); >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^ Ditto. You're skipping 2*dummy words. > > Why? > READ_BUF increments argp->p in XDR_QUADLEN(dummy) words > and the local p should be adjusted correspondingly. > This used to happen as the side effect of COPY_MEM. > >>>>> >>>>> - /* server_scope */ >>>>> + /* Throw away server_scope */ >>>>> READ_BUF(4); >>>>> - READ32(res->server_scope.server_scope_sz); >>>>> - READ_BUF(res->server_scope.server_scope_sz); >>>>> - COPYMEM(res->server_scope.server_scope, >>>>> - res->server_scope.server_scope_sz); >>>>> + READ32(dummy); >>>>> + READ_BUF(dummy); >>>>> + p += XDR_QUADLEN(dummy); >>>> ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto > > Ditto > >>>>> + >>>>> /* Throw away Implementation id array */ >>>>> READ_BUF(4); >>>>> READ32(dummy); >>>>> + READ_BUF(dummy); >>>>> p += XDR_QUADLEN(dummy); >>>> ^^^^^^^^^^^^^^^^^^^^^^^^ Ditto >>>> > > and here too... > > BTW, Calling READ_BUF here was missing before, so this could be put > in a patch of its own, and adjusting p is not really necessary since > we're about to exit the function... > > In any case, are we going to squash these fixes into the respective > queued patch, or would like to start accumulating the patches for > 2.6.31 without rebasing? > > Benny > >>>>> >>>>> return 0; >>>>> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h >>>>> index 071a6d1..62f63fb 100644 >>>>> --- a/include/linux/nfs_xdr.h >>>>> +++ b/include/linux/nfs_xdr.h >>>>> @@ -925,9 +925,6 @@ struct server_scope { >>>>> struct nfs41_exchange_id_res { >>>>> struct nfs_client *client; >>>>> u32 flags; >>>>> - struct server_owner server_owner; >>>>> - struct server_scope server_scope; >>>>> - struct nfs_impl_id4 impl_id; >>>>> }; >>>>> >>>>> struct nfs41_create_session_args { >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 20:35 ` Benny Halevy @ 2009-05-05 22:12 ` Trond Myklebust 2009-05-05 22:15 ` Benny Halevy 0 siblings, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-05 22:12 UTC (permalink / raw) To: Benny Halevy; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On Tue, 2009-05-05 at 23:35 +0300, Benny Halevy wrote: > On 2009-05-05 23:28, Benny Halevy wrote: > > On May. 05, 2009, 22:41 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > >> On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: > >>> On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: > >>>> On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: > >>>>> struct nfs41_exchange_id_res is currently allocated on the stack > >>>>> insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( > >>>>> byte arrays embedded in server_owner and server_scope. > >>>>> Since these are not in use yet, this patch gets rid of them for the > >>>>> time being. > >>>>> > >>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com> > >>>>> --- > >>>>> fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- > >>>>> include/linux/nfs_xdr.h | 3 --- > >>>>> 2 files changed, 14 insertions(+), 16 deletions(-) > >>>>> > >>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > >>>>> index 80af0ae..3350d19 100644 > >>>>> --- a/fs/nfs/nfs4xdr.c > >>>>> +++ b/fs/nfs/nfs4xdr.c > >>>>> @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) > >>>>> static int decode_exchange_id(struct xdr_stream *xdr, > >>>>> struct nfs41_exchange_id_res *res) > >>>>> { > >>>>> - uint32_t *p; > >>>>> - int status, dummy; > >>>>> + uint32_t *p, dummy; > >> Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' > >> checker will yell at you. > > > > Thanks! I'll send a fixed version > > of this patch and also look into the rest of the xdr code. > > > >>>>> + int status; > >>>>> struct nfs_client *clp = res->client; > >>>>> > >>>>> status = decode_op_hdr(xdr, OP_EXCHANGE_ID); > >>>>> @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, > >>>>> if (dummy != SP4_NONE) > >>>>> return -EIO; > >>>>> > >>>>> - /* minor_id */ > >>>>> + /* Throw away minor_id */ > >>>>> READ_BUF(8); > >>>>> - READ64(res->server_owner.minor_id); > >>>>> + p += 8; > >>>> ^^^^^^^^ Err... This isn't the same thing at all! > > > > Ouch, of course. What did I smoke that day? > > > >>>> You're suddenly skipping 10=words instead of the original 2. READ_BUF() > >>>> will already take care of updating the 'p' pointer. > > > > Which p? > > It takes care of argp->p, not the local 'p' variable, doesn't it. > > Grr, I meant "xdr->p" of course, via xdr_inline_decode > (argp is the server's READ_BUF, sigh) > > > p += 2 has an equivalent side effect on 'p' as doing READ64. > > I can do "p = argp;" instead though to reset 'p' onto > > p = xdr->p; ... Not necessary. Look again at the first line of the READ_BUF(nbytes) macro: p = xdr_inline_decode(xdr, nbytes); So the value of 'p' is always correctly set to the beginning of the buffer of length 'nbytes'. I'll make a point of removing those macro references from the client code in the next week or so. I'm getting really tired of them... Cheers Trond ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 22:12 ` Trond Myklebust @ 2009-05-05 22:15 ` Benny Halevy 2009-05-05 22:39 ` Trond Myklebust 2009-05-05 22:43 ` [pnfs] [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members J. Bruce Fields 0 siblings, 2 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-05 22:15 UTC (permalink / raw) To: Trond Myklebust; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On 2009-05-06 01:12, Trond Myklebust wrote: > On Tue, 2009-05-05 at 23:35 +0300, Benny Halevy wrote: >> On 2009-05-05 23:28, Benny Halevy wrote: >>> On May. 05, 2009, 22:41 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: >>>> On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: >>>>> On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: >>>>>> On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: >>>>>>> struct nfs41_exchange_id_res is currently allocated on the stack >>>>>>> insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( >>>>>>> byte arrays embedded in server_owner and server_scope. >>>>>>> Since these are not in use yet, this patch gets rid of them for the >>>>>>> time being. >>>>>>> >>>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >>>>>>> --- >>>>>>> fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- >>>>>>> include/linux/nfs_xdr.h | 3 --- >>>>>>> 2 files changed, 14 insertions(+), 16 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c >>>>>>> index 80af0ae..3350d19 100644 >>>>>>> --- a/fs/nfs/nfs4xdr.c >>>>>>> +++ b/fs/nfs/nfs4xdr.c >>>>>>> @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) >>>>>>> static int decode_exchange_id(struct xdr_stream *xdr, >>>>>>> struct nfs41_exchange_id_res *res) >>>>>>> { >>>>>>> - uint32_t *p; >>>>>>> - int status, dummy; >>>>>>> + uint32_t *p, dummy; >>>> Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' >>>> checker will yell at you. >>> Thanks! I'll send a fixed version >>> of this patch and also look into the rest of the xdr code. >>> >>>>>>> + int status; >>>>>>> struct nfs_client *clp = res->client; >>>>>>> >>>>>>> status = decode_op_hdr(xdr, OP_EXCHANGE_ID); >>>>>>> @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, >>>>>>> if (dummy != SP4_NONE) >>>>>>> return -EIO; >>>>>>> >>>>>>> - /* minor_id */ >>>>>>> + /* Throw away minor_id */ >>>>>>> READ_BUF(8); >>>>>>> - READ64(res->server_owner.minor_id); >>>>>>> + p += 8; >>>>>> ^^^^^^^^ Err... This isn't the same thing at all! >>> Ouch, of course. What did I smoke that day? >>> >>>>>> You're suddenly skipping 10=words instead of the original 2. READ_BUF() >>>>>> will already take care of updating the 'p' pointer. >>> Which p? >>> It takes care of argp->p, not the local 'p' variable, doesn't it. >> Grr, I meant "xdr->p" of course, via xdr_inline_decode >> (argp is the server's READ_BUF, sigh) >> >>> p += 2 has an equivalent side effect on 'p' as doing READ64. >>> I can do "p = argp;" instead though to reset 'p' onto >> p = xdr->p; ... > > Not necessary. Look again at the first line of the READ_BUF(nbytes) > macro: > > p = xdr_inline_decode(xdr, nbytes); > > So the value of 'p' is always correctly set to the beginning of the > buffer of length 'nbytes'. Right, but then we want to skip over the buffer. > > I'll make a point of removing those macro references from the client > code in the next week or so. I'm getting really tired of them... Yeah, I'm all for it. Let me know if there's anything I can do to help with that. Benny > > Cheers > Trond > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 22:15 ` Benny Halevy @ 2009-05-05 22:39 ` Trond Myklebust 2009-05-07 15:56 ` Benny Halevy 2009-05-05 22:43 ` [pnfs] [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members J. Bruce Fields 1 sibling, 1 reply; 22+ messages in thread From: Trond Myklebust @ 2009-05-05 22:39 UTC (permalink / raw) To: Benny Halevy; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On Wed, 2009-05-06 at 01:15 +0300, Benny Halevy wrote: > On 2009-05-06 01:12, Trond Myklebust wrote: > > Not necessary. Look again at the first line of the READ_BUF(nbytes) > > macro: > > > > p = xdr_inline_decode(xdr, nbytes); > > > > So the value of 'p' is always correctly set to the beginning of the > > buffer of length 'nbytes'. > > Right, but then we want to skip over the buffer. The next call to READ_BUF() will take care of updating p to the beginning of the next buffer irrespective of whether or not you have read data from the current buffer. Cheers Trond ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 22:39 ` Trond Myklebust @ 2009-05-07 15:56 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF Benny Halevy ` (5 more replies) 0 siblings, 6 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 15:56 UTC (permalink / raw) To: Trond Myklebust; +Cc: Andy Adamson, pnfs, Stephen Rothwell, linux-next On May. 06, 2009, 1:39 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > On Wed, 2009-05-06 at 01:15 +0300, Benny Halevy wrote: >> On 2009-05-06 01:12, Trond Myklebust wrote: >>> Not necessary. Look again at the first line of the READ_BUF(nbytes) >>> macro: >>> >>> p = xdr_inline_decode(xdr, nbytes); >>> >>> So the value of 'p' is always correctly set to the beginning of the >>> buffer of length 'nbytes'. >> Right, but then we want to skip over the buffer. > > The next call to READ_BUF() will take care of updating p to the > beginning of the next buffer irrespective of whether or not you have > read data from the current buffer. Got it. Thanks for being patient :-) > > Cheers > Trond > The following 6 patches fix nfs41 xdr code based on the current helpers (over nfs-2.6/nfsv41). I can squash them into the current patchset after your cleanup of the xdr macros. [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF [PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type [PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members [PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type [PATCH 5/6] nfs41: refactor decoding of channel attributes [PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type Benny ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF 2009-05-07 15:56 ` Benny Halevy @ 2009-05-07 16:00 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type Benny Halevy ` (4 subsequent siblings) 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:00 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy READ_BUF is required to adjust the xdr stream pointer. This does not affect anybody right now since we're only sending singleton EXCHANGE_IDs, but we must be good citizens and do our part properly in any case :-) Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index 80af0ae..d682af4 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -4223,7 +4223,7 @@ static int decode_exchange_id(struct xdr_stream *xdr, /* Throw away Implementation id array */ READ_BUF(4); READ32(dummy); - p += XDR_QUADLEN(dummy); + READ_BUF(dummy); return 0; } -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type 2009-05-07 15:56 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF Benny Halevy @ 2009-05-07 16:00 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy ` (3 subsequent siblings) 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:00 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy should point to __be32, not uint43_t. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index d682af4..b7cfd36 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1546,7 +1546,7 @@ static void encode_exchange_id(struct xdr_stream *xdr, struct nfs41_exchange_id_args *args, struct compound_hdr *hdr) { - uint32_t *p; + __be32 *p; RESERVE_SPACE(4 + sizeof(args->verifier->data)); WRITE32(OP_EXCHANGE_ID); @@ -4185,7 +4185,7 @@ static int decode_delegreturn(struct xdr_stream *xdr) static int decode_exchange_id(struct xdr_stream *xdr, struct nfs41_exchange_id_res *res) { - uint32_t *p; + __be32 *p; int status, dummy; struct nfs_client *clp = res->client; -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-07 15:56 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF Benny Halevy 2009-05-07 16:00 ` [PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type Benny Halevy @ 2009-05-07 16:00 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type Benny Halevy ` (2 subsequent siblings) 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:00 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy struct nfs41_exchange_id_res is currently allocated on the stack insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K) byte arrays embedded in server_owner and server_scope. Since these are not in use yet, this patch gets rid of them for the time being. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 22 ++++++++++------------ include/linux/nfs_xdr.h | 3 --- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index b7cfd36..d0fa05b 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -4186,7 +4186,8 @@ static int decode_exchange_id(struct xdr_stream *xdr, struct nfs41_exchange_id_res *res) { __be32 *p; - int status, dummy; + uint32_t dummy; + int status; struct nfs_client *clp = res->client; status = decode_op_hdr(xdr, OP_EXCHANGE_ID); @@ -4204,22 +4205,19 @@ static int decode_exchange_id(struct xdr_stream *xdr, if (dummy != SP4_NONE) return -EIO; - /* minor_id */ + /* Throw away minor_id */ READ_BUF(8); - READ64(res->server_owner.minor_id); - /* Major id */ + /* Throw away Major id */ READ_BUF(4); - READ32(res->server_owner.major_id_sz); - READ_BUF(res->server_owner.major_id_sz); - COPYMEM(res->server_owner.major_id, res->server_owner.major_id_sz); + READ32(dummy); + READ_BUF(dummy); - /* server_scope */ + /* Throw away server_scope */ READ_BUF(4); - READ32(res->server_scope.server_scope_sz); - READ_BUF(res->server_scope.server_scope_sz); - COPYMEM(res->server_scope.server_scope, - res->server_scope.server_scope_sz); + READ32(dummy); + READ_BUF(dummy); + /* Throw away Implementation id array */ READ_BUF(4); READ32(dummy); diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 071a6d1..62f63fb 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -925,9 +925,6 @@ struct server_scope { struct nfs41_exchange_id_res { struct nfs_client *client; u32 flags; - struct server_owner server_owner; - struct server_scope server_scope; - struct nfs_impl_id4 impl_id; }; struct nfs41_create_session_args { -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type 2009-05-07 15:56 ` Benny Halevy ` (2 preceding siblings ...) 2009-05-07 16:00 ` [PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy @ 2009-05-07 16:00 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 5/6] nfs41: refactor decoding of channel attributes Benny Halevy 2009-05-07 16:01 ` [PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type Benny Halevy 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:00 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy should point to __be32, not uint32_t. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index d0fa05b..ea16df2 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1566,7 +1566,7 @@ static void encode_create_session(struct xdr_stream *xdr, struct nfs41_create_session_args *args, struct compound_hdr *hdr) { - uint32_t *p; + __be32 *p; char machine_name[NFS4_MAX_MACHINE_NAME_LEN]; uint32_t len; struct nfs_client *clp = args->client; @@ -4229,7 +4229,7 @@ static int decode_exchange_id(struct xdr_stream *xdr, static int decode_create_session(struct xdr_stream *xdr, struct nfs41_create_session_res *res) { - uint32_t *p; + __be32 *p; int status; u32 nr_attrs; struct nfs_client *clp = res->client; -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] nfs41: refactor decoding of channel attributes 2009-05-07 15:56 ` Benny Halevy ` (3 preceding siblings ...) 2009-05-07 16:00 ` [PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type Benny Halevy @ 2009-05-07 16:00 ` Benny Halevy 2009-05-07 16:01 ` [PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type Benny Halevy 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:00 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy Also, verify ca_rdma_ird value and print a warning if > 1. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 60 +++++++++++++++++++++++++---------------------------- 1 files changed, 28 insertions(+), 32 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index ea16df2..e4c0209 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -4226,12 +4226,35 @@ static int decode_exchange_id(struct xdr_stream *xdr, return 0; } +static int decode_chan_attrs(struct xdr_stream *xdr, + struct nfs4_channel_attrs *attrs) +{ + __be32 *p; + u32 nr_attrs; + + READ_BUF(28); + READ32(attrs->headerpadsz); + READ32(attrs->max_rqst_sz); + READ32(attrs->max_resp_sz); + READ32(attrs->max_resp_sz_cached); + READ32(attrs->max_ops); + READ32(attrs->max_reqs); + READ32(nr_attrs); + if (unlikely(nr_attrs > 1)) { + printk(KERN_WARNING "%s: Invalid rdma channel attrs count %u\n", + __func__, nr_attrs); + return -EINVAL; + } + if (nr_attrs == 1) + READ_BUF(4); /* skip rdma_attrs */ + return 0; +} + static int decode_create_session(struct xdr_stream *xdr, struct nfs41_create_session_res *res) { __be32 *p; int status; - u32 nr_attrs; struct nfs_client *clp = res->client; struct nfs4_session *session = clp->cl_session; @@ -4250,37 +4273,10 @@ static int decode_create_session(struct xdr_stream *xdr, READ32(session->flags); /* Channel attributes */ - /* fore channel */ - READ_BUF(24); - READ32(session->fc_attrs.headerpadsz); - READ32(session->fc_attrs.max_rqst_sz); - READ32(session->fc_attrs.max_resp_sz); - READ32(session->fc_attrs.max_resp_sz_cached); - READ32(session->fc_attrs.max_ops); - READ32(session->fc_attrs.max_reqs); - READ_BUF(4); - READ32(nr_attrs); - if (nr_attrs == 1) { - READ_BUF(4); - p++; /* skip rdma_attrs */ - } - - /* back channel */ - READ_BUF(24); - READ32(session->bc_attrs.headerpadsz); - READ32(session->bc_attrs.max_rqst_sz); - READ32(session->bc_attrs.max_resp_sz); - READ32(session->bc_attrs.max_resp_sz_cached); - READ32(session->bc_attrs.max_ops); - READ32(session->bc_attrs.max_reqs); - READ_BUF(4); - READ32(nr_attrs); - if (nr_attrs == 1) { - READ_BUF(4); - p++; /* skip rdma_attrs */ - } - - return 0; + status = decode_chan_attrs(xdr, &session->fc_attrs); + if (!status) + status = decode_chan_attrs(xdr, &session->bc_attrs); + return status; } static int decode_destroy_session(struct xdr_stream *xdr, void *dummy) -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type 2009-05-07 15:56 ` Benny Halevy ` (4 preceding siblings ...) 2009-05-07 16:00 ` [PATCH 5/6] nfs41: refactor decoding of channel attributes Benny Halevy @ 2009-05-07 16:01 ` Benny Halevy 5 siblings, 0 replies; 22+ messages in thread From: Benny Halevy @ 2009-05-07 16:01 UTC (permalink / raw) To: Trond Myklebust Cc: Andy Adamson, pnfs, linux-next, Stephen Rothwell, Benny Halevy should point to __be32, not uint32_t. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfs/nfs4xdr.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c index e4c0209..617273e 100644 --- a/fs/nfs/nfs4xdr.c +++ b/fs/nfs/nfs4xdr.c @@ -1628,7 +1628,7 @@ static void encode_destroy_session(struct xdr_stream *xdr, struct nfs4_session *session, struct compound_hdr *hdr) { - uint32_t *p; + __be32 *p; RESERVE_SPACE(4 + NFS4_MAX_SESSIONID_LEN); WRITE32(OP_DESTROY_SESSION); WRITEMEM(session->sess_id.data, NFS4_MAX_SESSIONID_LEN); -- 1.6.2.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [pnfs] [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members 2009-05-05 22:15 ` Benny Halevy 2009-05-05 22:39 ` Trond Myklebust @ 2009-05-05 22:43 ` J. Bruce Fields 1 sibling, 0 replies; 22+ messages in thread From: J. Bruce Fields @ 2009-05-05 22:43 UTC (permalink / raw) To: Benny Halevy Cc: Trond Myklebust, Andy Adamson, pnfs, linux-next, Stephen Rothwell On Wed, May 06, 2009 at 01:15:57AM +0300, Benny Halevy wrote: > On 2009-05-06 01:12, Trond Myklebust wrote: > > On Tue, 2009-05-05 at 23:35 +0300, Benny Halevy wrote: > >> On 2009-05-05 23:28, Benny Halevy wrote: > >>> On May. 05, 2009, 22:41 +0300, Trond Myklebust <trond.myklebust@fys.uio.no> wrote: > >>>> On Tue, 2009-05-05 at 15:39 -0400, Trond Myklebust wrote: > >>>>> On Tue, 2009-05-05 at 15:34 -0400, Trond Myklebust wrote: > >>>>>> On Fri, 2009-05-01 at 23:14 +0300, Benny Halevy wrote: > >>>>>>> struct nfs41_exchange_id_res is currently allocated on the stack > >>>>>>> insanely taking over 2K of stack space due to the NFS4_OPAQUE_LIMIT (1K( > >>>>>>> byte arrays embedded in server_owner and server_scope. > >>>>>>> Since these are not in use yet, this patch gets rid of them for the > >>>>>>> time being. > >>>>>>> > >>>>>>> Signed-off-by: Benny Halevy <bhalevy@panasas.com> > >>>>>>> --- > >>>>>>> fs/nfs/nfs4xdr.c | 27 ++++++++++++++------------- > >>>>>>> include/linux/nfs_xdr.h | 3 --- > >>>>>>> 2 files changed, 14 insertions(+), 16 deletions(-) > >>>>>>> > >>>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > >>>>>>> index 80af0ae..3350d19 100644 > >>>>>>> --- a/fs/nfs/nfs4xdr.c > >>>>>>> +++ b/fs/nfs/nfs4xdr.c > >>>>>>> @@ -4185,8 +4185,8 @@ static int decode_delegreturn(struct xdr_stream *xdr) > >>>>>>> static int decode_exchange_id(struct xdr_stream *xdr, > >>>>>>> struct nfs41_exchange_id_res *res) > >>>>>>> { > >>>>>>> - uint32_t *p; > >>>>>>> - int status, dummy; > >>>>>>> + uint32_t *p, dummy; > >>>> Oh, and 'p' _always_ has to be of type '__be32', otherwise the 'sparse' > >>>> checker will yell at you. > >>> Thanks! I'll send a fixed version > >>> of this patch and also look into the rest of the xdr code. > >>> > >>>>>>> + int status; > >>>>>>> struct nfs_client *clp = res->client; > >>>>>>> > >>>>>>> status = decode_op_hdr(xdr, OP_EXCHANGE_ID); > >>>>>>> @@ -4204,25 +4204,26 @@ static int decode_exchange_id(struct xdr_stream *xdr, > >>>>>>> if (dummy != SP4_NONE) > >>>>>>> return -EIO; > >>>>>>> > >>>>>>> - /* minor_id */ > >>>>>>> + /* Throw away minor_id */ > >>>>>>> READ_BUF(8); > >>>>>>> - READ64(res->server_owner.minor_id); > >>>>>>> + p += 8; > >>>>>> ^^^^^^^^ Err... This isn't the same thing at all! > >>> Ouch, of course. What did I smoke that day? > >>> > >>>>>> You're suddenly skipping 10=words instead of the original 2. READ_BUF() > >>>>>> will already take care of updating the 'p' pointer. > >>> Which p? > >>> It takes care of argp->p, not the local 'p' variable, doesn't it. > >> Grr, I meant "xdr->p" of course, via xdr_inline_decode > >> (argp is the server's READ_BUF, sigh) > >> > >>> p += 2 has an equivalent side effect on 'p' as doing READ64. > >>> I can do "p = argp;" instead though to reset 'p' onto > >> p = xdr->p; ... > > > > Not necessary. Look again at the first line of the READ_BUF(nbytes) > > macro: > > > > p = xdr_inline_decode(xdr, nbytes); > > > > So the value of 'p' is always correctly set to the beginning of the > > buffer of length 'nbytes'. > > Right, but then we want to skip over the buffer. > > > > > I'll make a point of removing those macro references from the client > > code in the next week or so. I'm getting really tired of them... > > Yeah, I'm all for it. > Let me know if there's anything I can do to help with that. Ditto. (I'll probably follow your example in fs/nfsd.) --b. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: linux-next: nfs tree build warning 2009-05-01 3:15 linux-next: nfs tree build warning Stephen Rothwell 2009-05-01 3:22 ` Trond Myklebust @ 2009-06-09 9:13 ` Stephen Rothwell 1 sibling, 0 replies; 22+ messages in thread From: Stephen Rothwell @ 2009-06-09 9:13 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-next, Benny Halevy, Andy Adamson [-- Attachment #1: Type: text/plain, Size: 503 bytes --] Hi Trond, On Fri, 1 May 2009 13:15:45 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote: > > Today's linux-next build (x86_64 allmodconfig) produced this warning: > > fs/nfs/nfs4proc.c: In function 'nfs4_proc_exchange_id': > fs/nfs/nfs4proc.c:4279: warning: the frame size of 2288 bytes is larger than 2048 bytes I am still getting this warning. What happened to Benny's patch? -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2009-06-09 9:13 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-05-01 3:15 linux-next: nfs tree build warning Stephen Rothwell 2009-05-01 3:22 ` Trond Myklebust 2009-05-01 12:19 ` Benny Halevy 2009-05-01 14:56 ` William A. (Andy) Adamson 2009-05-01 20:14 ` [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy 2009-05-05 19:34 ` Trond Myklebust 2009-05-05 19:39 ` Trond Myklebust 2009-05-05 19:41 ` Trond Myklebust 2009-05-05 20:28 ` Benny Halevy 2009-05-05 20:35 ` Benny Halevy 2009-05-05 22:12 ` Trond Myklebust 2009-05-05 22:15 ` Benny Halevy 2009-05-05 22:39 ` Trond Myklebust 2009-05-07 15:56 ` Benny Halevy 2009-05-07 16:00 ` [PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF Benny Halevy 2009-05-07 16:00 ` [PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type Benny Halevy 2009-05-07 16:00 ` [PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members Benny Halevy 2009-05-07 16:00 ` [PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type Benny Halevy 2009-05-07 16:00 ` [PATCH 5/6] nfs41: refactor decoding of channel attributes Benny Halevy 2009-05-07 16:01 ` [PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type Benny Halevy 2009-05-05 22:43 ` [pnfs] [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members J. Bruce Fields 2009-06-09 9:13 ` linux-next: nfs tree build warning Stephen Rothwell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).