[NFS] Increase size of struct fid raw buffer

[NFS] Increase size of struct fid raw buffer

[Steven Whitehouse]

>From ba713d661333d790db5106c54d1f0ee94a876944 Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Mon, 7 Apr 2008 09:53:24 +0100
Subject: [PATCH] [NFS] Increase size of struct fid raw buffer

GFS2 requires the NFS filehandle buffer to be larger than the
minimum size as per the bug report: http://lkml.org/lkml/2007/10/24/374
Its a pretty trivial fix for now and I've done a test which shows
that it works ok.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Neil Brown <neilb@suse.de>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Adrian Bunk <bunk@kernel.org>

diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index adcbb05..a6e928d 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -43,7 +43,7 @@ struct fid {
 			u32 parent_ino;
 			u32 parent_gen;
 		} i32;
-		__u32 raw[6];
+		__u32 raw[8];
 	};
 };
 
-- 
1.5.1.2

Re: [NFS] Increase size of struct fid raw buffer

[Linus Torvalds]

On Mon, 7 Apr 2008, Steven Whitehouse wrote:
> 
> GFS2 requires the NFS filehandle buffer to be larger than the
> minimum size as per the bug report: http://lkml.org/lkml/2007/10/24/374
> Its a pretty trivial fix for now and I've done a test which shows
> that it works ok.

I'm not seeing the point of this.

Every single instance of "struct fid" that I saw in a quick grep was 
created not as a "struct fid", but as some other data structure that was 
then cast to a "struct fid *".

So the _underlying_ size of "struct fid" seems to be pretty random, and 
totally unrelated to this declaration.

But admittedly that really was just a quick grep, and maybe I missed 
something. But it seems like this patch doesn't really change anything, 
just largely makes a change in a structure that is apparently used as an 
opaque pointer.

Is there anything that actually uses "struct fid" as an _allocation_ 
entity?

And if not, then that "_u32 raw[6]" should probably be a un-sized "_u32 
raw[]" instead, no?

		Linus

Re: [NFS] Increase size of struct fid raw buffer

[Steven Whitehouse]

Hi,

On Mon, 2008-04-07 at 08:54 -0700, Linus Torvalds wrote:
> 
> On Mon, 7 Apr 2008, Steven Whitehouse wrote:
> > 
> > GFS2 requires the NFS filehandle buffer to be larger than the
> > minimum size as per the bug report: http://lkml.org/lkml/2007/10/24/374
> > Its a pretty trivial fix for now and I've done a test which shows
> > that it works ok.
> 
> I'm not seeing the point of this.
> 
> Every single instance of "struct fid" that I saw in a quick grep was 
> created not as a "struct fid", but as some other data structure that was 
> then cast to a "struct fid *".
> 
> So the _underlying_ size of "struct fid" seems to be pretty random, and 
> totally unrelated to this declaration.
> 
> But admittedly that really was just a quick grep, and maybe I missed 
> something. But it seems like this patch doesn't really change anything, 
> just largely makes a change in a structure that is apparently used as an 
> opaque pointer.
> 
> Is there anything that actually uses "struct fid" as an _allocation_ 
> entity?
> 
> And if not, then that "_u32 raw[6]" should probably be a un-sized "_u32 
> raw[]" instead, no?
> 
> 		Linus

I'm happy with that solution, although I'd assumed that the reason this
field had a size in the first place was that the NFS people had a plan
to use the structure as an allocation entity in the future. Can an NFS
developer please confirm/deny this?

If everybody is happy with the plan, then I'll send a patch to make the
change as you suggest shortly,

Steve.

Re: [NFS] Increase size of struct fid raw buffer

[Christoph Hellwig]

On Tue, Apr 08, 2008 at 08:50:05AM +0100, Steven Whitehouse wrote:
> I'm happy with that solution, although I'd assumed that the reason this
> field had a size in the first place was that the NFS people had a plan
> to use the structure as an allocation entity in the future. Can an NFS
> developer please confirm/deny this?
> 
> If everybody is happy with the plan, then I'll send a patch to make the
> change as you suggest shortly,

I've introduce it and I don't plan to use it as allocation entity.  I
don't quite remember why I sized it either, so it's conceptually fine
to make it a VLA.  Just do some testing with that variant please.

Re: [NFS] Increase size of struct fid raw buffer

[Steven Whitehouse]

Hi,

On Tue, 2008-04-08 at 03:58 -0400, Christoph Hellwig wrote:
> On Tue, Apr 08, 2008 at 08:50:05AM +0100, Steven Whitehouse wrote:
> > I'm happy with that solution, although I'd assumed that the reason this
> > field had a size in the first place was that the NFS people had a plan
> > to use the structure as an allocation entity in the future. Can an NFS
> > developer please confirm/deny this?
> > 
> > If everybody is happy with the plan, then I'll send a patch to make the
> > change as you suggest shortly,
> 
> I've introduce it and I don't plan to use it as allocation entity.  I
> don't quite remember why I sized it either, so it's conceptually fine
> to make it a VLA.  Just do some testing with that variant please.

Then we get this:

In file included from mm/shmem.c:30:
include/linux/exportfs.h:46: error: flexible array member in union
make[1]: *** [mm/shmem.o] Error 1
make: *** [mm] Error 2

but we can have a zero length array though, so I'll test that and send a
patch in due course,

Steve.

[NFS] Use a zero sized array for raw field in struct fid

[Steven Whitehouse]

>From b8732f8bea4e8abc331b8fa58a4047c91e2e7d02 Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Tue, 8 Apr 2008 13:12:52 +0100
Subject: [PATCH] [NFS] Use a zero sized array for raw field in struct fid

The raw field's size can vary so we use a zero sized array since
gcc will not allow a variable sized array inside a union. This
has been tested with ext3 and gfs2 and relates to the bug
report: http://lkml.org/lkml/2007/10/24/374 and discussion
thread: http://lkml.org/lkml/2008/4/7/65

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Neil Brown <neilb@suse.de>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Adrian Bunk <bunk@kernel.org>

diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index adcbb05..de8387b 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -43,7 +43,7 @@ struct fid {
 			u32 parent_ino;
 			u32 parent_gen;
 		} i32;
-		__u32 raw[6];
+		__u32 raw[0];
 	};
 };
 
-- 
1.5.1.2

Re: [NFS] Use a zero sized array for raw field in struct fid

[J. Bruce Fields]

On Tue, Apr 08, 2008 at 02:01:26PM +0100, Steven Whitehouse wrote:
> >From b8732f8bea4e8abc331b8fa58a4047c91e2e7d02 Mon Sep 17 00:00:00 2001
> From: Steven Whitehouse <swhiteho@redhat.com>
> Date: Tue, 8 Apr 2008 13:12:52 +0100
> Subject: [PATCH] [NFS] Use a zero sized array for raw field in struct fid
> 
> The raw field's size can vary so we use a zero sized array since
> gcc will not allow a variable sized array inside a union. This
> has been tested with ext3 and gfs2 and relates to the bug
> report: http://lkml.org/lkml/2007/10/24/374 and discussion
> thread: http://lkml.org/lkml/2008/4/7/65

OK; queued for 2.6.26.--b.

> 
> Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Neil Brown <neilb@suse.de>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: Adrian Bunk <bunk@kernel.org>
> 
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index adcbb05..de8387b 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -43,7 +43,7 @@ struct fid {
>  			u32 parent_ino;
>  			u32 parent_gen;
>  		} i32;
> -		__u32 raw[6];
> +		__u32 raw[0];
>  	};
>  };
>  
> -- 
> 1.5.1.2
> 
> 
>