Skip to contentSkip to conversation

ocfs2: fix dead lock caused by ocfs2_defrag_extent

PATCH In review Larry Chen 3 participants 9 messages Nov 1, 2018 – Nov 2, 2018 OCFS2

lore
Share & tools
lore

Larry Chen

author
#0 lore raw

PATCH v3 1/1 ocfs2: fix dead lock caused by ocfs2_defrag_extent

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
 fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
fs/ocfs2/move_extents.c +26 -21
index 7eb3b0a6347e..f55f82ca3425 100644
@@ -156,18 +156,14 @@ static int __ocfs2_move_extent(handle_t *handle,
 }
 
 /*
- * lock allocators, and reserving appropriate number of bits for
- * meta blocks and data clusters.
- *
- * in some cases, we don't need to reserve clusters, just let data_ac
- * be NULL.
+ * lock allocator, and reserve appropriate number of bits for
+ * meta blocks.
  */
-static int ocfs2_lock_allocators_move_extents(struct inode *inode,
+static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
 					struct ocfs2_extent_tree *et,
 					u32 clusters_to_move,
 					u32 extents_to_split,
 					struct ocfs2_alloc_context **meta_ac,
-					struct ocfs2_alloc_context **data_ac,
 					int extra_blocks,
 					int *credits)
 {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
 		goto out;
 	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
 	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
 	handle = ocfs2_start_trans(osb, credits);
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
Signature · 1 line
2.16.4
#1 lore raw

Hi Larry,

Quoted 50 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
 fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
fs/ocfs2/move_extents.c +0 -2
index 7eb3b0a6347e..f55f82ca3425 100644
@@ -156,18 +156,14 @@ static int __ocfs2_move_extent(handle_t *handle,
 }
 
 /*
- * lock allocators, and reserving appropriate number of bits for
- * meta blocks and data clusters.

fs/ocfs2/move_extents.c static int __ocfs2_move_extent(handle_t *handle, · line 164

Quoted 10 lines
fs/ocfs2/move_extents.c +3 -4
- *
- * in some cases, we don't need to reserve clusters, just let data_ac
- * be NULL.
+ * lock allocator, and reserve appropriate number of bits for
+ * meta blocks.
  */
-static int ocfs2_lock_allocators_move_extents(struct inode *inode,
+static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
 					struct ocfs2_extent_tree *et,
 					u32 clusters_to_move,

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

Thanks,
Joseph

Quoted 72 lines
fs/ocfs2/move_extents.c +23 -15
 					u32 extents_to_split,
 					struct ocfs2_alloc_context **meta_ac,
-					struct ocfs2_alloc_context **data_ac,
 					int extra_blocks,
 					int *credits)
 {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
 		goto out;
 	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
 	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
 	handle = ocfs2_start_trans(osb, credits);
 	if (IS_ERR(handle)) {
 		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
 		}
 	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
#2 lore raw

Hello Joseph,

Quoted 44 lines · Joseph Qi

Hi Larry,

Quoted 31 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Quoted 9 lines

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
  fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
  1 file changed, 26 insertions(+), 21 deletions(-)
Quoted 3 lines

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

Thanks,
Changwei

Quoted 81 lines

Thanks,
Joseph

Quoted 62 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
  					int extra_blocks,
  					int *credits)
  {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
  		goto out;
  	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
  	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
  	handle = ocfs2_start_trans(osb, credits);
  	if (IS_ERR(handle)) {
  		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
  		}
  	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#3 lore raw
Quoted 47 lines · Changwei Ge

Hello Joseph,

Quoted 43 lines · Joseph Qi

Hi Larry,

Quoted 40 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
  fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
  1 file changed, 26 insertions(+), 21 deletions(-)
Quoted 8 lines
Quoted 3 lines

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

I was not saying just remove it.
IIUC, clusters_to_move is for reserving data clusters (for meta_ac, we mostly talk about blocks). Since we have moved data cluster reserve logic out of ocfs2_lock_allocators_move_extents() now, then left clusters_to_move related logic here is odd.

Quoted 78 lines
Quoted 78 lines
Quoted 62 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
  					int extra_blocks,
  					int *credits)
  {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
  		goto out;
  	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
  	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
  	handle = ocfs2_start_trans(osb, credits);
  	if (IS_ERR(handle)) {
  		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
  		}
  	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#4 lore raw

Hello Joseph,

Quoted 54 lines · Joseph Qi
Quoted 51 lines · Changwei Ge

Hello Joseph,

Quoted 47 lines · Joseph Qi

Hi Larry,

Quoted 31 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Quoted 9 lines

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
   fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
   1 file changed, 26 insertions(+), 21 deletions(-)

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

Quoted 6 lines
Quoted 4 lines

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

I was not saying just remove it.
IIUC, clusters_to_move is for reserving data clusters (for meta_ac, we

Um...
*cluster_to_move* is not only used for reserving data clusters.
It is also an effecting input for calculating if existed extents still have enough free records for later tree operation like attaching clusters to extents.

Please refer to below code:
175 unsigned int max_recs_needed = 2 * extents_to_split + clusters_to_move;

Quoted 3 lines

mostly talk about blocks). Since we have moved data cluster reserve logic out of ocfs2_lock_allocators_move_extents() now, then left clusters_to_move related logic here is odd.

Like my preceding elaboration, it is used for telling if we need more extents. Anyway, I think we must keep *cluster_to_move* here as it used to. :-)

Thanks,
Changwei

Quoted 80 lines
Quoted 79 lines
Quoted 79 lines
Quoted 63 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
   					int extra_blocks,
   					int *credits)
   {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
   		goto out;
   	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
   	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
   	handle = ocfs2_start_trans(osb, credits);
   	if (IS_ERR(handle)) {
   		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
   		}
   	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#5 lore raw
Quoted 64 lines · Changwei Ge

Hello Joseph,

Quoted 54 lines · Joseph Qi
Quoted 51 lines · Changwei Ge

Hello Joseph,

Quoted 47 lines · Joseph Qi

Hi Larry,

Quoted 31 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Quoted 9 lines

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
   fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
   1 file changed, 26 insertions(+), 21 deletions(-)

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

Quoted 6 lines
Quoted 4 lines

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

I was not saying just remove it.
IIUC, clusters_to_move is for reserving data clusters (for meta_ac, we

Quoted 8 lines

Um...
*cluster_to_move* is not only used for reserving data clusters.
It is also an effecting input for calculating if existed extents still have enough free records for later tree operation like attaching clusters to extents.

Please refer to below code:
175 unsigned int max_recs_needed = 2 * extents_to_split + clusters_to_move;

IC. It is a bit odd that calculate it here but do the real reserve out.

Quoted 95 lines
Quoted 5 lines

mostly talk about blocks). Since we have moved data cluster reserve logic out of ocfs2_lock_allocators_move_extents() now, then left clusters_to_move related logic here is odd.

Like my preceding elaboration, it is used for telling if we need more extents. Anyway, I think we must keep *cluster_to_move* here as it used to. :-)

Thanks,
Changwei

Quoted 80 lines
Quoted 79 lines
Quoted 79 lines
Quoted 63 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
   					int extra_blocks,
   					int *credits)
   {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
   		goto out;
   	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
   	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
   	handle = ocfs2_start_trans(osb, credits);
   	if (IS_ERR(handle)) {
   		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
   		}
   	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#6 lore raw
In reply to Changwei Ge

Hi Joseph,

Quoted 47 lines · Changwei Ge

Hello Joseph,

Quoted 43 lines · Joseph Qi

Hi Larry,

Quoted 40 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
   fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
   1 file changed, 26 insertions(+), 21 deletions(-)
Quoted 8 lines
Quoted 3 lines

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

Yeah, I think clusters_to_move should be reserved, in order to keep the original logic as it was.

But I'm curious about why max_recs_needed should be equal to 2 * extents_to_split + cluster_to_move?
Does that mean that each cluster might form an extent??

Thanks,
Larry

Quoted 85 lines

Thanks,
Changwei

Quoted 81 lines

Thanks,
Joseph

Quoted 62 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
   					int extra_blocks,
   					int *credits)
   {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
   		goto out;
   	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
   	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
   		}
   	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
   	handle = ocfs2_start_trans(osb, credits);
   	if (IS_ERR(handle)) {
   		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
   		}
   	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
   	if (ret) {
   		mlog_errno(ret);
   		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#7 lore raw

Hi Larry,

Quoted 59 lines · Larry Chen

Hi Joseph,

Quoted 47 lines · Changwei Ge

Hello Joseph,

Quoted 43 lines · Joseph Qi

Hi Larry,

Quoted 40 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
---
    fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
    1 file changed, 26 insertions(+), 21 deletions(-)
Quoted 8 lines
Quoted 3 lines

IMO, here clusters_to_move is only for data_ac, since we change this function to only handle meta_ac, I'm afraid clusters_to_move related logic has to been changed correspondingly.

I think we can't remove *clusters_to_move* from here as clusters can be reserved latter outsides this function, but we still have to reserve metadata(extents) in advance.
So we need that argument.

Quoted 6 lines

Yeah, I think clusters_to_move should be reserved, in order to keep the original logic as it was.

But I'm curious about why max_recs_needed should be equal to 2 * extents_to_split + cluster_to_move?
Does that mean that each cluster might form an extent??

It's because in the worst case that the file is of great fragment and one extent record can only represent one cluster. And we might encounter extents split during merging and rotation so we also need _2 * extents_to_split_.

Thanks,
Changwei

Quoted 92 lines

Thanks,
Larry

Quoted 85 lines

Thanks,
Changwei

Quoted 81 lines

Thanks,
Joseph

Quoted 62 lines

u32 extents_to_split, struct ocfs2_alloc_context **meta_ac,

fs/ocfs2/move_extents.c +19 -12
-					struct ocfs2_alloc_context **data_ac,
    					int extra_blocks,
    					int *credits)
    {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
    		goto out;
    	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
    	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
    		}
    	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
    	if (ret) {
    		mlog_errno(ret);
    		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
    		}
    	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
    	handle = ocfs2_start_trans(osb, credits);
    	if (IS_ERR(handle)) {
    		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
    		}
    	}

fs/ocfs2/move_extents.c static int ocfs2_move_extent(struct ocfs2_move_extents_context *context, · line 613

Quoted 10 lines
fs/ocfs2/move_extents.c +4 -3
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
    	if (ret) {
    		mlog_errno(ret);
    		goto out;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Reviewed-by on v3 1/1
#8 lore raw
In reply to Larry Chen

Hi Larry,

The change looks good to me.

Quoted 31 lines · Larry Chen

ocfs2_defrag_extent may fall into deadlock.

ocfs2_ioctl_move_extents
ocfs2_ioctl_move_extents
ocfs2_move_extents
ocfs2_defrag_extent
ocfs2_lock_allocators_move_extents

ocfs2_reserve_clusters
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

__ocfs2_flush_truncate_log
inode_lock GLOBAL_BITMAP_SYSTEM_INODE

As backtrace shows above, ocfs2_reserve_clusters() will call inode_lock against the global bitmap if local allocator has not sufficient cluters. Once global bitmap could meet the demand, ocfs2_reserve_cluster will return success with global bitmap locked.

After ocfs2_reserve_cluster(), if truncate log is full, __ocfs2_flush_truncate_log() will definitely fall into deadlock because it needs to inode_lock global bitmap, which has already been locked.

To fix this bug, we could remove from ocfs2_lock_allocators_move_extents() the code which intends to lock global allocator, and put the removed code after __ocfs2_flush_truncate_log().

ocfs2_lock_allocators_move_extents() is referred by 2 places, one is here, the other does not need the data allocator context, which means this patch does not affect the caller so far.

Quoted 5 lines

Change log:
1. Correct the function comment.
2. Remove unused argument from ocfs2_lock_meta_allocator_move_extents.

Signed-off-by Larry Chen <[email protected]>
Reviewed-by Changwei Ge <[email protected]>
Quoted 96 lines
---
  fs/ocfs2/move_extents.c | 47 ++++++++++++++++++++++++++---------------------
  1 file changed, 26 insertions(+), 21 deletions(-)
fs/ocfs2/move_extents.c +26 -21
index 7eb3b0a6347e..f55f82ca3425 100644
@@ -156,18 +156,14 @@ static int __ocfs2_move_extent(handle_t *handle,
  }
 
  /*
- * lock allocators, and reserving appropriate number of bits for
- * meta blocks and data clusters.
- *
- * in some cases, we don't need to reserve clusters, just let data_ac
- * be NULL.
+ * lock allocator, and reserve appropriate number of bits for
+ * meta blocks.
   */
-static int ocfs2_lock_allocators_move_extents(struct inode *inode,
+static int ocfs2_lock_meta_allocator_move_extents(struct inode *inode,
  					struct ocfs2_extent_tree *et,
  					u32 clusters_to_move,
  					u32 extents_to_split,
  					struct ocfs2_alloc_context **meta_ac,
-					struct ocfs2_alloc_context **data_ac,
  					int extra_blocks,
  					int *credits)
  {

@@ -192,13 +188,6 @@ static int ocfs2_lock_allocators_move_extents(struct inode *inode,
  		goto out;
  	}
 
-	if (data_ac) {
-		ret = ocfs2_reserve_clusters(osb, clusters_to_move, data_ac);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
 
  	*credits += ocfs2_calc_extend_credits(osb->sb, et->et_root_el);
 

@@ -257,10 +246,10 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, *len, 1,
-						 &context->meta_ac,
-						 &context->data_ac,
-						 extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						*len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;

@@ -283,6 +272,21 @@ static int ocfs2_defrag_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
+	/*
+	 * Make sure ocfs2_reserve_cluster is called after
+	 * __ocfs2_flush_truncate_log, otherwise, dead lock may happen.
+	 *
+	 * If ocfs2_reserve_cluster is called
+	 * before __ocfs2_flush_truncate_log, dead lock on global bitmap
+	 * may happen.
+	 *
+	 */
+	ret = ocfs2_reserve_clusters(osb, *len, &context->data_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto out_unlock_mutex;
+	}
+
  	handle = ocfs2_start_trans(osb, credits);
  	if (IS_ERR(handle)) {
  		ret = PTR_ERR(handle);

@@ -600,9 +604,10 @@ static int ocfs2_move_extent(struct ocfs2_move_extents_context *context,
  		}
  	}
 
-	ret = ocfs2_lock_allocators_move_extents(inode, &context->et, len, 1,
-						 &context->meta_ac,
-						 NULL, extra_blocks, &credits);
+	ret = ocfs2_lock_meta_allocator_move_extents(inode, &context->et,
+						len, 1,
+						&context->meta_ac,
+						extra_blocks, &credits);
  	if (ret) {
  		mlog_errno(ret);
  		goto out;