linux-next: nfs tree build warning

linux-next: nfs tree build warning

[Stephen Rothwell]

[-- 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 --]

Re: linux-next: nfs tree build warning

[Trond Myklebust]

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

Re: linux-next: nfs tree build warning

[Benny Halevy]

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

Re: linux-next: nfs tree build warning

[William A. (Andy) Adamson]

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
>

[PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[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

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Trond Myklebust]

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 {

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Trond Myklebust]

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 {
> 

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Trond Myklebust]

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 {
> > 
> 

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Benny Halevy]

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 {
> 
> 

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Benny Halevy]

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 {
>>

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Trond Myklebust]

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

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Benny Halevy]

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
> 

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Trond Myklebust]

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

Re: [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[Benny Halevy]

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

[PATCH 1/6] nfs41: Ignoring impid in decode_exchange_id is missing a READ_BUF

[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

[PATCH 2/6] nfs41: fix Xcode_exchange_id's xdr Xcoding pointer type

[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

[PATCH 3/6] nfs41: get rid of unused struct nfs41_exchange_id_res members

[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

[PATCH 4/6] nfs41: fix Xcode_create_session's xdr Xcoding pointer type

[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

[PATCH 5/6] nfs41: refactor decoding of channel attributes

[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

[PATCH 6/6] nfs41: fix encode_destroy_session's xdr Xcoding pointer type

[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

Re: [pnfs] [PATCH] nfs41: get rid of unused struct nfs41_exchange_id_res members

[J. Bruce Fields]

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.

Re: linux-next: nfs tree build warning

[Stephen Rothwell]

[-- 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 --]