linux-next: next-20090609 hangs in early user mode

linux-next: next-20090609 hangs in early user mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

Hi all,

My boot tests last night produced this during boot:

Loading, please wait...
exe used greatest stack depth: 11120 bytes left
async/0 used greatest stack depth: 10928 bytes left
-- 0:conmux-control -- time-stamp -- Jun/09/09 21:02:12 --
INFO: task init:1 blocked for more than 120 seconds.
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
init          D 0000000010078228  8192     1      0 0x00008010
Call Trace:
[c0000000be67f380] [c0000000be67f430] 0xc0000000be67f430 (unreliable)
[c0000000be67f550] [c00000000001153c] .__switch_to+0xfc/0x1b0
[c0000000be67f5e0] [c00000000055dc70] .schedule+0x2b0/0x980
[c0000000be67f700] [c000000000138850] .inode_wait+0x10/0x30
[c0000000be67f780] [c00000000055ebd8] .__wait_on_bit+0xd8/0x160
[c0000000be67f830] [c00000000055ece8] .out_of_line_wait_on_bit+0x88/0xb0
[c0000000be67f910] [c000000000139060] .clear_inode+0x120/0x150
[c0000000be67f990] [c0000000001395c8] .generic_drop_inode+0x198/0x330
[c0000000be67fa20] [c0000000001388f8] .iput+0x88/0xc0
[c0000000be67faa0] [c000000000134308] .dentry_iput+0x108/0x220
[c0000000be67fb30] [c0000000001345c8] .d_kill+0x68/0xb0
[c0000000be67fbc0] [c000000000135104] .dput+0x154/0x2c0
[c0000000be67fc60] [c00000000011f9a4] .__fput+0x1b4/0x290
[c0000000be67fd00] [c00000000011b760] .filp_close+0xa0/0x100
[c0000000be67fd90] [c00000000011b870] .SyS_close+0xb0/0x180
[c0000000be67fe30] [c0000000000084ac] syscall_exit+0x0/0x40

And it never recovers.  Sometimes during the bisection run, it was a
different process - but it was the same backtrace.

Bisection lead me to this:

d4fdcb2068eef29c03d6027aa219fa60171c6b87 is first bad commit
commit d4fdcb2068eef29c03d6027aa219fa60171c6b87
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu May 21 16:00:59 2009 -0400

    fs: i_flags and i_state in struct inode only need to be unsigned short
    
    Currently i_flags and i_state do not need to be an unsigned int and an
    unsigned long, respectively.  (We currently use 9 i_flags bits, and 8
    i_state bits.)  Changing them to be an unsigned short saves 4 bytes
    per inode on an x86 platform, and 8 bytes on an x86_64 platform.
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
    Cc: linux-fsdevel@vger.kernel.org
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Reverting that commit from next-20090609 gets me a bootable kernel.

The machines are PowerPC64 (so big endian).  It looks to me like we use
bitops on i_state - don't our bitops only apply to unsigned longs?
-- 
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: next-20090609 hangs in early user mode

[Christoph Hellwig]

On Wed, Jun 10, 2009 at 06:00:58PM +1000, Stephen Rothwell wrote:
> The machines are PowerPC64 (so big endian).  It looks to me like we use
> bitops on i_state - don't our bitops only apply to unsigned longs?

Yes, bitops need unsigned long.  And on most plaforms should give you
warnings when using them on the wrong type.

Re: linux-next: next-20090609 hangs in early user mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Hi Christoph,

On Wed, 10 Jun 2009 04:31:39 -0400 Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Jun 10, 2009 at 06:00:58PM +1000, Stephen Rothwell wrote:
> > The machines are PowerPC64 (so big endian).  It looks to me like we use
> > bitops on i_state - don't our bitops only apply to unsigned longs?
> 
> Yes, bitops need unsigned long.  And on most plaforms should give you
> warnings when using them on the wrong type.

In this case the "word" is being passed through wait_on_bit which takes a
"void *", so we lose the type information.
-- 
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: next-20090609 hangs in early user mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Wed, 10 Jun 2009 18:00:58 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> commit d4fdcb2068eef29c03d6027aa219fa60171c6b87
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu May 21 16:00:59 2009 -0400
> 
>     fs: i_flags and i_state in struct inode only need to be unsigned short

I will revert that commit from linux-next today.

-- 
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: next-20090609 hangs in early user mode

[Al Viro]

On Wed, Jun 10, 2009 at 06:00:58PM +1000, Stephen Rothwell wrote:

> d4fdcb2068eef29c03d6027aa219fa60171c6b87 is first bad commit
> commit d4fdcb2068eef29c03d6027aa219fa60171c6b87
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu May 21 16:00:59 2009 -0400
> 
>     fs: i_flags and i_state in struct inode only need to be unsigned short
>     
>     Currently i_flags and i_state do not need to be an unsigned int and an
>     unsigned long, respectively.  (We currently use 9 i_flags bits, and 8
>     i_state bits.)  Changing them to be an unsigned short saves 4 bytes
>     per inode on an x86 platform, and 8 bytes on an x86_64 platform.
>     
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>     Cc: linux-fsdevel@vger.kernel.org
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

*gyah*

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.

Re: linux-next: next-20090609 hangs in early user mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

Hi Al,

On Wed, 10 Jun 2009 14:00:54 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
> On Wed, Jun 10, 2009 at 06:00:58PM +1000, Stephen Rothwell wrote:
> 
> > d4fdcb2068eef29c03d6027aa219fa60171c6b87 is first bad commit
> > commit d4fdcb2068eef29c03d6027aa219fa60171c6b87
> > Author: Theodore Ts'o <tytso@mit.edu>
> > Date:   Thu May 21 16:00:59 2009 -0400
> > 
> >     fs: i_flags and i_state in struct inode only need to be unsigned short
> >     
> >     Currently i_flags and i_state do not need to be an unsigned int and an
> >     unsigned long, respectively.  (We currently use 9 i_flags bits, and 8
> >     i_state bits.)  Changing them to be an unsigned short saves 4 bytes
> >     per inode on an x86 platform, and 8 bytes on an x86_64 platform.
> >     
> >     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> >     Cc: linux-fsdevel@vger.kernel.org
> >     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> *gyah*
> 
> 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.

OK, thanks.

-- 
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: next-20090609 hangs in early user mode

[Theodore Tso]

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.

       	      	       		     - Ted

Re: linux-next: next-20090609 hangs in early user mode

[Al Viro]

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

Re: linux-next: next-20090609 hangs in early user mode

[Theodore Tso]

On Wed, Jun 10, 2009 at 04:08:39PM +0100, Al Viro wrote:
> 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...

Yelch....  good point.  I'll look and see if there's some other 8 or
16-bit type to combine it with, but we may have started to hit
diminishing returns with this this approach to sliming the inode slab
caches.  I'm beginning to think if I want to make the inodes smaller,
I'm going to have to create a separate substructure for fields only
used when a file descriptor is opened on that inode, both in struct
inode and in struct ext4_inode_info.  (Lifetime management of the
substructure is going to be non-trivial, though.)

					- Ted