Re: [PATCH] jbd2: adjust location of journal->j_list_lock

[Jan Kara <jack@suse.cz>]

On Wed 09-01-19 11:34:57, Jiang Ying wrote:
> From: jiangying13 <jiangying13@meituan.com>
> 
> kernel panics with kernel BUG at fs/jbd2/journal.c:2526! which is
> J_ASSERT_JH(jh, jh->b_transaction == NULL)
> 
> Locate the spinlock of journal->j_list_lock after
> J_ASSERT_JH(jh, jh->b_transaction == commit_transaction) that can ensure
> jh->b_transaction not NULL, advoiding jh->b_transaction is set NULL
> before executing __jbd2_journal_remove_checkpoint.
> 
> The bug is not easy to reproduce, the call trace is as following:
> 
>  Call Trace:
>   [<ffffffffc02b7e7b>] __jbd2_journal_remove_checkpoint+0x5b/0x160 [jbd2]
>   [<ffffffffc02b616e>] jbd2_journal_commit_transaction+0x10be/0x1950 [jbd2]
>   [<ffffffff81029557>] ? __switch_to+0xd7/0x510
>   [<ffffffffc02bba99>] kjournald2+0xc9/0x260 [jbd2]
>   [<ffffffff810b1930>] ? wake_up_atomic_t+0x30/0x30
>   [<ffffffffc02bb9d0>] ? commit_timeout+0x10/0x10 [jbd2]
>   [<ffffffff810b09af>] kthread+0xcf/0xe0
>   [<ffffffff810b08e0>] ? insert_kthread_work+0x40/0x40
>   [<ffffffff816ba358>] ret_from_fork+0x58/0x90
>   [<ffffffff810b08e0>] ? insert_kthread_work+0x40/0x40
> 
> Signed-off-by: jiangying13 <jiangying13@meituan.com>

Hum, why do you think the patch below changes anything for the assertion
failure you mention above? The code that gets additionally covered by
j_list_lock is just handling of journal head frozen & b_committed_data
buffers...

With which kernel version did you see the assertion failure?

								Honza

> ---
>  fs/jbd2/commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 2eb55c3..19aa2b0 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -930,6 +930,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		 * We also know that the frozen data has already fired
>  		 * its triggers if they exist, so we can clear that too.
>  		 */
> +		spin_lock(&journal->j_list_lock);
>  		if (jh->b_committed_data) {
>  			jbd2_free(jh->b_committed_data, bh->b_size);
>  			jh->b_committed_data = NULL;
> @@ -944,7 +945,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  			jh->b_frozen_triggers = NULL;
>  		}
>  
> -		spin_lock(&journal->j_list_lock);
>  		cp_transaction = jh->b_cp_transaction;
>  		if (cp_transaction) {
>  			JBUFFER_TRACE(jh, "remove from old cp transaction");
> -- 
> 1.8.3.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR