Date:   Tue, 31 Jan 2023 10:56:57 -0800
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <chao@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] f2fs: clean up __update_extent_tree_range()
Message-ID: <Y9lkeVCMRA9goN2E@google.com>
References: <20230131144701.34418-1-chao@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230131144701.34418-1-chao@kernel.org>
Precedence: bulk

On 01/31, Chao Yu wrote:
> No logic change, just avoid goto statement.

I wanted to avoid a deep if/else statement.

> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index d70ad6a44cbf..cf65a188d112 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -782,47 +782,43 @@ static void __update_extent_tree_range(struct inode *inode,
>  		en = next_en;
>  	}
>  
> -	if (type == EX_BLOCK_AGE)
> -		goto update_age_extent_cache;
> -
> -	/* 3. update extent in read extent cache */
> -	BUG_ON(type != EX_READ);
> -
> -	if (tei->blk) {
> -		__set_extent_info(&ei, fofs, len, tei->blk, false,
> -				  0, 0, EX_READ);
> -		if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
> -			__insert_extent_tree(sbi, et, &ei,
> +	/* 3. update extent in extent cache */
> +	if (type == EX_READ) {
> +		if (tei->blk) {
> +			__set_extent_info(&ei, fofs, len, tei->blk, false,
> +					  0, 0, EX_READ);
> +			if (!__try_merge_extent_node(sbi, et, &ei,
> +							prev_en, next_en))
> +				__insert_extent_tree(sbi, et, &ei,
>  					insert_p, insert_parent, leftmost);
>  
> -		/* give up extent_cache, if split and small updates happen */
> -		if (dei.len >= 1 &&
> -				prev.len < F2FS_MIN_EXTENT_LEN &&
> -				et->largest.len < F2FS_MIN_EXTENT_LEN) {
> -			et->largest.len = 0;
> -			et->largest_updated = true;
> -			set_inode_flag(inode, FI_NO_EXTENT);
> +			/* give up read extent cache, if split and small updates happen */
> +			if (dei.len >= 1 &&
> +					prev.len < F2FS_MIN_EXTENT_LEN &&
> +					et->largest.len < F2FS_MIN_EXTENT_LEN) {
> +				et->largest.len = 0;
> +				et->largest_updated = true;
> +				set_inode_flag(inode, FI_NO_EXTENT);
> +			}
>  		}
> -	}
> -
> -	if (is_inode_flag_set(inode, FI_NO_EXTENT))
> -		__free_extent_tree(sbi, et);
>  
> -	if (et->largest_updated) {
> -		et->largest_updated = false;
> -		updated = true;
> -	}
> -	goto out_read_extent_cache;
> -update_age_extent_cache:
> -	if (!tei->last_blocks)
> -		goto out_read_extent_cache;
> +		if (is_inode_flag_set(inode, FI_NO_EXTENT))
> +			__free_extent_tree(sbi, et);
>  
> -	__set_extent_info(&ei, fofs, len, 0, false,
> -			tei->age, tei->last_blocks, EX_BLOCK_AGE);
> -	if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
> -		__insert_extent_tree(sbi, et, &ei,
> +		if (et->largest_updated) {
> +			et->largest_updated = false;
> +			updated = true;
> +		}
> +	} else if (type == EX_BLOCK_AGE) {
> +		if (tei->last_blocks) {
> +			__set_extent_info(&ei, fofs, len, 0, false,
> +				tei->age, tei->last_blocks, EX_BLOCK_AGE);
> +			if (!__try_merge_extent_node(sbi, et, &ei,
> +							prev_en, next_en))
> +				__insert_extent_tree(sbi, et, &ei,
>  					insert_p, insert_parent, leftmost);
> -out_read_extent_cache:
> +		}
> +	}
>  	write_unlock(&et->lock);
>  
>  	if (updated)
> -- 
> 2.36.1