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.
[ 2152.923123] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 2152.923933] last sysfs file: /sys/devices/virtual/net/sit0/type
[ 2152.923933] Dumping ftrace buffer:
[ 2152.923933] (ftrace buffer empty)
[ 2152.923933] CPU 0
[ 2152.923933] Modules linked in: sit tunnel4 nfs lockd nfs_acl bridge stp bnep sco l2cap bluetooth sunrpc ip6t_REJECT nf_conntrack_ipv6 ipv6 dm_multipath ppdev floppy pcspkr e1000 i2c_piix4 i2c_core shpchp parport_pc parport ac scsi_wait_scan mptspi mptscsih mptbase scsi_transport_spi ext4 jbd2 crc16 [last unloaded: ip6_tables]
[ 2152.923933] Pid: 629, comm: kjournald2 Not tainted 2.6.27 #125
[ 2152.923933] RIP: 0010:[<ffffffffa0055510>] [<ffffffffa0055510>] release_blocks_on_commit+0x28/0x15a [ext4]
[ 2152.923933] RSP: 0018:ffff8800159a1d10 EFLAGS: 00010282
[ 2152.923933] RAX: ffffffffa00554e8 RBX: ffff88001546b1d8 RCX: ffffffff819cf478
[ 2152.923933] RDX: 6b6b6b6b6b6b6b6b RSI: ffff88000f4e36d0 RDI: ffff88001546b1b0
[ 2152.923933] RBP: ffff8800159a1d90 R08: 0000000000000000 R09: ffff88000f4e38d0
[ 2152.923933] R10: 000000000000005a R11: ffffffff81097077 R12: ffff88001546b6b8
[ 2152.923933] R13: ffff88001546a120 R14: ffff88001546b1b0 R15: ffff88000f4e36d0
[ 2152.923933] FS: 0000000000000000(0000) GS:ffffffff81866980(0000) knlGS:0000000000000000
[ 2152.923933] CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[ 2152.923933] CR2: 00007fc237226170 CR3: 0000000010964000 CR4: 00000000000006e0
[ 2152.923933] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2152.923933] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 2152.923933] Process kjournald2 (pid: 629, threadinfo ffff8800159a0000, task ffff8800158f8000)
[ 2152.923933] Stack:
[ 2152.923933] ffff8800159a1d30 ffff88000f4e36d0 ffff8800158f8000 ffff88001546b590
[ 2152.923933] ffff8800159a1d70 0000000000000246 000000000006e957 0000000000000246
[ 2152.923933] ffff8800159a1d70 ffffffff8119d972 ffff88001546b578 ffff88001546b1d8
[ 2152.923933] Call Trace:
[ 2152.923933] [<ffffffff8119d972>] ? _raw_spin_unlock+0x8e/0x93
[ 2152.923933] [<ffffffffa0016932>] jbd2_journal_commit_transaction+0x1274/0x1312 [jbd2]
[ 2152.923933] [<ffffffff8106be97>] ? trace_hardirqs_on+0xd/0xf
[ 2152.923933] [<ffffffff81053486>] ? try_to_del_timer_sync+0x50/0x5e
[ 2152.923933] [<ffffffffa001ae07>] kjournald2+0x14f/0x3b5 [jbd2]
[ 2152.923933] [<ffffffff8105e05d>] ? autoremove_wake_function+0x0/0x3d
[ 2152.923933] [<ffffffffa001acb8>] ? kjournald2+0x0/0x3b5 [jbd2]
[ 2152.923933] [<ffffffffa001acb8>] ? kjournald2+0x0/0x3b5 [jbd2]
[ 2152.923933] [<ffffffff8105dece>] kthread+0x4e/0x7c
[ 2152.923933] [<ffffffff81012859>] child_rip+0xa/0x11
[ 2152.923933] [<ffffffff81011a63>] ? restore_args+0x0/0x30
[ 2152.923933] [<ffffffff810445b6>] ? finish_task_switch+0x0/0xda
[ 2152.923933] [<ffffffff8105de80>] ? kthread+0x0/0x7c
[ 2152.923933] [<ffffffff8101284f>] ? child_rip+0x0/0x11
[ 2152.923933] Code: 5f c9 c3 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 58 0f 1f 44 00 00 48 89 75 88 48 8b 96 00 01 00 00 4c 8b af a0 05 00 00 <4c> 8b 32 e9 08 01 00 00 4c 8d 62 e8 4c 8d 7d 90 4c 89 ef 49 8b
[ 2152.923933] RIP [<ffffffffa0055510>] release_blocks_on_commit+0x28/0x15a [ext4]
[ 2152.923933] RSP <ffff8800159a1d10>
[ 2153.162115] ---[ end trace 18e42daa426b7e09 ]---
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
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?
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);
}
-Eric
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
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
On Mon, Oct 27, 2008 at 07:08:37PM -0500, Eric Sandeen wrote:
> 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.
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
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 8b119e1..ebc667b 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -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);
Theodore Tso wrote:
> 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
Looks reasonable to me from a correctness perspective, anyway, as long
as holding the j_list_lock over that callback is ok.
It does fix the oops-on-reboot that I could reproduce.
-Eric
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 8b119e1..ebc667b 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -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]>
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 8b119e1..ebc667b 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -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);
Theodore Tso wrote:
> 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]>
Acked-by: Eric Sandeen <[email protected]>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 8b119e1..ebc667b 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -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);
On Mon, 2008-10-27 at 21:41 -0500, Eric Sandeen wrote:
> Theodore Tso wrote:
> > 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]>
>
> Acked-by: Eric Sandeen <[email protected]>
Tested-by: Eric Paris <[email protected]>