Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[EUNBONG SONG]

> CONFIG_IA64_PAGE_SIZE_64KB=y

> fsblock size is whatever is the default for SLES11SP2 on ia64 - which
> tool will tell me?

> My git bisect finally competed and points the a finger at:

> bisect> git bisect good
> ae4647fb7654676fc44a97e86eb35f9f06b99f66 is first bad commit
> commit ae4647fb7654676fc44a97e86eb35f9f06b99f66
> Author: Jan Kara 
> Date:   Fri Apr 12 00:03:42 2013 -0400

>     jbd2: reduce journal_head size

>     Remove unused t_cow_tid field (ext4 copy-on-write support doesn't seem
>     to be happening) and change b_modified and b_jlist to bitfields thus
>     saving 8 bytes in the structure.

>     Signed-off-by: Jan Kara 
>     Signed-off-by: "Theodore Ts'o" 
>     Reviewed-by: Zheng Liu 

> :040000 040000 c39ece4341894b3daf84764ba425a87ffb90fe50
> d4e8d9185c2a1b740c235ca8ed05d496a442fce3 M      include

Hi, my git bisect result is same yours. And i reported that to community yesterday.
Thanks. 

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Tony Luck]

On Sun, May 12, 2013 at 7:21 PM, EUNBONG SONG <eunb.song@samsung.com> wrote:
> Hi, my git bisect result is same yours. And i reported that to community yesterday.

Ah. Good to have some confirmation (I was never sure how long to keep
running before deciding that a test was "good".  My slowest "bad" test took
about 2.5 hours.  I mostly let the tests run for >6 hours before deciding.

I just confirmed that 3.10-rc1 still fails (30 minutes).  Now running a test
on 3.10-rc1 with just this commit reverted. Only been going for about
15 minutes, so no useful information yet.

My best guess as to why this commit causes problems is that there are places
where updates to individual fields in this structure used to be independent
because they were to whole words.  Now we have bitfileds there are races
between access to different fields in the same word.

-Tony

Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Theodore Ts'o]

On Sun, May 12, 2013 at 08:11:59PM -0700, Tony Luck wrote:
> 
> My best guess as to why this commit causes problems is that there are places
> where updates to individual fields in this structure used to be independent
> because they were to whole words.  Now we have bitfileds there are races
> between access to different fields in the same word.

Yeah, except we access the fields while holding a lock.... wait a
minute.  We're using bit_spinlocks().... and am I missing something?

Where are the barrier statements to prevent the CPU or the compiler
from reordering statements around bit_spin_lock()?  But if that's the
problem, I would have expected lots of other things to be broken.

	   	      	       	       	     - Ted

Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Mike Galbraith]

On Sun, 2013-05-12 at 23:36 -0400, Theodore Ts'o wrote: 
> On Sun, May 12, 2013 at 08:11:59PM -0700, Tony Luck wrote:
> > 
> > My best guess as to why this commit causes problems is that there are places
> > where updates to individual fields in this structure used to be independent
> > because they were to whole words.  Now we have bitfileds there are races
> > between access to different fields in the same word.
> 
> Yeah, except we access the fields while holding a lock.... wait a
> minute.  We're using bit_spinlocks().... and am I missing something?
> 
> Where are the barrier statements to prevent the CPU or the compiler
> from reordering statements around bit_spin_lock()?  But if that's the
> problem, I would have expected lots of other things to be broken.

Those use test_and_set_bit(), which per Paul McMemory-Wizard...

ATOMIC OPERATIONS
-----------------

Whilst they are technically interprocessor interaction considerations, atomic
operations are noted specially as some of them imply full memory barriers and
some don't, but they're very heavily relied on as a group throughout the
kernel.

Any atomic operation that modifies some state in memory and returns information
about the state (old or new) implies an SMP-conditional general memory barrier
(smp_mb()) on each side of the actual operation (with the exception of
explicit lock operations, described later).  These include:

        xchg();
        cmpxchg();
        atomic_xchg();
        atomic_cmpxchg();
        atomic_inc_return();
        atomic_dec_return();
        atomic_add_return();
        atomic_sub_return();
        atomic_inc_and_test();
        atomic_dec_and_test();
        atomic_sub_and_test();
        atomic_add_negative();
        atomic_add_unless();    /* when succeeds (returns 1) */
        test_and_set_bit();
        test_and_clear_bit();
        test_and_change_bit();

Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Tony Luck]

The 3.10-rc1 with ae4647fb765467 reverted is still running OK. At
3 hours now (only marginally longer that the 2.5 hours that one of
the "bad" runs during the bisect managed).  So I'm about 30% sure
that we have a winner at the moment. I'll leave it running and check
again in the morning. This penguin is heading to bed now.

-Tony

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[EUNBONG SONG]

> Hi all,

> First of all I couldn't reproduce this regression in my sand box.  So
> the following speculation is only my guess.  I suspect that the commit
> (ae4647fb) isn't root cause.  It just uncover a potential bug that has
> been there for a long time.  I look at the code, and found two
> suspicious stuff in jbd2.  The first one is in do_get_write_access().
> In this function we forgot to lock bh state when we check b_jlist ==
> BJ_Shadow.  I generate a patch to fix it, and I really think it is the
> root cause.  Further, in __journal_remove_journal_head() we check
> b_jlist == BJ_None.  But, when this function is called, bh state won't
> be locked sometimes.  So I suspect this is why we hit a BUG in
> jbd2_journal_put_journal_head().  But I don't have a good solution to
> fix this until now because I don't know whether we need to lock bh state
> here, or maybe we should remove this assertation.
>
> So, generally, Tony, Eunbong, could you please try the following patch?
>
> Thanks in advance,
>                                                 - Zheng


Hi, I tested your patch. Unfortunately, the same problem was reproduced.
Thanks.

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Zheng Liu]

On Mon, May 13, 2013 at 09:53:25AM +0000, EUNBONG SONG wrote:
> 
> 
> > Hi all,
> 
> > First of all I couldn't reproduce this regression in my sand box.  So
> > the following speculation is only my guess.  I suspect that the commit
> > (ae4647fb) isn't root cause.  It just uncover a potential bug that has
> > been there for a long time.  I look at the code, and found two
> > suspicious stuff in jbd2.  The first one is in do_get_write_access().
> > In this function we forgot to lock bh state when we check b_jlist ==
> > BJ_Shadow.  I generate a patch to fix it, and I really think it is the
> > root cause.  Further, in __journal_remove_journal_head() we check
> > b_jlist == BJ_None.  But, when this function is called, bh state won't
> > be locked sometimes.  So I suspect this is why we hit a BUG in
> > jbd2_journal_put_journal_head().  But I don't have a good solution to
> > fix this until now because I don't know whether we need to lock bh state
> > here, or maybe we should remove this assertation.
> >
> > So, generally, Tony, Eunbong, could you please try the following patch?
> >
> > Thanks in advance,
> >                                                 - Zheng
> 
> 
> Hi, I tested your patch. Unfortunately, the same problem was reproduced.
> Thanks.

Thanks for trying this patch.  Could you please repost the dmesg log for
me?  I want to make sure whether the second suspicious stuff causes this
regression or not.  Further, that would be great if you could try to
comment this line as the following?

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 886ec2f..a9e3779 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2453,7 +2453,7 @@ static void __journal_remove_journal_head(struct
buffer_head *bh)
        J_ASSERT_JH(jh, jh->b_transaction == NULL);
        J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
        J_ASSERT_JH(jh, jh->b_cp_transaction == NULL);
-       J_ASSERT_JH(jh, jh->b_jlist == BJ_None);
+       /*J_ASSERT_JH(jh, jh->b_jlist == BJ_None);*/
        J_ASSERT_BH(bh, buffer_jbd(bh));
        J_ASSERT_BH(bh, jh2bh(jh) == bh);
        BUFFER_TRACE(bh, "remove journal_head");

Really thanks,
                                                - Zheng

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Dmitry Monakhov]

On Mon, 13 May 2013 19:26:34 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> On Mon, May 13, 2013 at 09:53:25AM +0000, EUNBONG SONG wrote:
> > 
> > 
> > > Hi all,
> > 
> > > First of all I couldn't reproduce this regression in my sand box.  So
> > > the following speculation is only my guess.  I suspect that the commit
> > > (ae4647fb) isn't root cause.  It just uncover a potential bug that has
> > > been there for a long time.  I look at the code, and found two
> > > suspicious stuff in jbd2.  The first one is in do_get_write_access().
> > > In this function we forgot to lock bh state when we check b_jlist ==
> > > BJ_Shadow.  I generate a patch to fix it, and I really think it is the
> > > root cause.  Further, in __journal_remove_journal_head() we check
> > > b_jlist == BJ_None.  But, when this function is called, bh state won't
> > > be locked sometimes.  So I suspect this is why we hit a BUG in
> > > jbd2_journal_put_journal_head().  But I don't have a good solution to
> > > fix this until now because I don't know whether we need to lock bh state
> > > here, or maybe we should remove this assertation.
> > >
> > > So, generally, Tony, Eunbong, could you please try the following patch?
> > >
> > > Thanks in advance,
> > >                                                 - Zheng
> > 
> > 
> > Hi, I tested your patch. Unfortunately, the same problem was reproduced.
> > Thanks.
> 
> Thanks for trying this patch.  Could you please repost the dmesg log for
> me?  I want to make sure whether the second suspicious stuff causes this
> regression or not.  Further, that would be great if you could try to
> comment this line as the following?
AFAIK  following assertion was triggered jh->b_transaction != NULL
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 886ec2f..a9e3779 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2453,7 +2453,7 @@ static void __journal_remove_journal_head(struct
> buffer_head *bh)
>         J_ASSERT_JH(jh, jh->b_transaction == NULL);
>         J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
>         J_ASSERT_JH(jh, jh->b_cp_transaction == NULL);
> -       J_ASSERT_JH(jh, jh->b_jlist == BJ_None);
> +       /*J_ASSERT_JH(jh, jh->b_jlist == BJ_None);*/
>         J_ASSERT_BH(bh, buffer_jbd(bh));
>         J_ASSERT_BH(bh, jh2bh(jh) == bh);
>         BUFFER_TRACE(bh, "remove journal_head");
> 
> Really thanks,
>                                                 - Zheng
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: Re: EXT4 panic at jbd2_journal_put_journal_head() in 3.9+

[Eunbong Song]

Hi, I just wonder. Is there no problem with endianess.
I mean usually bit field is defined with __BIG_ENDIAN_BITFIELD or
__LITTLE_ENDIAN_BITFIELD. But b_jlist and b_modfied is defined with no
pad.
It seems to be good but i just want to make sure.


Thanks.

2013/5/13 Dmitry Monakhov <dmonakhov@openvz.org>:
> On Mon, 13 May 2013 19:26:34 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
>> On Mon, May 13, 2013 at 09:53:25AM +0000, EUNBONG SONG wrote:
>> >
>> >
>> > > Hi all,
>> >
>> > > First of all I couldn't reproduce this regression in my sand box.  So
>> > > the following speculation is only my guess.  I suspect that the commit
>> > > (ae4647fb) isn't root cause.  It just uncover a potential bug that has
>> > > been there for a long time.  I look at the code, and found two
>> > > suspicious stuff in jbd2.  The first one is in do_get_write_access().
>> > > In this function we forgot to lock bh state when we check b_jlist ==
>> > > BJ_Shadow.  I generate a patch to fix it, and I really think it is the
>> > > root cause.  Further, in __journal_remove_journal_head() we check
>> > > b_jlist == BJ_None.  But, when this function is called, bh state won't
>> > > be locked sometimes.  So I suspect this is why we hit a BUG in
>> > > jbd2_journal_put_journal_head().  But I don't have a good solution to
>> > > fix this until now because I don't know whether we need to lock bh state
>> > > here, or maybe we should remove this assertation.
>> > >
>> > > So, generally, Tony, Eunbong, could you please try the following patch?
>> > >
>> > > Thanks in advance,
>> > >                                                 - Zheng
>> >
>> >
>> > Hi, I tested your patch. Unfortunately, the same problem was reproduced.
>> > Thanks.
>>
>> Thanks for trying this patch.  Could you please repost the dmesg log for
>> me?  I want to make sure whether the second suspicious stuff causes this
>> regression or not.  Further, that would be great if you could try to
>> comment this line as the following?
> AFAIK  following assertion was triggered jh->b_transaction != NULL
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 886ec2f..a9e3779 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -2453,7 +2453,7 @@ static void __journal_remove_journal_head(struct
>> buffer_head *bh)
>>         J_ASSERT_JH(jh, jh->b_transaction == NULL);
>>         J_ASSERT_JH(jh, jh->b_next_transaction == NULL);
>>         J_ASSERT_JH(jh, jh->b_cp_transaction == NULL);
>> -       J_ASSERT_JH(jh, jh->b_jlist == BJ_None);
>> +       /*J_ASSERT_JH(jh, jh->b_jlist == BJ_None);*/
>>         J_ASSERT_BH(bh, buffer_jbd(bh));
>>         J_ASSERT_BH(bh, jh2bh(jh) == bh);
>>         BUFFER_TRACE(bh, "remove journal_head");
>>
>> Really thanks,
>>                                                 - Zheng
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/