[v2] ocfs2: try to reuse extent block in dealloc without meta_alloc
diff mbox

Message ID 1515359275.3044.4.camel@nixnuts.net
State New
Headers show

Commit Message

John Lightsey Jan. 7, 2018, 9:07 p.m. UTC
On Fri, 2017-12-29 at 22:36 +0000, Ge Changwei wrote:
> Hi john,
> 
> Thanks for your test.
> I am on a vacation until 3, January.
> I will investigate the issue when I am back to work.
> 

>From what I can tell, the order of reclaiming blocs vs allocating new
ones just needs to be reversed for everything to work correctly.

This patch on top of yours works in my testing.

I'm not certain my test scenario is hitting ocfs2_add_branch() with
new_blocks > 1 and meta_ac != NULL though.


>From 317cbd5a7c8ee5f8f6dff3844d23d3169db990a4 Mon Sep 17 00:00:00 2001
From: John Lightsey <john@nixnuts.net>
Date: Sun, 7 Jan 2018 14:43:21 -0600
Subject: [PATCH] Reuse deallocated extents before claiming new extents.

By reusing deallocated extents before attempting to claim new
extents, this avoids a condition where the extent tree is truncated,
and then an allocation requests are made for more extents than were
originally reserved.
---
 fs/ocfs2/alloc.c | 26 +++++++++++++++++++-------
 fs/ocfs2/aops.c  |  3 +--
 2 files changed, 20 insertions(+), 9 deletions(-)

Comments

Changwei Ge Jan. 8, 2018, 12:29 a.m. UTC | #1
Hi John,

Sorry for reply you so late since too busy these days.
Thanks for your contribution for this issue.

Thanks to the reproducer you provided, I have reproduced the crash issue you
reported.
The back trace was found.
ocfs2_mark_extent_written
  ocfs2_change_extent_flag
    ocfs2_split_extent
     ocfs2_split_and_insert
      ocfs2_grow_tree
       ocfs2_add_branch
        ocfs2_create_new_meta_bhs
         ocfs2_claim_metadata

I will post my v3 patch today.
My direction to fix above crash issue has something similar to yours.
What the difference is I revised ocfs2_reuse_blk_from_dealloc() adding a
new parameter to it.

Thanks,
Changwei

On 2018/1/8 5:08, John Lightsey wrote:
> On Fri, 2017-12-29 at 22:36 +0000, Ge Changwei wrote:
>> Hi john,
>>
>> Thanks for your test.
>> I am on a vacation until 3, January.
>> I will investigate the issue when I am back to work.
>>
> 
>  From what I can tell, the order of reclaiming blocs vs allocating new
> ones just needs to be reversed for everything to work correctly.
> 
> This patch on top of yours works in my testing.
> 
> I'm not certain my test scenario is hitting ocfs2_add_branch() with
> new_blocks > 1 and meta_ac != NULL though.
> 
> 
>  From 317cbd5a7c8ee5f8f6dff3844d23d3169db990a4 Mon Sep 17 00:00:00 2001
> From: John Lightsey <john@nixnuts.net>
> Date: Sun, 7 Jan 2018 14:43:21 -0600
> Subject: [PATCH] Reuse deallocated extents before claiming new extents.
> 
> By reusing deallocated extents before attempting to claim new
> extents, this avoids a condition where the extent tree is truncated,
> and then an allocation requests are made for more extents than were
> originally reserved.
> ---
>   fs/ocfs2/alloc.c | 26 +++++++++++++++++++-------
>   fs/ocfs2/aops.c  |  3 +--
>   2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index 89807ea7..dce2eaa7 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -1166,7 +1166,7 @@ static int ocfs2_add_branch(handle_t *handle,
>   			    struct buffer_head **last_eb_bh,
>   			    struct ocfs2_alloc_context *meta_ac)
>   {
> -	int status, new_blocks, i;
> +	int status, new_blocks, reclaimed_blocks, i;
>   	u64 next_blkno, new_last_eb_blk;
>   	struct buffer_head *bh;
>   	struct buffer_head **new_eb_bhs = NULL;
> @@ -1222,8 +1222,20 @@ static int ocfs2_add_branch(handle_t *handle,
>   	}
>   
>   	if (meta_ac) {
> -		status = ocfs2_create_new_meta_bhs(handle, et, new_blocks,
> -						   meta_ac, new_eb_bhs);
> +		reclaimed_blocks = 0;
> +		while ( reclaimed_blocks < new_blocks && !ocfs2_is_dealloc_empty(et) ) {
> +			status = ocfs2_reuse_blk_from_dealloc(handle, et,
> +					new_eb_bhs + reclaimed_blocks, 1);
> +			if (status < 0) {
> +				mlog_errno(status);
> +				goto bail;
> +			}
> +			reclaimed_blocks++;
> +		}
> +		if (reclaimed_blocks < new_blocks) {
> +			status = ocfs2_create_new_meta_bhs(handle, et, new_blocks - reclaimed_blocks,
> +				meta_ac, new_eb_bhs + reclaimed_blocks);
> +		}
>   	} else if (!ocfs2_is_dealloc_empty(et)) {
>   		status = ocfs2_reuse_blk_from_dealloc(handle, et,
>   						      new_eb_bhs, new_blocks);
> @@ -1362,12 +1374,12 @@ static int ocfs2_shift_tree_depth(handle_t *handle,
>   	struct ocfs2_extent_list  *root_el;
>   	struct ocfs2_extent_list  *eb_el;
>   
> -	if (meta_ac) {
> -		status = ocfs2_create_new_meta_bhs(handle, et, 1, meta_ac,
> -						   &new_eb_bh);
> -	} else if (!ocfs2_is_dealloc_empty(et)) {
> +	if (!ocfs2_is_dealloc_empty(et)) {
>   		status = ocfs2_reuse_blk_from_dealloc(handle, et,
>   						      &new_eb_bh, 1);
> +	} else if (meta_ac) {
> +		status = ocfs2_create_new_meta_bhs(handle, et, 1, meta_ac,
> +						   &new_eb_bh);
>   	} else {
>   		BUG();
>   	}
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index dcea6d5c..8ad37965 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1743,8 +1743,7 @@ int ocfs2_write_begin_nolock(struct address_space *mapping,
>   		ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode),
>   					      wc->w_di_bh);
>   		ret = ocfs2_lock_allocators(inode, &et,
> -					    clusters_to_alloc,
> -					    2*extents_to_split,
> +					    clusters_to_alloc, extents_to_split,
>   					    &data_ac, &meta_ac);
>   		if (ret) {
>   			mlog_errno(ret);
>

Patch
diff mbox

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index 89807ea7..dce2eaa7 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -1166,7 +1166,7 @@  static int ocfs2_add_branch(handle_t *handle,
 			    struct buffer_head **last_eb_bh,
 			    struct ocfs2_alloc_context *meta_ac)
 {
-	int status, new_blocks, i;
+	int status, new_blocks, reclaimed_blocks, i;
 	u64 next_blkno, new_last_eb_blk;
 	struct buffer_head *bh;
 	struct buffer_head **new_eb_bhs = NULL;
@@ -1222,8 +1222,20 @@  static int ocfs2_add_branch(handle_t *handle,
 	}
 
 	if (meta_ac) {
-		status = ocfs2_create_new_meta_bhs(handle, et, new_blocks,
-						   meta_ac, new_eb_bhs);
+		reclaimed_blocks = 0;
+		while ( reclaimed_blocks < new_blocks && !ocfs2_is_dealloc_empty(et) ) {
+			status = ocfs2_reuse_blk_from_dealloc(handle, et,
+					new_eb_bhs + reclaimed_blocks, 1);
+			if (status < 0) {
+				mlog_errno(status);
+				goto bail;
+			}
+			reclaimed_blocks++;
+		}
+		if (reclaimed_blocks < new_blocks) {
+			status = ocfs2_create_new_meta_bhs(handle, et, new_blocks - reclaimed_blocks,
+				meta_ac, new_eb_bhs + reclaimed_blocks);
+		}
 	} else if (!ocfs2_is_dealloc_empty(et)) {
 		status = ocfs2_reuse_blk_from_dealloc(handle, et,
 						      new_eb_bhs, new_blocks);
@@ -1362,12 +1374,12 @@  static int ocfs2_shift_tree_depth(handle_t *handle,
 	struct ocfs2_extent_list  *root_el;
 	struct ocfs2_extent_list  *eb_el;
 
-	if (meta_ac) {
-		status = ocfs2_create_new_meta_bhs(handle, et, 1, meta_ac,
-						   &new_eb_bh);
-	} else if (!ocfs2_is_dealloc_empty(et)) {
+	if (!ocfs2_is_dealloc_empty(et)) {
 		status = ocfs2_reuse_blk_from_dealloc(handle, et,
 						      &new_eb_bh, 1);
+	} else if (meta_ac) {
+		status = ocfs2_create_new_meta_bhs(handle, et, 1, meta_ac,
+						   &new_eb_bh);
 	} else {
 		BUG();
 	}
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index dcea6d5c..8ad37965 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1743,8 +1743,7 @@  int ocfs2_write_begin_nolock(struct address_space *mapping,
 		ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode),
 					      wc->w_di_bh);
 		ret = ocfs2_lock_allocators(inode, &et,
-					    clusters_to_alloc,
-					    2*extents_to_split,
+					    clusters_to_alloc, extents_to_split,
 					    &data_ac, &meta_ac);
 		if (ret) {
 			mlog_errno(ret);