Re: unmount oops in log_do_checkpoint

[Jan Kara <jack@suse.cz>]

> On Tue, Jan 17, 2006 at 11:23:53PM +0100, Jan Kara wrote:
> > > On Tue, Jan 17, 2006 at 05:32:35PM +0100, Jan Kara wrote:
> > > > > On Tue, Jan 17, 2006 at 12:59:45PM +0100, Nick Piggin wrote:
> > > > > 
> > > > > Maybe it is because people haven't been turning on their debugging options,
> > > > > tsk tsk ;) It only oopses when DEBUG_SLAB and DEBUG_PAGEALLOC are both
> > > > > enabled. And only then when the jbd patch is not reverted. Weird.
> > > >   Hmm, that's really strange, maybe we have some use-after-free
> > > > problem or so... I'll see what I can do :).
> > > > 
> > > 
> > > Are you able to reproduce? If not I can test patches...
> >   Hmm, I was not able to reproduce the problem even with those debug
> > options set :(. As I'm looking into the code it seems that somebody
> > managed to free the transaction but did not clear the
> > j_checkpoint_transactions pointer. It's even stranger that it's during
> > umount time when there should not be much processes playing with the JBD
> > structures on that filesystem.
> >   Attached is the patch that fixes two minor possible problems I've
> > found. Neither of them should be causing your oops but one never knows
> > :). Also turn on the JBD debugging in config. Maybe it spits something
> > useful. If you still see the same oops, I'll create some debugging
> > patch.
> 
> This patch does the trick. Survived several reboots now while without
> the patch it has oopsed 100% of the time so far. Thanks!
  Good to hear :).

> I have also attached a full jbd debug output and oops for the vanilla
> 2.6.16-rc1 case, just in case that helps.
  It helped me to verify my idea why my patch helped. Thanks. The problem was
the || instead of && in log_do_checkpoint(). When the journal checkpoint
list became empty, the pointer check in the loop failed and so because
of || we tried also the second check which was using transaction->t_tid.
If the transaction was already freed, bad luck...
  Andrew, attached is the fix with logs etc. I split my original patch
into two as they are in fact unrelated things. Please apply.

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

While checkpointing we have to check that our transaction still is in the
checkpoint list *and* (not or) that it's not just a different transaction
with the same address.

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.16-rc1/fs/jbd/checkpoint.c linux-2.6.16-rc1-1-checkpoint-fix/fs/jbd/checkpoint.c
--- linux-2.6.16-rc1/fs/jbd/checkpoint.c	2006-01-17 21:44:02.000000000 +0100
+++ linux-2.6.16-rc1-1-checkpoint-fix/fs/jbd/checkpoint.c	2006-01-17 23:35:49.000000000 +0100
@@ -338,7 +338,7 @@ restart:
 	 * done (maybe it's a new transaction, but it fell at the same
 	 * address).
 	 */
- 	if (journal->j_checkpoint_transactions == transaction ||
+ 	if (journal->j_checkpoint_transactions == transaction &&
 			transaction->t_tid == this_tid) {
 		int batch_count = 0;
 		struct buffer_head *bhs[NR_BATCH];

We have to check that also the second checkpoint list is non-empty before
dropping the transaction.

Signed-off-by: Jan Kara <jack@suse.cz>

diff -rupX /home/jack/.kerndiffexclude linux-2.6.16-rc1/fs/jbd/commit.c linux-2.6.16-rc1-1-checkpoint-fix/fs/jbd/commit.c
--- linux-2.6.16-rc1/fs/jbd/commit.c	2006-01-15 00:20:12.000000000 +0100
+++ linux-2.6.16-rc1-1-checkpoint-fix/fs/jbd/commit.c	2006-01-17 23:35:19.000000000 +0100
@@ -829,7 +829,8 @@ restart_loop:
 	journal->j_committing_transaction = NULL;
 	spin_unlock(&journal->j_state_lock);
 
-	if (commit_transaction->t_checkpoint_list == NULL) {
+	if (commit_transaction->t_checkpoint_list == NULL &&
+	    commit_transaction->t_checkpoint_io_list == NULL) {
 		__journal_drop_transaction(journal, commit_transaction);
 	} else {
 		if (journal->j_checkpoint_transactions == NULL) {