Messages in this thread Patches in this message | | | Date | Wed, 18 Jan 2006 11:35:54 +0100 | From | Jan Kara <> | Subject | Re: unmount oops in log_do_checkpoint |
| |
> 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) { | |