thr3ads.net - Btrfs devel - [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails [Nov 2012]

If this information is useful, please help other people find it:
Share via:

Miao Xie

2012-Nov-19 09:20 UTC

[PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call
fails and jumps to the cleanup_transaction label with these mutexes still held
causing deadlock when they are reacquired.

Fix this problem by unlocking those mutexes at the suitable place.

Reported-by: Daniel J Blueman <daniel@quora.org>
Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/delayed-ref.c |  8 ++++++++
 fs/btrfs/delayed-ref.h |  6 ++++++
 fs/btrfs/extent-tree.c | 25 ++++++++++++++++++-------
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index ae94117..364cb90 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -422,6 +422,14 @@ again:
 	return 1;
 }
 
+void btrfs_release_ref_cluster(struct list_head *cluster)
+{
+	struct list_head *pos, *q;
+
+	list_for_each_safe(pos, q, cluster)
+		list_del_init(pos);
+}
+
 /*
  * helper function to update an extent delayed ref in the
  * rbtree.  existing and update must both have the same
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index ab53005..fcc5a29 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head *
 btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr);
 int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans,
 			   struct btrfs_delayed_ref_head *head);
+static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head
*head)
+{
+	mutex_unlock(&head->mutex);
+}
+
 int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans,
 			   struct list_head *cluster, u64 search_start);
+void btrfs_release_ref_cluster(struct list_head *cluster);
 
 int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info,
 			    struct btrfs_delayed_ref_root *delayed_refs,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3d3e2c1..43a58cb 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct btrfs_trans_handle
*trans,
 						      node->num_bytes);
 			}
 		}
-		mutex_unlock(&head->mutex);
 		return ret;
 	}
 
@@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 			 * process of being added. Don''t run this ref yet.
 			 */
 			list_del_init(&locked_ref->cluster);
-			mutex_unlock(&locked_ref->mutex);
+			btrfs_delayed_ref_unlock(locked_ref);
 			locked_ref = NULL;
 			delayed_refs->num_heads_ready++;
 			spin_unlock(&delayed_refs->lock);
@@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 				if (ret) {
 					printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned %d\n",
ret);
 					spin_lock(&delayed_refs->lock);
+					btrfs_delayed_ref_unlock(locked_ref);
 					return ret;
 				}
 
 				goto next;
 			}
-
-			list_del_init(&locked_ref->cluster);
-			locked_ref = NULL;
 		}
 
 		ref->in_tree = 0;
@@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 
 		ret = run_one_delayed_ref(trans, root, ref, extent_op,
 					  must_insert_reserved);
-
-		btrfs_put_delayed_ref(ref);
 		kfree(extent_op);
 		count++;
 
+		/*
+		 * If this node is a head, we will pick the next head to deal
+		 * with. If there is something wrong when we process the
+		 * delayed ref, we will end our operation. So in these two
+		 * cases, we have to unlock the head and drop it from the
+		 * cluster list before we release it though the code is ugly.
+		 */
+		if (btrfs_delayed_ref_is_head(ref) || ret) {
+			list_del_init(&locked_ref->cluster);
+			btrfs_delayed_ref_unlock(locked_ref);
+			locked_ref = NULL;
+		}
+
+		btrfs_put_delayed_ref(ref);
+
 		if (ret) {
 			printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n",
ret);
 			spin_lock(&delayed_refs->lock);
@@ -2510,6 +2520,7 @@ again:
 
 		ret = run_clustered_refs(trans, root, &cluster);
 		if (ret < 0) {
+			btrfs_release_ref_cluster(&cluster);
 			spin_unlock(&delayed_refs->lock);
 			btrfs_abort_transaction(trans, root, ret);
 			return ret;
-- 
1.7.11.7

--
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

Liu Bo

2012-Nov-19 10:18 UTC

head link

Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

On Mon, Nov 19, 2012 at 05:20:06PM +0800, Miao Xie
wrote:> Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call
> fails and jumps to the cleanup_transaction label with these mutexes still
held
> causing deadlock when they are reacquired.
> 
> Fix this problem by unlocking those mutexes at the suitable place.
> 
> Reported-by: Daniel J Blueman <daniel@quora.org>
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
>  fs/btrfs/delayed-ref.c |  8 ++++++++
>  fs/btrfs/delayed-ref.h |  6 ++++++
>  fs/btrfs/extent-tree.c | 25 ++++++++++++++++++-------
>  3 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index ae94117..364cb90 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -422,6 +422,14 @@ again:
>  	return 1;
>  }
>  
> +void btrfs_release_ref_cluster(struct list_head *cluster)
> +{
> +	struct list_head *pos, *q;
> +
> +	list_for_each_safe(pos, q, cluster)
> +		list_del_init(pos);
> +}
> +
>  /*
>   * helper function to update an extent delayed ref in the
>   * rbtree.  existing and update must both have the same
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index ab53005..fcc5a29 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head *
>  btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr);
>  int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans,
>  			   struct btrfs_delayed_ref_head *head);
> +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head
*head)
> +{
> +	mutex_unlock(&head->mutex);
> +}
> +
>  int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans,
>  			   struct list_head *cluster, u64 search_start);
> +void btrfs_release_ref_cluster(struct list_head *cluster);
>  
>  int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info,
>  			    struct btrfs_delayed_ref_root *delayed_refs,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 3d3e2c1..43a58cb 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct
btrfs_trans_handle *trans,
>  						      node->num_bytes);
>  			}
>  		}
> -		mutex_unlock(&head->mutex);
I think code can be clean here if we keep this mutex_unlock(), see below,
please.
>  		return ret;
>  	}
>  
> @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  			 * process of being added. Don''t run this ref yet.
>  			 */
>  			list_del_init(&locked_ref->cluster);
> -			mutex_unlock(&locked_ref->mutex);
> +			btrfs_delayed_ref_unlock(locked_ref);
>  			locked_ref = NULL;
>  			delayed_refs->num_heads_ready++;
>  			spin_unlock(&delayed_refs->lock);
> @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  				if (ret) {
>  					printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned
%d\n", ret);
>  					spin_lock(&delayed_refs->lock);
> +					btrfs_delayed_ref_unlock(locked_ref);
>  					return ret;
>  				}
>  
>  				goto next;
>  			}
> -
> -			list_del_init(&locked_ref->cluster);
> -			locked_ref = NULL;
>  		}
>  
>  		ref->in_tree = 0;
> @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  
>  		ret = run_one_delayed_ref(trans, root, ref, extent_op,
>  					  must_insert_reserved);
> -
> -		btrfs_put_delayed_ref(ref);
>  		kfree(extent_op);
>  		count++;
>  
> +		/*
> +		 * If this node is a head, we will pick the next head to deal
> +		 * with. If there is something wrong when we process the
> +		 * delayed ref, we will end our operation. So in these two
> +		 * cases, we have to unlock the head and drop it from the
> +		 * cluster list before we release it though the code is ugly.
> +		 */
> +		if (btrfs_delayed_ref_is_head(ref) || ret) {
> +			list_del_init(&locked_ref->cluster);
> +			btrfs_delayed_ref_unlock(locked_ref);
> +			locked_ref = NULL;
> +		}
> +
In case that we don''t remove mutex_unlock above,

if ret is non-zero, either
A)locked_ref is not NULL, or
B)locked_ref is NULL, and it has done list_del_init above and
  also done mutex_unlock in run_one_delayed_ref().

So in the case A), it is ok to do list_del_init() and mutex_unlock(),
while in the case B), we need to do nothing.

Then the code can be clean as we wish,
if (ret) {
	if (locked_ref) {
		list_del_init();
		mutex_unlock();
	}
	...
}

thanks,
liubo
> +		btrfs_put_delayed_ref(ref);
> +
>  		if (ret) {
>  			printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n",
ret);
>  			spin_lock(&delayed_refs->lock);
> @@ -2510,6 +2520,7 @@ again:
>  
>  		ret = run_clustered_refs(trans, root, &cluster);
>  		if (ret < 0) {
> +			btrfs_release_ref_cluster(&cluster);
>  			spin_unlock(&delayed_refs->lock);
>  			btrfs_abort_transaction(trans, root, ret);
>  			return ret;
> -- 
> 1.7.11.7
> --
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

Miao Xie

2012-Nov-20 02:47 UTC

head link

Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

On mon, 19 Nov 2012 18:18:48 +0800, Liu Bo wrote:>> @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>>  				if (ret) {
>>  					printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned
%d\n", ret);
>>  					spin_lock(&delayed_refs->lock);
>> +					btrfs_delayed_ref_unlock(locked_ref);
>>  					return ret;
>>  				}
>>  
>>  				goto next;
>>  			}
>> -
>> -			list_del_init(&locked_ref->cluster);
>> -			locked_ref = NULL;
>>  		}
>>  
>>  		ref->in_tree = 0;
>> @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>>  
>>  		ret = run_one_delayed_ref(trans, root, ref, extent_op,
>>  					  must_insert_reserved);
>> -
>> -		btrfs_put_delayed_ref(ref);
>>  		kfree(extent_op);
>>  		count++;
>>  
>> +		/*
>> +		 * If this node is a head, we will pick the next head to deal
>> +		 * with. If there is something wrong when we process the
>> +		 * delayed ref, we will end our operation. So in these two
>> +		 * cases, we have to unlock the head and drop it from the
>> +		 * cluster list before we release it though the code is ugly.
>> +		 */
>> +		if (btrfs_delayed_ref_is_head(ref) || ret) {
>> +			list_del_init(&locked_ref->cluster);
>> +			btrfs_delayed_ref_unlock(locked_ref);
>> +			locked_ref = NULL;
>> +		}
>> +
> 
> In case that we don''t remove mutex_unlock above,
> 
> if ret is non-zero, either
> A)locked_ref is not NULL, or
> B)locked_ref is NULL, and it has done list_del_init above and
>   also done mutex_unlock in run_one_delayed_ref().
> 
> So in the case A), it is ok to do list_del_init() and mutex_unlock(),
> while in the case B), we need to do nothing.
> 
> Then the code can be clean as we wish,
> if (ret) {
> 	if (locked_ref) {
> 		list_del_init();
> 		mutex_unlock();
> 	}
> 	...
> }
I think it is not good style that locking/unlocking a lock in the different
functions, because
it is error prone and the readability of the code is very bad, so I remove
mutex_unlock() in
run_one_delayed_ref().

Maybe I should not mix the code of the error path into the normal one, I will
send out a new patch
to make the code cleaner and more readable.

Thanks
Miao
--
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

Miao Xie

2012-Nov-20 02:52 UTC

head link

[PATCH V2] Btrfs: fix deadlock when the process of delayed refs fails

Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call
fails and jumps to the cleanup_transaction label with these mutexes still held
causing deadlock when they are reacquired.

Fix this problem by unlocking those mutexes at the suitable place.

Reported-by: Daniel J Blueman <daniel@quora.org>
Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog from v1 -> v2:
- split the code of the run_one_delayed_ref()''s error path from the
normal path
---
 fs/btrfs/delayed-ref.c |  8 ++++++++
 fs/btrfs/delayed-ref.h |  6 ++++++
 fs/btrfs/extent-tree.c | 36 ++++++++++++++++++++++++------------
 3 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index ae94117..364cb90 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -422,6 +422,14 @@ again:
 	return 1;
 }
 
+void btrfs_release_ref_cluster(struct list_head *cluster)
+{
+	struct list_head *pos, *q;
+
+	list_for_each_safe(pos, q, cluster)
+		list_del_init(pos);
+}
+
 /*
  * helper function to update an extent delayed ref in the
  * rbtree.  existing and update must both have the same
diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
index ab53005..fcc5a29 100644
--- a/fs/btrfs/delayed-ref.h
+++ b/fs/btrfs/delayed-ref.h
@@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head *
 btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr);
 int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans,
 			   struct btrfs_delayed_ref_head *head);
+static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head
*head)
+{
+	mutex_unlock(&head->mutex);
+}
+
 int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans,
 			   struct list_head *cluster, u64 search_start);
+void btrfs_release_ref_cluster(struct list_head *cluster);
 
 int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info,
 			    struct btrfs_delayed_ref_root *delayed_refs,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3d3e2c1..93faf5e 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct btrfs_trans_handle
*trans,
 						      node->num_bytes);
 			}
 		}
-		mutex_unlock(&head->mutex);
 		return ret;
 	}
 
@@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 			 * process of being added. Don''t run this ref yet.
 			 */
 			list_del_init(&locked_ref->cluster);
-			mutex_unlock(&locked_ref->mutex);
+			btrfs_delayed_ref_unlock(locked_ref);
 			locked_ref = NULL;
 			delayed_refs->num_heads_ready++;
 			spin_unlock(&delayed_refs->lock);
@@ -2314,16 +2313,17 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 				kfree(extent_op);
 
 				if (ret) {
-					printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned %d\n",
ret);
+					printk(KERN_DEBUG
+					       "btrfs: run_delayed_extent_op "
+					       "returned %d\n",
+					       ret);
 					spin_lock(&delayed_refs->lock);
+					btrfs_delayed_ref_unlock(locked_ref);
 					return ret;
 				}
 
 				goto next;
 			}
-
-			list_del_init(&locked_ref->cluster);
-			locked_ref = NULL;
 		}
 
 		ref->in_tree = 0;
@@ -2350,17 +2350,28 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
 
 		ret = run_one_delayed_ref(trans, root, ref, extent_op,
 					  must_insert_reserved);
-
-		btrfs_put_delayed_ref(ref);
 		kfree(extent_op);
-		count++;
-
 		if (ret) {
-			printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n",
ret);
+			btrfs_delayed_ref_unlock(locked_ref);
+			btrfs_put_delayed_ref(ref);
+			printk(KERN_DEBUG
+			       "btrfs: run_one_delayed_ref returned %d\n", ret);
 			spin_lock(&delayed_refs->lock);
 			return ret;
 		}
-
+		/*
+		 * If this node is a head, that means all the refs in this head
+		 * have been dealt with, and we will pick the next head to deal
+		 * with, so we must unlock the head and drop it from the cluster
+		 * list before we release it.
+		 */
+		if (btrfs_delayed_ref_is_head(ref)) {
+			list_del_init(&locked_ref->cluster);
+			btrfs_delayed_ref_unlock(locked_ref);
+			locked_ref = NULL;
+		}
+		btrfs_put_delayed_ref(ref);
+		count++;
 next:
 		cond_resched();
 		spin_lock(&delayed_refs->lock);
@@ -2510,6 +2521,7 @@ again:
 
 		ret = run_clustered_refs(trans, root, &cluster);
 		if (ret < 0) {
+			btrfs_release_ref_cluster(&cluster);
 			spin_unlock(&delayed_refs->lock);
 			btrfs_abort_transaction(trans, root, ret);
 			return ret;
-- 
1.7.11.7

--
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

Liu Bo

2012-Nov-20 03:17 UTC

head link

Re: [PATCH V2] Btrfs: fix deadlock when the process of delayed refs fails

On Tue, Nov 20, 2012 at 10:52:36AM +0800, Miao Xie
wrote:> Delayed ref mutexes are taken inside btrfs_commit_transaction. A later call
> fails and jumps to the cleanup_transaction label with these mutexes still
held
> causing deadlock when they are reacquired.
> 
> Fix this problem by unlocking those mutexes at the suitable place.
> 
> Reported-by: Daniel J Blueman <daniel@quora.org>
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> Changelog from v1 -> v2:
> - split the code of the run_one_delayed_ref()''s error path from
the normal path
> ---
>  fs/btrfs/delayed-ref.c |  8 ++++++++
>  fs/btrfs/delayed-ref.h |  6 ++++++
>  fs/btrfs/extent-tree.c | 36 ++++++++++++++++++++++++------------
>  3 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index ae94117..364cb90 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -422,6 +422,14 @@ again:
>  	return 1;
>  }
>  
> +void btrfs_release_ref_cluster(struct list_head *cluster)
> +{
> +	struct list_head *pos, *q;
> +
> +	list_for_each_safe(pos, q, cluster)
> +		list_del_init(pos);
> +}
> +
>  /*
>   * helper function to update an extent delayed ref in the
>   * rbtree.  existing and update must both have the same
> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h
> index ab53005..fcc5a29 100644
> --- a/fs/btrfs/delayed-ref.h
> +++ b/fs/btrfs/delayed-ref.h
> @@ -176,8 +176,14 @@ struct btrfs_delayed_ref_head *
>  btrfs_find_delayed_ref_head(struct btrfs_trans_handle *trans, u64 bytenr);
>  int btrfs_delayed_ref_lock(struct btrfs_trans_handle *trans,
>  			   struct btrfs_delayed_ref_head *head);
> +static inline void btrfs_delayed_ref_unlock(struct btrfs_delayed_ref_head
*head)
> +{
> +	mutex_unlock(&head->mutex);
> +}
> +
>  int btrfs_find_ref_cluster(struct btrfs_trans_handle *trans,
>  			   struct list_head *cluster, u64 search_start);
> +void btrfs_release_ref_cluster(struct list_head *cluster);
>  
>  int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info,
>  			    struct btrfs_delayed_ref_root *delayed_refs,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 3d3e2c1..93faf5e 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2160,7 +2160,6 @@ static int run_one_delayed_ref(struct
btrfs_trans_handle *trans,
>  						      node->num_bytes);
>  			}
>  		}
> -		mutex_unlock(&head->mutex);
>  		return ret;
>  	}
>  
> @@ -2275,7 +2274,7 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  			 * process of being added. Don''t run this ref yet.
>  			 */
>  			list_del_init(&locked_ref->cluster);
> -			mutex_unlock(&locked_ref->mutex);
> +			btrfs_delayed_ref_unlock(locked_ref);
>  			locked_ref = NULL;
>  			delayed_refs->num_heads_ready++;
>  			spin_unlock(&delayed_refs->lock);
> @@ -2314,16 +2313,17 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  				kfree(extent_op);
>  
>  				if (ret) {
> -					printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned
%d\n", ret);
> +					printk(KERN_DEBUG
> +					       "btrfs: run_delayed_extent_op "
> +					       "returned %d\n",
> +					       ret);
>  					spin_lock(&delayed_refs->lock);
> +					btrfs_delayed_ref_unlock(locked_ref);
>  					return ret;
>  				}
>  
>  				goto next;
>  			}
> -
> -			list_del_init(&locked_ref->cluster);
> -			locked_ref = NULL;
>  		}
>  
>  		ref->in_tree = 0;
> @@ -2350,17 +2350,28 @@ static noinline int run_clustered_refs(struct
btrfs_trans_handle *trans,
>  
>  		ret = run_one_delayed_ref(trans, root, ref, extent_op,
>  					  must_insert_reserved);
> -
> -		btrfs_put_delayed_ref(ref);
>  		kfree(extent_op);
> -		count++;
> -
>  		if (ret) {
> -			printk(KERN_DEBUG "btrfs: run_one_delayed_ref returned %d\n",
ret);
> +			btrfs_delayed_ref_unlock(locked_ref);
> +			btrfs_put_delayed_ref(ref);
> +			printk(KERN_DEBUG
> +			       "btrfs: run_one_delayed_ref returned %d\n", ret);
>  			spin_lock(&delayed_refs->lock);
>  			return ret;
>  		}
> -
> +		/*
> +		 * If this node is a head, that means all the refs in this head
> +		 * have been dealt with, and we will pick the next head to deal
> +		 * with, so we must unlock the head and drop it from the cluster
> +		 * list before we release it.
> +		 */
Well...I don''t think these comments are necessary here though.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

thanks,
liubo
> +		if (btrfs_delayed_ref_is_head(ref)) {
> +			list_del_init(&locked_ref->cluster);
> +			btrfs_delayed_ref_unlock(locked_ref);
> +			locked_ref = NULL;
> +		}
> +		btrfs_put_delayed_ref(ref);
> +		count++;
>  next:
>  		cond_resched();
>  		spin_lock(&delayed_refs->lock);
> @@ -2510,6 +2521,7 @@ again:
>  
>  		ret = run_clustered_refs(trans, root, &cluster);
>  		if (ret < 0) {
> +			btrfs_release_ref_cluster(&cluster);
>  			spin_unlock(&delayed_refs->lock);
>  			btrfs_abort_transaction(trans, root, ret);
>  			return ret;
> -- 
> 1.7.11.7
> 
> --
> 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

Btrfs devel - Nov 2012 - [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

[PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

Re: [PATCH RESEND] Btrfs: fix deadlock when the process of delayed refs fails

[PATCH V2] Btrfs: fix deadlock when the process of delayed refs fails

Re: [PATCH V2] Btrfs: fix deadlock when the process of delayed refs fails