On Wed, Jun 10, 2009 at 09:19:09AM -0400, Theodore Tso wrote: > On Wed, Jun 10, 2009 at 02:00:54PM +0100, Al Viro wrote: > > > > Yes, it's obviously bogus. Dropped from the tree; I don't think it's > > really salvagable - even merging into one unsigned long will not be > > enough, since we will end up with different locking for different bits. > > Oops, sorry, I didn't realize we were using bitops for i_state. As > far as I can tell we're not using the bitops functions for i_flags, > though. Is that right? So we can convert i_flags to be a unsigned > short, but we can't do anything with i_state. We can, but... it's again a matter of combining things with different locking. i_flags is protected by i_mutex, so if you put another unsigned short next to it, you'd better make sure that i_mutex is necessary and sufficient for modifying it. Depending on the target, gcc may turn 16bit read-modify-store into 32bit one, so if you have two 16bit fields next to each other, you can run into CPU1: CPU2: r1 = *(u32 *)p; r2 = *(u32 *)p; r1 |= 1; r2 |= 1 << 16; *(u32 *)p = r1; *(u32 *)p = r2; with obvious results. So we need the same locking for both such fields... -- To unsubscribe from this list: send the line "unsubscribe linux-next" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
