Btrfs devel - Jun 2008 - btrfs panic - BUG: soft lockup - CPU#0 stuck for 61s! [fs_mark:4573]

I can reliably get btrfs to panic by running my fs_mark code on a newly 
created file system with lots of threads on an 8-way box. If this is too 
aggressive, let me know ;-)

Here is a summary of the panic:

EXT3-fs: recovery complete.
EXT3-fs: mounted filesystem with ordered data mode.
device fsid 814e6131acbfcbec-7a2a40df880929bb devid 1 transid 495 /dev/sdb1
BUG: soft lockup - CPU#1 stuck for 61s! [fs_mark:4572]
CPU 1:
Modules linked in: btrfs libcrc32c ipt_MASQUERADE iptable_nat nf_nat 
bridge bnep rfcomm l2cap bluetooth ib_iser rdma_cm ib_cm iw_cm ib_sa 
ib_mad ib_core ib_addr iscsi_tcp libiscsi scsi_transport_iscsi fuse 
sunrpc ipt_REJECT nf_conntrack_ipv4 iptable_filter ip_tables ip6t_REJECT 
xt_tcpudp nf_conntrack_ipv6 xt_state nf_conntrack ip6table_filter 
ip6_tables x_tables ipv6 dm_mirror dm_multipath dm_mod kvm_intel kvm 
snd_hda_intel snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq 
snd_seq_device snd_pcm_oss iTCO_wdt snd_mixer_oss iTCO_vendor_support 
nvidia(P) pata_acpi button ata_piix snd_pcm ppdev firewire_ohci i2c_i801 
ata_generic firewire_core pcspkr dcdbas snd_timer sr_mod cdrom 
snd_page_alloc snd_hwdep snd tg3 serio_raw i2c_core shpchp crc_itu_t sg 
parport_pc soundcore parport floppy ahci libata sd_mod scsi_mod ext3 jbd 
mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: freq_table]
Pid: 4572, comm: fs_mark Tainted: P         2.6.25.3-18.fc9.x86_64 #1
RIP: 0010:[<ffffffff81132929>]  [<ffffffff81132929>] 
__write_lock_failed+0x9/0x20
RSP: 0018:ffff81000c529e60  EFLAGS: 00000206
RAX: ffff810015c0e000 RBX: ffff81000c529e68 RCX: 0000000000000016
RDX: ffff81003d019e00 RSI: 0000000000000001 RDI: ffff8100100e24f0
RBP: ffff8100189bef00 R08: 0000000000000000 R09: 0000000000000016
R10: 0000000012750e57 R11: 0000000000000246 R12: 0000000000000202
R13: ffff81000c529de8 R14: 00000000000011dc R15: ffff81000c529f58
FS:  000000000159b850(0063) GS:ffff81003f802680(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f55301940a8 CR3: 000000000c513000 CR4: 00000000000026e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400

Call Trace:
 [<ffffffff81292408>] ? _write_lock+0x12/0x14
 [<ffffffff88c27ff2>] ? :btrfs:btrfs_del_ordered_inode+0xc0/0x13f
 [<ffffffff88c1e67f>] ? :btrfs:btrfs_release_file+0x9/0xd
 [<ffffffff810a5eff>] ? __fput+0xca/0x189
 [<ffffffff810a5fd2>] ? fput+0x14/0x16
 [<ffffffff810a32c8>] ? filp_close+0x66/0x71
 [<ffffffff810a336c>] ? sys_close+0x99/0xd2
 [<ffffffff8100c052>] ? tracesys+0xd5/0xda


ric

On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler
wrote:
>
> I can reliably get btrfs to panic by running my fs_mark code on a newly  
> created file system with lots of threads on an 8-way box. If this is too  
> aggressive, let me know ;-)
>
> Here is a summary of the panic:
I think this is due to a corruption on the data=ordered list.  I''m
testing a patch out here.

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

Chris Mason wrote:
> On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
>   
>> I can reliably get btrfs to panic by running my fs_mark code on a newly
>> created file system with lots of threads on an 8-way box. If this is
too
>> aggressive, let me know ;-)
>>
>> Here is a summary of the panic:
>>     
>
> I think this is due to a corruption on the data=ordered list.  I''m
> testing a patch out here.
>
> -chris
>   
I can test it tomorrow if you send it on... Thanks!

ric

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

On Wed, Jun 04, 2008 at 03:46:10PM -0400, Ric Wheeler
wrote:
> Chris Mason wrote:
>> On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
>>   
>>> I can reliably get btrfs to panic by running my fs_mark code on a 
>>> newly  created file system with lots of threads on an 8-way box. If
>>> this is too  aggressive, let me know ;-)
>>>
>>> Here is a summary of the panic:
>>>     
>>
>> I think this is due to a corruption on the data=ordered list. 
I''m
>> testing a patch out here.
>>
>> -chris
>>   
> I can test it tomorrow if you send it on... Thanks!
Patch is below, but I don''t have access to my test rig so I
haven''t
hammered on it yet.  I''m willing to corrupt Ric''s test box,
but everyone
else may want to wait ;)

-chris

diff -r 0b4ab489ffe1 file.c
--- a/file.c	Tue May 27 10:55:43 2008 -0400
+++ b/file.c	Wed Jun 04 16:10:40 2008 -0400
@@ -980,7 +980,7 @@ out_nolock:
 
 static int btrfs_release_file (struct inode * inode, struct file * filp)
 {
-	btrfs_del_ordered_inode(inode);
+	btrfs_del_ordered_inode(inode, 0);
 	return 0;
 }
 
diff -r 0b4ab489ffe1 inode.c
--- a/inode.c	Tue May 27 10:55:43 2008 -0400
+++ b/inode.c	Wed Jun 04 16:10:40 2008 -0400
@@ -861,7 +861,7 @@ static int btrfs_unlink(struct inode *di
 		 * we don''t need to worry about
 		 * data=ordered
 		 */
-		btrfs_del_ordered_inode(inode);
+		btrfs_del_ordered_inode(inode, 0);
 	}
 
 	btrfs_end_transaction(trans, root);
@@ -3352,6 +3352,7 @@ void btrfs_destroy_inode(struct inode *i
 	WARN_ON(!list_empty(&inode->i_dentry));
 	WARN_ON(inode->i_data.nrpages);
 
+	btrfs_del_ordered_inode(inode, 1);
 	btrfs_drop_extent_cache(inode, 0, (u64)-1);
 	kmem_cache_free(btrfs_inode_cachep, BTRFS_I(inode));
 }
diff -r 0b4ab489ffe1 ordered-data.c
--- a/ordered-data.c	Tue May 27 10:55:43 2008 -0400
+++ b/ordered-data.c	Wed Jun 04 16:10:40 2008 -0400
@@ -254,7 +254,7 @@ static void __btrfs_del_ordered_inode(st
 	return;
 }
 
-void btrfs_del_ordered_inode(struct inode *inode)
+void btrfs_del_ordered_inode(struct inode *inode, int force)
 {
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	u64 root_objectid = root->root_key.objectid;
@@ -263,8 +263,8 @@ void btrfs_del_ordered_inode(struct inod
 		return;
 	}
 
-	if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) ||
-	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK))
+	if (!force && (mapping_tagged(inode->i_mapping,
PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK)))
 		return;
 
 	spin_lock(&root->fs_info->new_trans_lock);
diff -r 0b4ab489ffe1 ordered-data.h
--- a/ordered-data.h	Tue May 27 10:55:43 2008 -0400
+++ b/ordered-data.h	Wed Jun 04 16:10:40 2008 -0400
@@ -38,6 +38,6 @@ int btrfs_find_first_ordered_inode(struc
 int btrfs_find_first_ordered_inode(struct btrfs_ordered_inode_tree *tree,
 				       u64 *root_objectid, u64 *objectid,
 				       struct inode **inode);
-void btrfs_del_ordered_inode(struct inode *inode);
+void btrfs_del_ordered_inode(struct inode *inode, int force);
 int btrfs_ordered_throttle(struct btrfs_root *root, struct inode *inode);
 #endif
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler
wrote:
>
> I can reliably get btrfs to panic by running my fs_mark code on a newly  
> created file system with lots of threads on an 8-way box. If this is too  
> aggressive, let me know ;-)
>
> Here is a summary of the panic:
BTW, exactly how are you running fs_mark?  Mingming reminded me that
strictly speaking this patch shouldn''t be required, so there might be
other related problems.

-chris

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

Chris Mason wrote:
> On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
>   
>> I can reliably get btrfs to panic by running my fs_mark code on a newly
>> created file system with lots of threads on an 8-way box. If this is
too
>> aggressive, let me know ;-)
>>
>> Here is a summary of the panic:
>>     
>
> BTW, exactly how are you running fs_mark?  Mingming reminded me that
> strictly speaking this patch shouldn''t be required, so there might
be
> other related problems.
>
> -chris
>
>   
This was the actual command:

./fs_mark -d /mnt/test -D 512 -t 16 -s 409600 -F

I will give your patch a spin right after lunch ;-)

ric


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

On Thu, 05 Jun 2008 13:43:48 -0400
Ric Wheeler <rwheeler@redhat.com> wrote:
> Chris Mason wrote:
> > On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
> >   
> >> I can reliably get btrfs to panic by running my fs_mark code on a
> >> newly created file system with lots of threads on an 8-way box. If
> >> this is too aggressive, let me know ;-)
> >>
> >> Here is a summary of the panic:
> >>     
> >
> > BTW, exactly how are you running fs_mark?  Mingming reminded me that
> > strictly speaking this patch shouldn''t be required, so there
might
> > be other related problems.
> >
> > -chris
> >
> >   
> It still crashes, Mingming is clearly correct ;-)
> 
Grin, I never should have doubted her.

So, the actual fix should be below.  It looks like the problem is that
I''ve got
a race in setting the pointer to a new transaction, which makes the
data=ordered code take a spin lock that hasn''t yet been setup.

Before this patch my test box got into an infinite loop with fs_mark.  Now it
seems to run to completion.

-chris

diff -r 0b4ab489ffe1 transaction.c
--- a/transaction.c	Tue May 27 10:55:43 2008 -0400
+++ b/transaction.c	Sun Jun 08 22:23:50 2008 -0400
@@ -56,7 +56,6 @@ static noinline int join_transaction(str
 		total_trans++;
 		BUG_ON(!cur_trans);
 		root->fs_info->generation++;
-		root->fs_info->running_transaction = cur_trans;
 		root->fs_info->last_alloc = 0;
 		root->fs_info->last_data_alloc = 0;
 		cur_trans->num_writers = 1;
@@ -74,6 +73,9 @@ static noinline int join_transaction(str
 		extent_io_tree_init(&cur_trans->dirty_pages,
 				     root->fs_info->btree_inode->i_mapping,
 				     GFP_NOFS);
+		spin_lock(&root->fs_info->new_trans_lock);
+		root->fs_info->running_transaction = cur_trans;
+		spin_unlock(&root->fs_info->new_trans_lock);
 	} else {
 		cur_trans->num_writers++;
 		cur_trans->num_joined++;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris Mason wrote:
> On Thu, 05 Jun 2008 13:43:48 -0400
> Ric Wheeler <rwheeler@redhat.com> wrote:
>
>   
>> Chris Mason wrote:
>>     
>>> On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
>>>   
>>>       
>>>> I can reliably get btrfs to panic by running my fs_mark code on
a
>>>> newly created file system with lots of threads on an 8-way box.
If
>>>> this is too aggressive, let me know ;-)
>>>>
>>>> Here is a summary of the panic:
>>>>     
>>>>         
>>> BTW, exactly how are you running fs_mark?  Mingming reminded me
that
>>> strictly speaking this patch shouldn''t be required, so
there might
>>> be other related problems.
>>>
>>> -chris
>>>
>>>   
>>>       
>> It still crashes, Mingming is clearly correct ;-)
>>
>>     
>
> Grin, I never should have doubted her.
>
> So, the actual fix should be below.  It looks like the problem is that
I''ve got
> a race in setting the pointer to a new transaction, which makes the
> data=ordered code take a spin lock that hasn''t yet been setup.
>
> Before this patch my test box got into an infinite loop with fs_mark.  Now
it
> seems to run to completion.
>
> -chris
>   
Thanks Chris - this patch works for me as well,

ric
> diff -r 0b4ab489ffe1 transaction.c
> --- a/transaction.c	Tue May 27 10:55:43 2008 -0400
> +++ b/transaction.c	Sun Jun 08 22:23:50 2008 -0400
> @@ -56,7 +56,6 @@ static noinline int join_transaction(str
>  		total_trans++;
>  		BUG_ON(!cur_trans);
>  		root->fs_info->generation++;
> -		root->fs_info->running_transaction = cur_trans;
>  		root->fs_info->last_alloc = 0;
>  		root->fs_info->last_data_alloc = 0;
>  		cur_trans->num_writers = 1;
> @@ -74,6 +73,9 @@ static noinline int join_transaction(str
>  		extent_io_tree_init(&cur_trans->dirty_pages,
>  				     root->fs_info->btree_inode->i_mapping,
>  				     GFP_NOFS);
> +		spin_lock(&root->fs_info->new_trans_lock);
> +		root->fs_info->running_transaction = cur_trans;
> +		spin_unlock(&root->fs_info->new_trans_lock);
>  	} else {
>  		cur_trans->num_writers++;
>  		cur_trans->num_joined++;
>   
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, 2008-06-08 at 22:37 -0400, Chris Mason wrote:
> On Thu, 05 Jun 2008 13:43:48 -0400
> Ric Wheeler <rwheeler@redhat.com> wrote:
> 
> > Chris Mason wrote:
> > > On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
> > >   
> > >> I can reliably get btrfs to panic by running my fs_mark code
on a
> > >> newly created file system with lots of threads on an 8-way
box. If
> > >> this is too aggressive, let me know ;-)
> > >>
> > >> Here is a summary of the panic:
> > >>     
> > >
> > > BTW, exactly how are you running fs_mark?  Mingming reminded me
that
> > > strictly speaking this patch shouldn''t be required, so
there might
> > > be other related problems.
> > >
> > > -chris
> > >
> > >   
> > It still crashes, Mingming is clearly correct ;-)
> > 
> 
> Grin, I never should have doubted her.
> 
:) 
> So, the actual fix should be below.  It looks like the problem is that
I''ve got
> a race in setting the pointer to a new transaction, which makes the
> data=ordered code take a spin lock that hasn''t yet been setup.
> 
Just to be clear, so the data=ordered code(btrfs_del_ordered_inode())
takes a spin lock (new_trans_lock) and assume the new transaction has
been setup, that races with join_transaction resetting the current
running transaction()? 

I also see the btrfs_commit_transaction() could reset the
root->fs_info->running_transaction to be NULL, but we did not check NULL
pointer in the data=ordered mode code, is this a potential Bug? Or it is
covered somewhere else?

Mingming
> Before this patch my test box got into an infinite loop with fs_mark.  Now
it
> seems to run to completion.
> 
> -chris
> 
> diff -r 0b4ab489ffe1 transaction.c
> --- a/transaction.c	Tue May 27 10:55:43 2008 -0400
> +++ b/transaction.c	Sun Jun 08 22:23:50 2008 -0400
> @@ -56,7 +56,6 @@ static noinline int join_transaction(str
>  		total_trans++;
>  		BUG_ON(!cur_trans);
>  		root->fs_info->generation++;
> -		root->fs_info->running_transaction = cur_trans;
>  		root->fs_info->last_alloc = 0;
>  		root->fs_info->last_data_alloc = 0;
>  		cur_trans->num_writers = 1;
> @@ -74,6 +73,9 @@ static noinline int join_transaction(str
>  		extent_io_tree_init(&cur_trans->dirty_pages,
>  				     root->fs_info->btree_inode->i_mapping,
>  				     GFP_NOFS);
> +		spin_lock(&root->fs_info->new_trans_lock);
> +		root->fs_info->running_transaction = cur_trans;
> +		spin_unlock(&root->fs_info->new_trans_lock);
>  	} else {
>  		cur_trans->num_writers++;
>  		cur_trans->num_joined++;
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 2008-06-09 at 17:10 -0700, Mingming Cao wrote:
> On Sun, 2008-06-08 at 22:37 -0400, Chris Mason wrote:
> > On Thu, 05 Jun 2008 13:43:48 -0400
> > Ric Wheeler <rwheeler@redhat.com> wrote:
> > 
> > > Chris Mason wrote:
> > > > On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler wrote:
> > > >   
> > > >> I can reliably get btrfs to panic by running my fs_mark
code on a
> > > >> newly created file system with lots of threads on an
8-way box. If
> > > >> this is too aggressive, let me know ;-)
> > > >>
> > > >> Here is a summary of the panic:
> > > >>     
> > > >
> > > > BTW, exactly how are you running fs_mark?  Mingming reminded
me that
> > > > strictly speaking this patch shouldn''t be required,
so there might
> > > > be other related problems.
> > > >
> > > > -chris
> > > >
> > > >   
> > > It still crashes, Mingming is clearly correct ;-)
> > > 
> > 
> > Grin, I never should have doubted her.
> > 
> :) 
> 
> > So, the actual fix should be below.  It looks like the problem is that
I''ve got
> > a race in setting the pointer to a new transaction, which makes the
> > data=ordered code take a spin lock that hasn''t yet been
setup.
> > 
> 
> Just to be clear, so the data=ordered code(btrfs_del_ordered_inode())
> takes a spin lock (new_trans_lock) and assume the new transaction has
> been setup, that races with join_transaction resetting the current
> running transaction()? 
> 
Yes
> I also see the btrfs_commit_transaction() could reset the
> root->fs_info->running_transaction to be NULL, but we did not check
NULL
> pointer in the data=ordered mode code, is this a potential Bug? Or it is
> covered somewhere else?
> 
Thanks for double checking these.

We don''t check it in btrfs_add_ordered_inode because that must be
called
with the transaction running.

btrfs_ordered_throttle is safe because it doesn''t actually deref the
pointer, it just checks for changes to it.  The important part of
ordered_throttle is the writeback count.

So, the others should be safe, but please let me know if you see any
holes there.

-chris


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

On Mon, 2008-06-09 at 20:47 -0400, Chris Mason wrote:
> On Mon, 2008-06-09 at 17:10 -0700, Mingming Cao wrote:
> > On Sun, 2008-06-08 at 22:37 -0400, Chris Mason wrote:
> > > On Thu, 05 Jun 2008 13:43:48 -0400
> > > Ric Wheeler <rwheeler@redhat.com> wrote:
> > > 
> > > > Chris Mason wrote:
> > > > > On Mon, Jun 02, 2008 at 01:52:47PM -0400, Ric Wheeler
wrote:
> > > > >   
> > > > >> I can reliably get btrfs to panic by running my
fs_mark code on a
> > > > >> newly created file system with lots of threads on
an 8-way box. If
> > > > >> this is too aggressive, let me know ;-)
> > > > >>
> > > > >> Here is a summary of the panic:
> > > > >>     
> > > > >
> > > > > BTW, exactly how are you running fs_mark?  Mingming
reminded me that
> > > > > strictly speaking this patch shouldn''t be
required, so there might
> > > > > be other related problems.
> > > > >
> > > > > -chris
> > > > >
> > > > >   
> > > > It still crashes, Mingming is clearly correct ;-)
> > > > 
> > > 
> > > Grin, I never should have doubted her.
> > > 
> > :) 
> > 
> > > So, the actual fix should be below.  It looks like the problem is
that I''ve got
> > > a race in setting the pointer to a new transaction, which makes
the
> > > data=ordered code take a spin lock that hasn''t yet been
setup.
> > > 
> > 
> > Just to be clear, so the data=ordered code(btrfs_del_ordered_inode())
> > takes a spin lock (new_trans_lock) and assume the new transaction has
> > been setup, that races with join_transaction resetting the current
> > running transaction()? 
> > 
> Yes
> 
> > I also see the btrfs_commit_transaction() could reset the
> > root->fs_info->running_transaction to be NULL, but we did not
check NULL
> > pointer in the data=ordered mode code, is this a potential Bug? Or it
is
> > covered somewhere else?
> > 
> 
> Thanks for double checking these.
> 
> We don''t check it in btrfs_add_ordered_inode because that must be
called
> with the transaction running.
> 
Thanks for clarifying, I missed this.
> btrfs_ordered_throttle is safe because it doesn''t actually deref
the
> pointer, it just checks for changes to it.  The important part of
> ordered_throttle is the writeback count.
> 
> So, the others should be safe, but please let me know if you see any
> holes there.
> 
Looks pretty safe to me now, I should not doubt you earlier:)

Mingming

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