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

[Jan Kara <jack@suse.cz>]

  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