bug caused by removal of trans_mutex? (Was: Re: kernel BUG at fs/btrfs/extent-tree.c:6164!)

Commit Message

Yan, Zheng June 14, 2011, 3:24 a.m. UTC

On Tue, Jun 14, 2011 at 3:55 AM, Chris Mason <[e-mail]> wrote:
> Excerpts from Yan, Zheng's message of 2011-06-13 10:58:35 -0400:
>> The usage of trans_mutex in relocation code is subtle. It controls
>> interaction of relocation
>> with transaction start, transaction commit and snapshot creation.
>> Simple replacing
>> trans_mutex with trans_lock is wrong.
>
> So, I've got a mutex around the reloc_root here and that was almost but
> not quite enough.  It looks like the biggest problem is that we need to
> wait in btrfs_record_root_in_trans for anyone inside merge_reloc_roots.
>
> I'm surviving much longer with a patch in place that synchronizes
> btrfs_record_root_in_trans better.
>
> Zheng if you have other comments on the locking please let me know.
>

following untested patch may help.
---
 		trans->block_rsv = rc->block_rsv;
@@ -1353,6 +1355,8 @@ int btrfs_init_reloc_root(struct
btrfs_trans_handle *trans,

 	__add_reloc_root(reloc_root);
 	root->reloc_root = reloc_root;
+unlock:
+	mutex_unlock(&root->fs_info->reloc_mutex);
 	return 0;
 }

@@ -1367,8 +1371,9 @@ int btrfs_update_reloc_root(struct
btrfs_trans_handle *trans,
 	int del = 0;
 	int ret;

+	mutex_lock(&root->fs_info->reloc_mutex);
 	if (!root->reloc_root)
-		return 0;
+		goto unlock;

 	reloc_root = root->reloc_root;
 	root_item = &reloc_root->root_item;
@@ -1390,6 +1395,8 @@ int btrfs_update_reloc_root(struct
btrfs_trans_handle *trans,
 	ret = btrfs_update_root(trans, root->fs_info->tree_root,
 				&reloc_root->root_key, root_item);
 	BUG_ON(ret);
+unlock:
+	mutex_unlock(&root->fs_info->reloc_mutex);
 	return 0;
 }

@@ -2142,10 +2149,10 @@ int prepare_to_merge(struct reloc_control *rc, int err)
 	u64 num_bytes = 0;
 	int ret;

-	spin_lock(&root->fs_info->trans_lock);
+	mutex_lock(&root->fs_info->reloc_mutex);
 	rc->merging_rsv_size += root->nodesize * (BTRFS_MAX_LEVEL - 1) * 2;
 	rc->merging_rsv_size += rc->nodes_relocated * 2;
-	spin_unlock(&root->fs_info->trans_lock);
+	mutex_unlock(&root->fs_info->reloc_mutex);
 again:
 	if (!err) {
 		num_bytes = rc->merging_rsv_size;
@@ -2214,9 +2221,9 @@ int merge_reloc_roots(struct reloc_control *rc)
 	int ret;
 again:
 	root = rc->extent_root;
-	spin_lock(&root->fs_info->trans_lock);
+	mutex_lock(&root->fs_info->reloc_mutex);
 	list_splice_init(&rc->reloc_roots, &reloc_roots);
-	spin_unlock(&root->fs_info->trans_lock);
+	mutex_unlock(&root->fs_info->reloc_mutex);

 	while (!list_empty(&reloc_roots)) {
 		found = 1;
@@ -3590,17 +3597,17 @@ next:
 static void set_reloc_control(struct reloc_control *rc)
 {
 	struct btrfs_fs_info *fs_info = rc->extent_root->fs_info;
-	spin_lock(&fs_info->trans_lock);
+	mutex_lock(&fs_info->reloc_mutex);
 	fs_info->reloc_ctl = rc;
-	spin_unlock(&fs_info->trans_lock);
+	mutex_unlock(&fs_info->reloc_mutex);
 }

 static void unset_reloc_control(struct reloc_control *rc)
 {
 	struct btrfs_fs_info *fs_info = rc->extent_root->fs_info;
-	spin_lock(&fs_info->trans_lock);
+	mutex_lock(&fs_info->reloc_mutex);
 	fs_info->reloc_ctl = NULL;
-	spin_unlock(&fs_info->trans_lock);
+	mutex_unlock(&fs_info->reloc_mutex);
 }

 static int check_extent_flags(u64 flags)
@@ -4335,16 +4342,16 @@ void btrfs_reloc_pre_snapshot(struct
btrfs_trans_handle *trans,
 			      struct btrfs_pending_snapshot *pending,
 			      u64 *bytes_to_reserve)
 {
-	struct btrfs_root *root;
+	struct btrfs_root *root = pending->root;
 	struct reloc_control *rc;

-	root = pending->root;
+	mutex_lock(&root->fs_info->reloc_mutex);
 	if (!root->reloc_root)
-		return;
+		goto unlock;

 	rc = root->fs_info->reloc_ctl;
 	if (!rc->merge_reloc_tree)
-		return;
+		goto unlock;

 	root = root->reloc_root;
 	BUG_ON(btrfs_root_refs(&root->root_item) == 0);
@@ -4359,6 +4366,8 @@ void btrfs_reloc_pre_snapshot(struct
btrfs_trans_handle *trans,
 	 * reserve extra space.
 	 */
 	*bytes_to_reserve += rc->nodes_relocated;
+unlock:
+	mutex_unlock(&root->fs_info->reloc_mutex);
 }

 /*
@@ -4374,8 +4383,9 @@ void btrfs_reloc_post_snapshot(struct
btrfs_trans_handle *trans,
 	struct reloc_control *rc;
 	int ret;

+	mutex_lock(&root->fs_info->reloc_mutex);
 	if (!root->reloc_root)
-		return;
+		goto unlock;

 	rc = root->fs_info->reloc_ctl;
 	rc->merging_rsv_size += rc->nodes_relocated;
@@ -4398,4 +4408,6 @@ void btrfs_reloc_post_snapshot(struct
btrfs_trans_handle *trans,
 		ret = clone_backref_node(trans, rc, root, reloc_root);
 		BUG_ON(ret);
 	}
+unlock:
+	mutex_unlock(&root->fs_info->reloc_mutex);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Li Zefan June 14, 2011, 5:44 a.m. UTC | #1

Yan, Zheng wrote:
> On Tue, Jun 14, 2011 at 3:55 AM, Chris Mason <[e-mail]> wrote:
>> Excerpts from Yan, Zheng's message of 2011-06-13 10:58:35 -0400:
>>> The usage of trans_mutex in relocation code is subtle. It controls
>>> interaction of relocation
>>> with transaction start, transaction commit and snapshot creation.
>>> Simple replacing
>>> trans_mutex with trans_lock is wrong.
>>
>> So, I've got a mutex around the reloc_root here and that was almost but
>> not quite enough.  It looks like the biggest problem is that we need to
>> wait in btrfs_record_root_in_trans for anyone inside merge_reloc_roots.
>>
>> I'm surviving much longer with a patch in place that synchronizes
>> btrfs_record_root_in_trans better.
>>
>> Zheng if you have other comments on the locking please let me know.
>>
> 
> following untested patch may help.

I've tested this patch, and the bug was triggered in minutes as usual.

Also I've tested a patch in an offline email from Chris, which survived
the test.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 378b5b4..0b20dda 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -951,6 +951,7 @@  struct btrfs_fs_info {
 	struct mutex cleaner_mutex;
 	struct mutex chunk_mutex;
 	struct mutex volume_mutex;
+	struct mutex reloc_mutex;
 	/*
 	 * this protects the ordered operations list only while we are
 	 * processing all of the entries on it.  This way we make
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9f68c68..28f8b11 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1714,6 +1714,7 @@  struct btrfs_root *open_ctree(struct super_block *sb,
 	mutex_init(&fs_info->transaction_kthread_mutex);
 	mutex_init(&fs_info->cleaner_mutex);
 	mutex_init(&fs_info->volume_mutex);
+	mutex_init(&fs_info->reloc_mutex);
 	init_rwsem(&fs_info->extent_commit_sem);
 	init_rwsem(&fs_info->cleanup_work_sem);
 	init_rwsem(&fs_info->subvol_sem);
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index b1ef27c..620e4af 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1330,18 +1330,20 @@  int btrfs_init_reloc_root(struct
btrfs_trans_handle *trans,
 			  struct btrfs_root *root)
 {
 	struct btrfs_root *reloc_root;
-	struct reloc_control *rc = root->fs_info->reloc_ctl;
+	struct reloc_control *rc;
 	int clear_rsv = 0;

+	mutex_lock(&root->fs_info->reloc_mutex);
 	if (root->reloc_root) {
 		reloc_root = root->reloc_root;
 		reloc_root->last_trans = trans->transid;
-		return 0;
+		goto unlock;
 	}

+	rc = root->fs_info->reloc_ctl;
 	if (!rc || !rc->create_reloc_tree ||
 	    root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID)
-		return 0;
+		goto unlock;

 	if (!trans->block_rsv) {