Compatibility issue with 2.2.19pre7

Hi,

is this part of 2.2.19pre7 really a good idea? Even in 2.4.0 the size field is still a short.

                                                                  -o)
    Hubert Mantel              Goodbye, dots...                   /\\
                                                                 _\_v

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla/include/linux/nfs.h linux.19p7/include/linux/nfs.h
@@ -94,7 +94,7 @@
  */
 #define NFS_MAXFHSIZE		64
 struct nfs_fh {
-	unsigned short		size;
+	unsigned int		size;
 	unsigned char		data[NFS_MAXFHSIZE];
 };
 

It needs to change in 2.4 as well. The cast of data to a struct isnt portable to all architectures.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

> Hubert Mantel writes:
>> is this part of 2.2.19pre7 really a good idea? Even in 2.4.0 >> the size field is still a short.
>> #define NFS_MAXFHSIZE 64
>> struct nfs_fh {
>> - unsigned short size;
>> + unsigned int size;
>> unsigned char data[NFS_MAXFHSIZE]; };

> This is an internal kernel data structure. Do you know of some > program that breaks as a result of this?
> _____

Any program which mounts an NFS partition.

If you do this, then you need to provide some sort of compatibility layer for nfs_mount.h since the format for version 4 of the NFS mount structure was decided more than 2 years ago.

Cheers,
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

No, it isn't, that's the whole point.

(spotted by Andi) util-linux-2.10o/mount/nfs_mount4.h:

struct nfs3_fh {
        unsigned short          size;
        unsigned char           data[64];
};

(see also nfs_mount_data structure in both kernel and mount)

I also don't understand Alan's comment, what has the cast of data to a structure have to do with the size of a field in the structure? Furthmore the cast of data to a struct should work on all architectures as far as C is concerned (if you then run alignment problems then it's your mistake).

As far I can see the only reason size makes sense to be 32bit is to get some more strict behaviour in the below code (to avoid discarding the most significant 16bits in sanity checks like this):

nlm4_decode_fh(u32 *p, struct nfs_fh *f)

{
        memset(f->data, 0, sizeof(f->data));
        f->size = ntohl(*p++);
        if (f->size > NFS_MAXFHSIZE) {
                printk(KERN_NOTICE
                        "lockd: bad fhandle size %d (should be <=%d)\n",
                        f->size, NFS_MAXFHSIZE);
                return NULL;
        }
        memcpy(f->data, p, f->size);
        return p + XDR_QUADLEN(f->size);
}

So for now I backed it out. If you want to push it in again then implement it right and make an mount backwards compatible nfs_fh type for the nfs_mount_data.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Well, let me put it this way:

1. NFS locking worked on ARM in 2.2.17 without any problems or modifications what so ever.

2. NFS locking does NOT work on ARM without the above patch. It FAILS completely.

I won't bother replying to this, since you obviously know the answer from your following comment. ;)

WRONG WRONG WRONG WRONG WRONG. You are *SO* wrong.

C explicitly allows this behaviour. The fact that most processors can load unaligned data without any trouble is merely co-incidence and leads to BAD programming techniques, like this one illusatrated above.

Therefore, it is BAD code which needs to be fixed. Now, on ARM, all structures are defined by the ABI to be aligned to a 32-bit word. End of story. No other story will do. Finito. Capisco?

Yes, you can get around them by taking an alignment abort, but that is REALLY REALLY expensive. Eg, instead of a load taking 1 cycle, it turns into around 500 cycles, just to decode the instruction by hand, calculate the addresses, calculate the side effects and implement them.

So, if we can get away with a little BETTER coding technique, then that is FAR FAR better than thaning a 499 cycle penalty.

Oh, and did I mention that x86 also has a performance penalty for unaligned loads as well?

This is pretty gawling, especially as the fix was posted to the NFS development list and not one person complained that it would break any APIs.

I'd like the NFS developers to look at this. Yes, I'm delegating it. Why? I've got too much on my plate at the moment, and I'm desperately trying to get rid of the 2.2 ARM kernel tree so I can concentrate on 2.4 only. In the spirit of open source, I am willing to test patches that claim to fix this problem once I've got my 2.2 kernel tree back into a reasonable state (after chasing some 128MB bad DIMM problem around for 2 months across 2 versions of Linux).

And yes, APIs shouldn't break in a major kernel release. Its a shame some API broke in 2.2.18.
_____

  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        [email protected]      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Sure, also alpha and sparc faults on misaligned accesses (on alpha we're nicer because we emulate them in kernel most of the time). I'm not missing those alignment issues.

Read again and you'll see that what I said isn't that ARM is broken because doesn't handle misaligned accesses. What I said is that I can write this C code:

	int x[2], * p = (int *) (((char *) &x)+1);
	main()
	{
		*p = 0;
	}

This is legal C code. Try compiling this on sparc that bus error on the above code. The cast works. Works fine. Compiler is happy.

Then when you execute the code the asm will happen to derefernce a misaligned and it breaks at runtime.

But it's _not_ the cast that is wrong. It's the _usage_ of the cast that is wrong, and it's your mistake not C's mistake or cast's mistake (and with "your" I didn't mean Russell's, you obviously care about those issues ;).

Infact the usage of the same below cast will work fine on ARM and sparc too, but now incidentally on an address that is properly aligned:

	int x[2], * p = (int *) (((char *) &x)+4);
	main()
	{
		*p = 0;
	}

Is it clear now what I said? Capito? ;)

I never suggest you should emulate this in kernel.

I know.

They missed it.

No problem, AFIK Andi will take care of it and he will implement the right fix.

However I'd _love_ to see the EIP where you get the fault, I currently don't see the line of code that is crashing your ARM and I know this code doesn't segfault on ARM.

        struct x {
                unsigned short x;
                unsigned char c[64];
        };
 
        main()
        {
                struct x x;
                x.x = 0;
		bzero(x.c, 64);	
        }

What broke in 2.2.18?

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Examine nlm_lookup_file() and the usage of fh->fh_dev and fh->fh_ino. What happens is that in that dprintk fh_dev and fh_ino contain garbage and as such, the nlm_lookup_file fails (since it doesn't refer to a valid device and inode pair). Therefore locking does not work.

Note: I've never said that it crashes. I've only said that NFS locking does not work.

The API changed:

struct nfs_mount_data {

        int             version;                /* 1 */
        int             fd;                     /* 1 */
-       struct nfs_fh   root;                   /* 1 */
+       struct nfs2_fh  old_root;               /* 1 */
        int             flags;                  /* 1 */
        int             rsize;                  /* 1 */
        int             wsize;                  /* 1 */
@@ -35,6 +42,7 @@
        char            hostname[256];          /* 1 */
        int             namlen;                 /* 2 */
        unsigned int    bsize;                  /* 3 */
+       struct nfs_fh   root;                   /* 4 */

};

which then caused this breakage. Therefore, I still propose my original patch as a bug fix against something that appears to be brand new in design.

Anyway, changing it back will break the API on ARM, so either way API breakage WILL happen at some point. The real question is which is better:

1. Yucky code in the NFS layers to copy a nfs_fh from userspace to kernel space, translating it into something sane.
2. Yucky code in the NFS layers to manually handle the nfs_fh to knfs_fh translation.
3. Accept the breakage of a *brand new* API in 2.2.18 and suffer zero yucky code.
_____

  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        [email protected]      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Zitiere Russell King <[email protected]>:

struct nfs_fh {
    unsigned short          size;
    unsigned char           data[NFS_MAXFHSIZE];
}

But it changed in 2.2.19pre, breaking nfs mount on i386.

-Andi

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Zitiere Andi Kleen <[email protected]>:

-ECONFUSED.

2.2.17 struct nfs_fh is identical to 2.2.18 nfs2_fh and 2.2.19pre7 nfs2_fh

2.2.18 struct nfs_fh is a new structure for nfsV3, it doesn't exist in 2.2.17. That structure is unusable on ARM.

Russel want's to change the new "struct nfs_fh" (from 2.2.18), and that change is included in 2.2.19pre7. But that change breaks i386 nfs mount.

Could someone with an Alpha/Sparc/ARM compiler compile a test program with "struct nfs_fh" from 2.2.18 and print the offset of nfs_fh.data?

--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Correct.

Unfortunately.

2
_____

  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        [email protected]      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

The change is rather useless anyways, because even in NFSv3 file handles cannot be >64bytes. Would even fit in a char, doesn't need a short nor an int.

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Indeed, but whether it be a char or a short, it'll still break on ARM. My original set of 3 solutions still stand therefore.
_____

  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        [email protected]      --- ---
  | | | |   http://www.arm.linux.org.uk/~rmk/aboutme.html    /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Err, no. This is not "legal" by any stretch of the imagination. This code has undefined behaviour.

As such, it may work, it may sigbus, it may write data at some address unrelated to "x", or it may start World War III (with appropriate hardware attached).

We aren't even obliged to allow this to compile.

r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

I know this code has undefined behaviour at _runtime_. But I thought you were obliged to allow it to compile. That was my only point.

I'd love if you could forbid it to compile.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

In article <[email protected]> you write:

There is no distinction between compilation and runtime in the standard. Actually, C could be interpreted, or a very smart compiler could also think real hard and replace the whole program by an equivalent printf().

Besides, a standard (C99) compiler will reject the 'main' definition. At least, the return type cannot be implicit anymore.

--Thomas Pornin
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Problem is that there's stuff like this all over the place. Plus, just because something is undefined by the standard doesn't mean it's not useful -- it's not possible to write either a kernel or libc without breaking some rules.

So any useful compile-time restrictions we could make is if the static expression was not computable (perhaps due to being a word addressed machine), or if we could somehow magically divine that the store would in fact go wrong at runtime.

r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

That's why I thought you were required to make it to compile. For example you don't know if there's another object that will cast the int pointer back to char pointer before dereferencing. That would get a defined runtime behaviour on all archs.

And yes, I see above I'm not allowed to say "runtime" nor "defined behaviour" according to the standard, but that's how thing works in practice.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

No. The representation of "int*" and "char*" needn't be the same. And isn't on old word addressed machines.

r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

     > As far I can see the only reason size makes sense to be 32bit
     > is to get some more strict behaviour in the below code (to
     > avoid discarding the most significant 16bits in sanity checks
     > like this):

     > nlm4_decode_fh(u32 *p, struct nfs_fh *f) {
     >         memset(f->data, 0, sizeof(f->data));
     >         f-> size = ntohl(*p++);
     >         if (f->size > NFS_MAXFHSIZE) {
     >                 printk(KERN_NOTICE
     >                         "lockd: bad fhandle size %d (should be
     >                         <=%d)\n",
     >                         f-> size, NFS_MAXFHSIZE);
     >                 return NULL;
     >         }
     >         memcpy(f->data, p, f->size);
     >         return p + XDR_QUADLEN(f->size);
     > }

I agree, and if that's the only problem, then the appended patch will fix it without any need to change struct nfs_fh.

As for the issue of casting 'fh->data' as a 'struct knfsd' then that is a perfectly valid operation.
The fh->data is a cookie as far as the client is concerned, and hence it will pass back exactly whatever the server sent it (alignment and all).

IOW: the knfsd server copied a struct knfsd and sent it off to the client, and now the exact same server is receiving a completely unadulterated version of said struct knfsd for use by the lockd server routines.
Unless somebody is using one compiler for the knfsd directory and then a completely different one for lockd, I fail to see why this should result in structure alignment problems on PPC or on any other platform.

Cheers,
Trond

@@ -83,16 +83,19 @@
 static u32 *
 nlm4_decode_fh(u32 *p, struct nfs_fh *f)
 {
+	unsigned int size;
+
 	memset(f->data, 0, sizeof(f->data));
-	f->size = ntohl(*p++);
-	if (f->size > NFS_MAXFHSIZE) {
+	size = ntohl(*p++);
+	if (size > NFS_MAXFHSIZE) {
 		printk(KERN_NOTICE
 			"lockd: bad fhandle size %x (should be %d)\n",
-			f->size, NFS_MAXFHSIZE);
+			size, NFS_MAXFHSIZE);
 		return NULL;
 	}
-      	memcpy(f->data, p, f->size);
-	return p + XDR_QUADLEN(f->size);
+	f->size = size;
+      	memcpy(f->data, p, size);
+	return p + XDR_QUADLEN(size);
 }
 
 static u32 *

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

No it isn't.

fh->data is an array of characters, thus without any alignment restrictions.
'struct knfsd' begins with a pointer, thus it must be 4 or 8 byte aligned.

The portable 'struct nfs_fh' structure would be

#define NFS_HANDLESIZE 64
struct nfs_fh

{
	unsigned short len;
	void* data[NFS_HANDLESIZE/sizeof(void*)];
};

But now its too late for such a change - it breaks at least i386, probably all platforms.

Does knfsd actually need all 64 bytes in the nfs_fh? What about aligning the 'struct knfsd' manually?

- struct knfsd* ptr = fh->data;
+ struct knfsd* ptr = (fh->data+15)&(~15);

That would be kernel only, no ABI problems.

--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

     > Trond Myklebust wrote:
    >>
    >>
    >> As for the issue of casting 'fh->data' as a 'struct knfsd' then
    >> that is a perfectly valid operation.
    >>
     > No it isn't.

     > fh-> data is an array of characters, thus without any alignment
     > restrictions.  'struct knfsd' begins with a pointer, thus it
     > must be 4 or 8 byte aligned.

     > The portable 'struct nfs_fh' structure would be

     > #define NFS_HANDLESIZE 64
     > struct nfs_fh {
     > 	unsigned short len; void* data[NFS_HANDLESIZE/sizeof(void*)];
     > };

Ok. I see your point now. How about the appended patch then? It means an extra copy operation, but it should be a lot less ugly than doing manual alignment...

Cheers,
Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
@@ -49,34 +49,37 @@
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 					struct nfs_fh *f)
 {
-	struct knfs_fh	*fh = (struct knfs_fh *) f->data;
+	struct knfs_fh	fh;
 	struct nlm_file	*file;
 	unsigned int	hash;
 	u32		nfserr;
 
+	/* Copy filehandle to avoid pointer alignment issues */
+	memcpy(&fh, f->data, sizeof(fh));
+
 	dprintk("lockd: nlm_file_lookup(%s/%u)\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 
-	hash = file_hash(u32_to_kdev_t(fh->fh_dev), u32_to_ino_t(fh->fh_ino));
+	hash = file_hash(u32_to_kdev_t(fh.fh_dev), u32_to_ino_t(fh.fh_ino));
 
 	/* Lock file table */
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (file->f_handle.fh_dcookie == fh.fh_dcookie &&
+		    !memcmp(&file->f_handle, &fh, sizeof(fh)))
 			goto found;
 	}
 
 	dprintk("lockd: creating file for %s/%u\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 	nfserr = nlm4_lck_denied_nolocks;
 	file = (struct nlm_file *) kmalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
 		goto out_unlock;
 
 	memset(file, 0, sizeof(*file));
-	file->f_handle = *fh;
+	memcpy(&file->f_handle, &fh, sizeof(file->f_handle));
 	file->f_sema   = MUTEX;
 
 	/* Open the file. Note that this must not sleep for too long, else

@@ -85,7 +88,7 @@
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, fh, &file->f_file)) != 0) {
+	if ((nfserr = nlmsvc_ops->fopen(rqstp, &fh, &file->f_file)) != 0) {
 		dprintk("lockd: open failed (nfserr %d)\n", ntohl(nfserr));
 		goto out_free;
 	}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

     > - if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
     > - !memcmp(&file->f_handle, fh, sizeof(*fh)))
     > + if (file->f_handle.fh_dcookie == fh.fh_dcookie &&
     > + !memcmp(&file->f_handle, &fh, sizeof(fh)))
     >  			goto found;

Come to think of it, this line looks pretty insane. Why on earth are we testing fh_dcookie twice?

I suspect that just the elimination of the redundant comparison in the above line would eliminate Russell's problem entirely, given that it's the only place in the entire routine where we actually reference fh->fh_base.fb_dentry.

In all other cases, we're referencing ordinary integers. Are there any alignment requirements on them?

Cheers,
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

On some arch int can be read only at a sizeof(int) byte aligned address (details in my example in reply to Russell).

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

> On Thu, Jan 11, 2001 at 07:22:03PM +0100, Trond Myklebust > wrote:
>> [..] Are there any alignment requirements on them?

> On some arch int can be read only at a sizeof(int) byte aligned > address (details in my example in reply to Russell).

OK. In that case my patch, would just be amended to eliminate the redundant comparison as is the case below.

Cheers,
Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
@@ -49,34 +49,36 @@
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,
 					struct nfs_fh *f)
 {
-	struct knfs_fh	*fh = (struct knfs_fh *) f->data;
+	struct knfs_fh	fh;
 	struct nlm_file	*file;
 	unsigned int	hash;
 	u32		nfserr;
 
+	/* Copy filehandle to avoid pointer alignment issues */
+	memcpy(&fh, f->data, sizeof(fh));
+
 	dprintk("lockd: nlm_file_lookup(%s/%u)\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 
-	hash = file_hash(u32_to_kdev_t(fh->fh_dev), u32_to_ino_t(fh->fh_ino));
+	hash = file_hash(u32_to_kdev_t(fh.fh_dev), u32_to_ino_t(fh.fh_ino));
 
 	/* Lock file table */
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (!memcmp(&file->f_handle, &fh, sizeof(fh)))
 			goto found;
 	}
 
 	dprintk("lockd: creating file for %s/%u\n",
-		kdevname(u32_to_kdev_t(fh->fh_dev)), fh->fh_ino);
+		kdevname(u32_to_kdev_t(fh.fh_dev)), fh.fh_ino);
 	nfserr = nlm4_lck_denied_nolocks;
 	file = (struct nlm_file *) kmalloc(sizeof(*file), GFP_KERNEL);
 	if (!file)
 		goto out_unlock;
 
 	memset(file, 0, sizeof(*file));
-	file->f_handle = *fh;
+	memcpy(&file->f_handle, &fh, sizeof(file->f_handle));
 	file->f_sema   = MUTEX;
 
 	/* Open the file. Note that this must not sleep for too long, else

@@ -85,7 +87,7 @@
 	 * We have to make sure we have the right credential to open
 	 * the file.
 	 */
-	if ((nfserr = nlmsvc_ops->fopen(rqstp, fh, &file->f_file)) != 0) {
+	if ((nfserr = nlmsvc_ops->fopen(rqstp, &fh, &file->f_file)) != 0) {
 		dprintk("lockd: open failed (nfserr %d)\n", ntohl(nfserr));
 		goto out_free;
 	}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

This patch looks fine w.r.t. alignment but given the below seems called at runtime (not just at mount time) for performance and to save a dozen of bytes of kernel stack it would probably better to use the nfs_fh structure in 2.2.19pre7 for the in-kernel representation and to define a new structure for userspace message passing (defined as the nfs_fh in 2.2.19pre6). But at least now we see _why_ it broke ;)

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

> On Thu, Jan 11, 2001 at 07:30:49PM +0100, Trond Myklebust > wrote:
>> OK. In that case my patch, would just be amended to eliminate >> the redundant comparison as is the case below.

> This patch looks fine w.r.t. alignment but given the below > seems called at runtime (not just at mount time) for > performance and to save a dozen of bytes of kernel stack it > would probably better to use the nfs_fh structure in 2.2.19pre7 > for the in-kernel representation and to define a new structure > for userspace message passing (defined as the nfs_fh in > 2.2.19pre6). But at least now we see _why_ it broke ;)

I'm not at all convinced that saving us a copy of 66 bytes in NLM is really worth the effort. It certainly isn't worth the ugliness of living with arrays of (void *) in order to match the alignment of a fake pointer. I'd rather we make them integers.
If we really are to change struct nfs_fh, I therefore propose that we also make struct nfs_fhbase reflect the fact that fb_dentry is a 32-bit cookie. That means that both nfs_fh and knfs_fh would be integer-aligned.

The following is untested, but should be correct.

Cheers,
Trond

diff -u --recursive --new-file linux-2.2.18/fs/lockd/svcsubs.c linux-2.2.18-fix_ppc/fs/lockd/svcsubs.c
@@ -44,6 +44,10 @@
  * Note that we open the file O_RDONLY even when creating write locks.
  * This is not quite right, but for now, we assume the client performs
  * the proper R/W checking.
+ *
+ * BEWARE:
+ * The cast to struct knfs_fh in this routine, imposes an alignment
+ * requirement on (struct nfs_fh)->data for some platforms.
  */
 u32
 nlm_lookup_file(struct svc_rqst *rqstp, struct nlm_file **result,

@@ -63,8 +67,7 @@
 	down(&nlm_file_sema);
 
 	for (file = nlm_files[hash]; file; file = file->f_next) {
-		if (file->f_handle.fh_dcookie == fh->fh_dcookie &&
-		    !memcmp(&file->f_handle, fh, sizeof(*fh)))
+		if (!memcmp(&file->f_handle, fh, sizeof(*fh)))
 			goto found;
 	}
 

diff -u --recursive --new-file linux-2.2.18/fs/nfs/inode.c linux-2.2.18-fix_ppc/fs/nfs/inode.c
@@ -273,9 +273,8 @@
 	struct nfs_server	*server;
 	struct rpc_xprt		*xprt = 0;
 	struct rpc_clnt		*clnt = 0;
-	struct nfs_fh		*root_fh = NULL,
-				*root = &data->root,
-				fh;
+	struct nfs_fh		fh,
+				*root_fh = NULL;
 	struct inode		*root_inode = NULL;
 	unsigned int		authflavor;
 	struct sockaddr_in	srvaddr;

@@ -290,7 +289,6 @@
 		goto failure;
 	}
 
-	memset(&fh, 0, sizeof(fh));
 	if (data->version != NFS_MOUNT_VERSION) {
 		printk(KERN_WARNING "nfs warning: mount version %s than kernel\n",
 			data->version < NFS_MOUNT_VERSION ? "older" : "newer");

@@ -298,12 +296,21 @@
 			data->namlen = 0;
 		if (data->version < 3)
 			data->bsize  = 0;
-		if (data->version < 4) {
+		if (data->version < 4)
 			data->flags &= ~NFS_MOUNT_VER3;
-			root = &fh;
-			root->size = NFS2_FHSIZE;
-			memcpy(root->data, data->old_root.data, NFS2_FHSIZE);
+	}
+
+	memset(&fh, 0, sizeof(fh));
+	if (data->version < 4) {
+		fh.size = NFS2_FHSIZE;
+		memcpy(fh.data, data->old_root.data, NFS2_FHSIZE);
+	} else {
+		fh.size = (data->flags & NFS_MOUNT_VER3) ? data->root.size : NFS2_FHSIZE;
+		if (fh.size > sizeof(fh.data)) {
+			printk(KERN_WARNING "NFS: mount program passes invalid filehandle!\n");
+			goto failure;
 		}
+		memcpy(fh.data, data->root.data, fh.size);
 	}
 
 	/* We now require that the mount process passes the remote address */

@@ -351,19 +358,15 @@
 	if (data->flags & NFS_MOUNT_VER3) {
 #ifdef CONFIG_NFS_V3
 		server->rpc_ops = &nfs_v3_clientops;
-		NFS_SB_FHSIZE(sb) = sizeof(unsigned short) + NFS3_FHSIZE;
+		NFS_SB_FHSIZE(sb) = NFS3_FHSIZE;
 		version = 3;
-		if (data->version < 4) {
-			printk(KERN_NOTICE "NFS: NFSv3 not supported by mount program.\n");
-			goto failure_unlock;
-		}
 #else
 		printk(KERN_NOTICE "NFS: NFSv3 not supported.\n");
 		goto failure_unlock;
 #endif
 	} else {
 		server->rpc_ops = &nfs_v2_clientops;
-		NFS_SB_FHSIZE(sb) = sizeof(unsigned short) + NFS2_FHSIZE;
+		NFS_SB_FHSIZE(sb) = NFS2_FHSIZE;
 		version = 2;
 	}
 

@@ -422,13 +425,14 @@
 	root_fh = nfs_fh_alloc();
 	if (!root_fh)
 		goto out_no_fh;
-	memcpy((u8*)root_fh, (u8*)root, sizeof(*root_fh));
+	memcpy((u8*)root_fh, (u8*)&fh, sizeof(*root_fh));
 
 	/* Did getting the root inode fail? */
-	if ((root->size > NFS_SB_FHSIZE(sb)
-	     || ! (root_inode = nfs_get_root(sb, root)))
+	if ((fh.size > NFS_SB_FHSIZE(sb)
+	     || ! (root_inode = nfs_get_root(sb, &fh)))
 	    && (data->flags & NFS_MOUNT_VER3)) {
 		data->flags &= ~NFS_MOUNT_VER3;
+		fh.size = NFS2_FHSIZE;
 		nfs_fh_free(root_fh);
 		rpciod_down();
 		rpc_shutdown_client(server->client);

@@ -445,7 +449,7 @@
 	sb->u.nfs_sb.s_root = root_fh;
 
 	/* Get some general file system info */
-	if (server->rpc_ops->statfs(server, root, &fsinfo) >= 0) {
+	if (server->rpc_ops->statfs(server, &fh, &fsinfo) >= 0) {
 		if (server->namelen == 0)
 			server->namelen = fsinfo.namelen;
 	} else {

diff -u --recursive --new-file linux-2.2.18/fs/nfsd/nfsfh.c linux-2.2.18-fix_ppc/fs/nfsd/nfsfh.c
@@ -690,7 +690,7 @@
 	fhp->fh_handle.fh_dev    = kdev_t_to_u32(parent->d_inode->i_dev);
 	fhp->fh_handle.fh_xdev   = kdev_t_to_u32(exp->ex_dev);
 	fhp->fh_handle.fh_xino   = ino_t_to_u32(exp->ex_ino);
-	fhp->fh_handle.fh_dcookie = (struct dentry *)0xfeebbaca;
+	fhp->fh_handle.fh_dcookie = 0xfeebbaca;
 	if (inode) {
 		fhp->fh_handle.fh_ino = ino_t_to_u32(inode->i_ino);
 		fhp->fh_handle.fh_generation = inode->i_generation;

diff -u --recursive --new-file linux-2.2.18/include/linux/nfs.h linux-2.2.18-fix_ppc/include/linux/nfs.h
@@ -94,8 +94,8 @@
  */
 #define NFS_MAXFHSIZE		64
 struct nfs_fh {
-	unsigned short		size;
-	unsigned char		data[NFS_MAXFHSIZE];
+	unsigned int		size;
+	unsigned int		data[NFS_MAXFHSIZE / sizeof(unsigned int)];
 };
 
 enum nfs3_stable_how {

diff -u --recursive --new-file linux-2.2.18/include/linux/nfs3.h linux-2.2.18-fix_ppc/include/linux/nfs3.h
@@ -58,6 +58,11 @@
 	NF3BAD  = 8
 };
 
+struct nfs3_fh {
+	unsigned short		size;
+	unsigned char		data[NFS3_FHSIZE];
+};
+
 #define NFS3_VERSION		3
 #define NFS3PROC_NULL		0
 #define NFS3PROC_GETATTR	1

diff -u --recursive --new-file linux-2.2.18/include/linux/nfs_mount.h linux-2.2.18-fix_ppc/include/linux/nfs_mount.h
@@ -8,9 +8,9 @@
  *
  *  structure passed from user-space to kernel-space during an nfs mount
  */
-#include <linux/nfs.h>
+#include <linux/in.h>
 #include <linux/nfs2.h>
-#include <linux/nfs_fs.h>
+#include <linux/nfs3.h>
 
 /*
  * WARNING!  Do not delete or change the order of these fields.  If

@@ -42,7 +42,7 @@
 	char		hostname[256];		/* 1 */
 	int		namlen;			/* 2 */
 	unsigned int	bsize;			/* 3 */
-	struct nfs_fh	root;			/* 4 */
+	struct nfs3_fh	root;			/* 4 */
 };
 
 /* bits in the flags field */

diff -u --recursive --new-file linux-2.2.18/include/linux/nfsd/nfsfh.h linux-2.2.18-fix_ppc/include/linux/nfsd/nfsfh.h
@@ -30,7 +30,7 @@
  * ino/dev of the exported inode.
  */
 struct nfs_fhbase {
-	struct dentry *	fb_dentry;	/* dentry cookie */
+	__u32		fb_dcookie;	/* dentry cookie */
 	__u32		fb_ino;		/* our inode number */
 	__u32		fb_dirino;	/* dir inode number */
 	__u32		fb_dev;		/* our device */

@@ -45,7 +45,7 @@
 	__u8			fh_cookie[NFS_FH_PADDING];
 };
 
-#define fh_dcookie		fh_base.fb_dentry
+#define fh_dcookie		fh_base.fb_dcookie
 #define fh_ino			fh_base.fb_ino
 #define fh_dirino		fh_base.fb_dirino
 #define fh_dev			fh_base.fb_dev

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Ok, this ties up 100% with my suggestion number (1), so I'm happy. ;)
_____

  |_____| ------------------------------------------------- ---+---+-
  |   |         Russell King        [email protected]      --- ---
  | | | | http://www.arm.linux.org.uk/personal/aboutme.html   /  /  |
  | +-+-+                                                     --- -+-
  /   |               THE developer of ARM Linux              |+| /|\
 /  | | |                                                     ---  |
    +-+-+ -------------------------------------------------  /\\\  |

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/