Re: [PATCH] Change ll_rw_block() calls in JBD

Jan Kara
2006-06-21T00:09:07Z

Hi,

> I have got some crashes due to:
>
> Assertion failure in __journal_file_buffer():
> "jh->b_transaction == transaction || jh->b_transaction == 0"
Yes, I've seen your reports. Thanks for them and sorry for being a bit
unresponsive (I have a lot of other work...).

<snip>
>
> --- Called from --- :
>
> journal_submit_data_buffers+0x200/0x660 [jbd]
> r32 : e0000001035ec100 journal
> r33 : e00000010396a380 commit_transaction
>
> As you can see, the current "jh" has been stolen for the new
> "->j_running_transaction" while we released temporarily "->j_list_lock"
> in the middle of "journal_submit_data_buffers()".
Yes, this seems to be correct analysis.

> Therefore the test "jh->b_jlist != BJ_SyncData", i.e. if it is still
> on a (_any_) sync. list is not enough.
Right.

> --- linux-2.6.16.20-orig/fs/jbd/commit.c 2006-06-20
> 17:19:47.000000000 +0200
> +++ linux-2.6.16.20/fs/jbd/commit.c 2006-06-20 17:35:54.000000000 +0200
> @@ -219,15 +219,26 @@
> bufs = 0;
> lock_buffer(bh);
> spin_lock(&journal->j_list_lock);
> + /* Stolen (e.g. for a new transaction) ? */
> + if (jh !=
> commit_transaction->t_sync_datalist) {
> + unlock_buffer(bh);
> + JBUFFER_TRACE(jh, "stolen sync.
> data");
> + put_bh(bh);
> + continue;
> + }
Yes, this is definitely safer check and should also catch the case
when jh was released from memory so buffer_jbd() is not needed any more.

> /* Someone already cleaned up the buffer? */
> - if (!buffer_jbd(bh)
> - || jh->b_jlist != BJ_SyncData) {
> +
> + // Can this happen???
> +
> + if (!buffer_jbd(bh)) {
> unlock_buffer(bh);
> BUFFER_TRACE(bh, "already cleaned
> up");
> put_bh(bh);
> continue;
> }
> put_bh(bh);
> + J_ASSERT_JH(jh, jh->b_transaction ==
> + commit_transaction);
> }
> if (test_clear_buffer_dirty(bh)) {
> BUFFER_TRACE(bh, "needs writeout,
> submitting");
>
> I am not really sure that the test "!buffer_jbd(bh)" is really useful.
> I left it alone for not introducing a new bug.
> If you can confirm that it is not necessary, I can take it away.
BTW: I've also written another version of the patch using a bit
different approach. When I have some time I can benchmark them against
each other to see if there is some difference...

Honza
--
Jan Kara <jack@suse.cz>
SuSE CR Labs