diff mbox

general protection fault: from release_blocks_on_commit

Message ID 49064027.9010509@redhat.com
State Superseded, archived
Headers show

Commit Message

Eric Sandeen Oct. 27, 2008, 10:26 p.m. UTC
Theodore Tso wrote:

> On Tue, Oct 21, 2008 at 02:03:01PM -0400, Eric Paris wrote:
>   
>> I can consistently get the below backtrace any time I try to shutdown my
>> machine.  This machine has ext4 as it's root FS.  This is 100%
>> reproducible.  I backed out commit
>> 3e624fc72fba09b6f999a9fbb87b64efccd38036 and it fixed the problem.
>>
>> This is a regression.
>>     
>
> Can you send me your .config, please?  I'm trying to duplicate it on
> my end.  
>
> 						- Ted
>   
Ted, you probably need some slab debugging on to hit it.

I think the problem is that jbd2_journal_commit_transaction may call
__jbd2_journal_drop_transaction(journal, commit_transaction) if the
checkpoint lists are NULL, and this frees the commit_transaction.

However, the call to ->j_commit_callback() tries to use it after that.

I'm out of time for now to be sure this is the right fix, but something
like this perhaps?


-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Theodore Y. Ts'o Oct. 27, 2008, 11:28 p.m. UTC | #1
On Mon, Oct 27, 2008 at 05:26:47PM -0500, Eric Sandeen wrote:
> Ted, you probably need some slab debugging on to hit it.

I had slab debugging enabled, but haven't been able to replicate it
yet.  I'll do some more work to try to replicate it.

> I think the problem is that jbd2_journal_commit_transaction may call
> __jbd2_journal_drop_transaction(journal, commit_transaction) if the
> checkpoint lists are NULL, and this frees the commit_transaction.

I think you're right.  I would probably change the patch around so
that after calling __jbd2_jurnal_drop_transaction(), we set
commit_transaction to NULL, and then adding an "if
(commit_transaction)" to the lines in questions; that way we keep the
commit callback outside of the j_list_lock() spinlock.

> Also, I'm not certain that it matters, but the loop in 
> release_blocks_on_commit() is kfreeing list entries w/o taking
> them off the list; I suppose maybe this is safe if the whole thing
> is getting discarded when we're done, but just to keep things sane,
> would this make sense

There are plenty of other loops in the kernel where we go through the
linked list and free all of the items on the list that don't bother to
call list_del().  That was one of the things I checked when I created
the patch.

> (also, I think we need to double-check use of
> s_md_lock; it's taken when adding things to the list, but not when
> freeing/removing ... if it's needed, isn't it needed on both ends...):

No, because the linked list is hanging off the transaction structure.
While the transaction is active, multiple CPU's can be adding elements
to the linked list.  But once the transaction has been committed, we
don't have to worry about any one else trying to modify the linked list.

      	      	    	      	       	      - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen Oct. 28, 2008, 12:08 a.m. UTC | #2
Theodore Tso wrote:
> On Mon, Oct 27, 2008 at 05:26:47PM -0500, Eric Sandeen wrote:
>> Ted, you probably need some slab debugging on to hit it.
> 
> I had slab debugging enabled, but haven't been able to replicate it
> yet.  I'll do some more work to try to replicate it.
> 
>> I think the problem is that jbd2_journal_commit_transaction may call
>> __jbd2_journal_drop_transaction(journal, commit_transaction) if the
>> checkpoint lists are NULL, and this frees the commit_transaction.
> 
> I think you're right.  I would probably change the patch around so
> that after calling __jbd2_jurnal_drop_transaction(), we set
> commit_transaction to NULL, and then adding an "if
> (commit_transaction)" to the lines in questions; that way we keep the
> commit callback outside of the j_list_lock() spinlock.

Ok, I thought about that; sounds good.  will resend.

>> Also, I'm not certain that it matters, but the loop in 
>> release_blocks_on_commit() is kfreeing list entries w/o taking
>> them off the list; I suppose maybe this is safe if the whole thing
>> is getting discarded when we're done, but just to keep things sane,
>> would this make sense
> 
> There are plenty of other loops in the kernel where we go through the
> linked list and free all of the items on the list that don't bother to
> call list_del().  That was one of the things I checked when I created
> the patch.

Ok.

>> (also, I think we need to double-check use of
>> s_md_lock; it's taken when adding things to the list, but not when
>> freeing/removing ... if it's needed, isn't it needed on both ends...):
> 
> No, because the linked list is hanging off the transaction structure.
> While the transaction is active, multiple CPU's can be adding elements
> to the linked list.  But once the transaction has been committed, we
> don't have to worry about any one else trying to modify the linked list.
> 
>       	      	    	      	       	      - Ted

ok, just double checking.

Thanks,
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: linux-2.6/fs/jbd2/commit.c
===================================================================
--- linux-2.6.orig/fs/jbd2/commit.c	2008-10-27 11:24:42.000000000 -0500
+++ linux-2.6/fs/jbd2/commit.c	2008-10-27 17:19:22.771063324 -0500
@@ -992,15 +992,15 @@  restart_loop:
 			commit_transaction->t_cpprev->t_cpnext =
 				commit_transaction;
 		}
+		if (journal->j_commit_callback)
+			journal->j_commit_callback(journal, commit_transaction);
+
+		trace_mark(jbd2_end_commit, "dev %s transaction %d head %d",
+			   journal->j_devname, commit_transaction->t_tid,
+			   journal->j_tail_sequence);
 	}
 	spin_unlock(&journal->j_list_lock);
 
-	if (journal->j_commit_callback)
-		journal->j_commit_callback(journal, commit_transaction);
-
-	trace_mark(jbd2_end_commit, "dev %s transaction %d head %d",
-		   journal->j_devname, commit_transaction->t_tid,
-		   journal->j_tail_sequence);
 	jbd_debug(1, "JBD: commit %d complete, head %d\n",
 		  journal->j_commit_sequence, journal->j_tail_sequence);
 

Also, I'm not certain that it matters, but the loop in 
release_blocks_on_commit() is kfreeing list entries w/o taking
them off the list; I suppose maybe this is safe if the whole thing
is getting discarded when we're done, but just to keep things sane,
would this make sense (also, I think we need to double-check use of
s_md_lock; it's taken when adding things to the list, but not when
freeing/removing ... if it's needed, isn't it needed on both ends...):


Index: linux-2.6/fs/ext4/mballoc.c
===================================================================
--- linux-2.6.orig/fs/ext4/mballoc.c	2008-10-27 11:24:41.000000000 -0500
+++ linux-2.6/fs/ext4/mballoc.c	2008-10-27 17:19:43.401064490 -0500
@@ -2644,6 +2644,7 @@  static void release_blocks_on_commit(jou
 	struct super_block *sb = journal->j_private;
 	struct ext4_buddy e4b;
 	struct ext4_group_info *db;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	int err, count = 0, count2 = 0;
 	struct ext4_free_data *entry;
 	ext4_fsblk_t discard_block;
@@ -2683,6 +2684,9 @@  static void release_blocks_on_commit(jou
 			   (unsigned long long) discard_block, entry->count);
 		sb_issue_discard(sb, discard_block, entry->count);
 
+		spin_lock(&sbi->s_md_lock);
+		list_del(&entry->list);
+		spin_unlock(&sbi->s_md_lock);
 		kmem_cache_free(ext4_free_ext_cachep, entry);
 		ext4_mb_release_desc(&e4b);
 	}