[1/3] f2fs: clean up __update_extent_tree_range()

  No logic change, just avoid goto statement.

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 35 deletions(-)
  

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
  

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Tue, 31 Jan 2023 22:46:59 +0800 you wrote:
> No logic change, just avoid goto statement.
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 35 deletions(-)

Here is the summary with links:
  - [f2fs-dev,1/3] f2fs: clean up __update_extent_tree_range()
    (no matching commit)
  - [f2fs-dev,2/3] f2fs: fix to update age extent correctly during truncation
    https://git.kernel.org/jaegeuk/f2fs/c/334ce4a79c9e
  - [f2fs-dev,3/3] f2fs: fix to update age extent in f2fs_do_zero_range()
    https://git.kernel.org/jaegeuk/f2fs/c/de6b3a5e09b2

You are awesome, thank you!
  

On 2023/2/1 2:56, Jaegeuk Kim wrote:
> I wanted to avoid a deep if/else statement.

So how about this:

 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)
  

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,
  

On 2023/2/6 11:41, Jaegeuk Kim wrote:
> 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.

Oops...

Thanks,