Date:   Sun, 5 Feb 2023 19:41:08 -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: <Y+B21EAHfuw2vyJ6@google.com>
References: <20230131144701.34418-1-chao@kernel.org>
 <Y9lkeVCMRA9goN2E@google.com>
 <a27b1135-757f-9609-1511-7c6738887472@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a27b1135-757f-9609-1511-7c6738887472@kernel.org>
Precedence: bulk

On 02/01, Chao Yu wrote:
> On 2023/2/1 2:56, Jaegeuk Kim wrote:
> > I wanted to avoid a deep if/else statement.
> 
> So how about this:

Nothing impressive.

> 
> From 418b408420367ac5491c97a7c4d26e3d0e68ea57 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao@kernel.org>
> Date: Tue, 31 Jan 2023 22:46:59 +0800
> Subject: [PATCH v2] f2fs: clean up __update_extent_tree_range()
> 
> Introduce __update_read_extent_cache() and __update_age_extent_cache()
> to clean up __update_extent_tree_range(), no logic change.
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> v2
> - introduce __update_read_extent_cache() and __update_age_extent_cache()
> to avoid a deep if/else statement in __update_extent_tree_range().
>  fs/f2fs/extent_cache.c | 116 +++++++++++++++++++++++++++--------------
>  1 file changed, 77 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index d70ad6a44cbf..887b0b2898b9 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -666,6 +666,75 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi,
>  	return en;
>  }
> 
> +static inline bool __update_read_extent_cache(struct inode *inode,
> +					struct extent_info *tei,
> +					struct extent_info *ei,
> +					struct extent_info *dei,
> +					struct extent_info *prev,
> +					unsigned int fofs, unsigned int len,
> +					struct extent_node *prev_en,
> +					struct extent_node *next_en,
> +					struct rb_node **insert_p,
> +					struct rb_node *insert_parent,
> +					bool leftmost)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_READ];
> +
> +	if (!tei->blk)
> +		return false;
> +
> +	__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 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;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static inline void __update_age_extent_cache(struct inode *inode,
> +					struct extent_info *tei,
> +					struct extent_info *ei,
> +					unsigned int fofs, unsigned int len,
> +					struct extent_node *prev_en,
> +					struct extent_node *next_en,
> +					struct rb_node **insert_p,
> +					struct rb_node *insert_parent,
> +					bool leftmost)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> +	struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_BLOCK_AGE];
> +
> +	if (!tei->last_blocks)
> +		return;
> +
> +	__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);
> +}
> +
>  static void __update_extent_tree_range(struct inode *inode,
>  			struct extent_info *tei, enum extent_type type)
>  {
> @@ -782,47 +851,16 @@ 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)
> +		updated = __update_read_extent_cache(inode, tei, &ei, &dei,
> +					&prev, fofs, len, prev_en, next_en,
>  					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);
> -		}
> -	}
> -
> -	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;
> -
> -	__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,
> +	else if (type == EX_BLOCK_AGE)
> +		__update_age_extent_cache(inode, tei, &ei,
> +					fofs, len, prev_en, next_en,
>  					insert_p, insert_parent, leftmost);
> -out_read_extent_cache:
> +
>  	write_unlock(&et->lock);
> 
>  	if (updated)
> -- 
> 2.25.1
> 
> Thanks,