2000-12-11 17:26:00

by Ulrich Weigand

[permalink] [raw]
Subject: NFS: set_bit on an 'int' variable OK for 64-bit?



Hello,

since test11, the NFS code uses the set_bit and related routines
to manipulate the wb_flags member of the nfs_page struct (nfs_page.h).
Unfortunately, wb_flags has still data type 'int'.

This is a problem (at least) on the 64-bit S/390 architecture,
as our ..._bit macros assume bit 0 is the least significant bit
of a 'long', which means due to big-endian byte order that bit 0
resides in the 7th byte of the variable. As an int occupies only
4 bytes, however, set_bit(0, int) clobbers memory.

Now the question is, who's correct?

At all other places (I found, at least), the ..._bit macros
are indeed used only on 'long' variables (or arrays).

However, on the Alpha, the ..._bit routines assume bit 0 to
be the least significant bit of an 'int'. Sparc64 on the other
hand also uses 'long' :-/

What do you suggest we should do? Fix nfs_page to use a 'long'
variable, or change our bitops macros to use ints?


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
Dr. Ulrich Weigand
Linux for S/390 Design & Development
IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
Phone: +49-7031/16-3727 --- Email: [email protected]



2000-12-11 17:32:31

by Jes Sorensen

[permalink] [raw]
Subject: Re: NFS: set_bit on an 'int' variable OK for 64-bit?

>>>>> "Ulrich" == Ulrich Weigand <[email protected]> writes:

Ulrich> Hello,

Ulrich> since test11, the NFS code uses the set_bit and related
Ulrich> routines to manipulate the wb_flags member of the nfs_page
Ulrich> struct (nfs_page.h). Unfortunately, wb_flags has still data
Ulrich> type 'int'.

Ulrich> This is a problem (at least) on the 64-bit S/390 architecture,
Ulrich> as our ..._bit macros assume bit 0 is the least significant
Ulrich> bit of a 'long', which means due to big-endian byte order that
Ulrich> bit 0 resides in the 7th byte of the variable. As an int
Ulrich> occupies only 4 bytes, however, set_bit(0, int) clobbers
Ulrich> memory.

Ulrich> Now the question is, who's correct?

You are, the bit macros should only be used on long's. It happens to
work on little endian bitfield machines like the Alpha but thats the
same as when we had the problem with bit operations on char * in the
file system code a couple of years ago.

The NFS code needs to be fixed for this then.

Jes

2000-12-11 17:37:24

by Alan

[permalink] [raw]
Subject: Re: NFS: set_bit on an 'int' variable OK for 64-bit?

> since test11, the NFS code uses the set_bit and related routines
> to manipulate the wb_flags member of the nfs_page struct (nfs_page.h).
> Unfortunately, wb_flags has still data type 'int'.

NFS is wrong. Rusty did a complete audit of the code and I've been feeding
some stuff to Linus. That one may have been missed

> What do you suggest we should do? Fix nfs_page to use a 'long'
> variable, or change our bitops macros to use ints?

Fix NFS


2000-12-17 08:45:49

by Rusty Russell

[permalink] [raw]
Subject: Re: NFS: set_bit on an 'int' variable OK for 64-bit?

In message <[email protected]> you write:
> > since test11, the NFS code uses the set_bit and related routines
> > to manipulate the wb_flags member of the nfs_page struct (nfs_page.h).
> > Unfortunately, wb_flags has still data type 'int'.
>
> NFS is wrong. Rusty did a complete audit of the code and I've been feeding
> some stuff to Linus. That one may have been missed

Yes, didn't grep the headers. Hmm... that's the only one in
include/linux/*.h though.

> > What do you suggest we should do? Fix nfs_page to use a 'long'
> > variable, or change our bitops macros to use ints?
>
> Fix NFS

Yep, it's trivial.

Cheers,
Rusty.
--
Hacking time.

--- working-2.4.0-test12/include/linux/nfs_page.h.~1~ Thu Dec 14 14:20:28 2000
+++ working-2.4.0-test12/include/linux/nfs_page.h Fri Dec 15 16:46:09 2000
@@ -31,10 +31,10 @@
struct page *wb_page; /* page to read in/write out */
wait_queue_head_t wb_wait; /* wait queue */
unsigned long wb_timeout; /* when to read/write/commit */
+ unsigned long wb_flags; /* long req'd for set_bit */
unsigned int wb_offset, /* Offset of read/write */
wb_bytes, /* Length of request */
- wb_count, /* reference count */
- wb_flags;
+ wb_count; /* reference count */
struct nfs_writeverf wb_verf; /* Commit cookie */
};