general protection fault: from release_blocks_on_commit

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.

Quoted 5 lines · Eric Paris

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.

Quoted 2 lines

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.

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...):

Quoted 1 line · Eric Sandeen

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

(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

Quoted 10 lines · Theodore Tso

Quoted 1 line · Eric Sandeen

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

I looked at this some more, and at least in theory it could happen that we could not have any buffers that need to be checkpointed (so t_checkpoint_list and t_checkpoint_io_list are NULL), but there are still blocks to be released (or some other users of the jbd2 layer still wants to have the callback be called). So I'm currently testing this patch (see below).

- Ted

fs/jbd2/commit.c +4 -4

index 8b119e1..ebc667b 100644
@@ -974,6 +974,9 @@ restart_loop:
 	journal->j_committing_transaction = NULL;
 	spin_unlock(&journal->j_state_lock);
 
+	if (journal->j_commit_callback)
+		journal->j_commit_callback(journal, commit_transaction);
+
 	if (commit_transaction->t_checkpoint_list == NULL &&
 	    commit_transaction->t_checkpoint_io_list == NULL) {
 		__jbd2_journal_drop_transaction(journal, commit_transaction);

@@ -995,11 +998,8 @@ restart_loop:
 	}
 	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_devname, journal->j_commit_sequence,
 		   journal->j_tail_sequence);
 	jbd_debug(1, "JBD: commit %d complete, head %d\n",
 		  journal->j_commit_sequence, journal->j_tail_sequence);

This is what I plan to send to Linus to fix the problem.

commit 6fecbc3c7d27800e90a5f5fbca2fb2847e2c2854

Author: Theodore Ts'o <[email protected]>
Date:   Mon Oct 27 22:11:39 2008 -0400

jbd2: Call the commit callback before the transaction could get dropped

The transaction can potentially get dropped if there are no buffers that need to be written. Make sure we call the commit callback before potentially deciding to drop the transaction. Also avoid dereferencing the commit_transaction pointer in the marker for the same reason.

This patch fixes the bug reported by Eric Paris at:
http://bugzilla.kernel.org/show_bug.cgi?id=11838

Signed-off-by "Theodore Ts'o" <[email protected]>

fs/jbd2/commit.c +4 -4

index 8b119e1..ebc667b 100644
@@ -974,6 +974,9 @@ restart_loop:
 	journal->j_committing_transaction = NULL;
 	spin_unlock(&journal->j_state_lock);
 
+	if (journal->j_commit_callback)
+		journal->j_commit_callback(journal, commit_transaction);
+
 	if (commit_transaction->t_checkpoint_list == NULL &&
 	    commit_transaction->t_checkpoint_io_list == NULL) {
 		__jbd2_journal_drop_transaction(journal, commit_transaction);

@@ -995,11 +998,8 @@ restart_loop:
 	}
 	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_devname, journal->j_commit_sequence,
 		   journal->j_tail_sequence);
 	jbd_debug(1, "JBD: commit %d complete, head %d\n",
 		  journal->j_commit_sequence, journal->j_tail_sequence);

Quoted 14 lines · Theodore Tso

This is what I plan to send to Linus to fix the problem.

commit 6fecbc3c7d27800e90a5f5fbca2fb2847e2c2854

Author: Theodore Ts'o <[email protected]>
Date:   Mon Oct 27 22:11:39 2008 -0400

jbd2: Call the commit callback before the transaction could get dropped

The transaction can potentially get dropped if there are no buffers that need to be written. Make sure we call the commit callback before potentially deciding to drop the transaction. Also avoid dereferencing the commit_transaction pointer in the marker for the same reason.

Quoted 4 lines

This patch fixes the bug reported by Eric Paris at:
http://bugzilla.kernel.org/show_bug.cgi?id=11838

Signed-off-by "Theodore Ts'o" <[email protected]>

Acked-by Eric Sandeen <[email protected]>