[Ocfs2-devel] [PATCH v2] ocfs2: try to reuse extent block in dealloc without meta_alloc

[Changwei Ge]

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 at 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);
>