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

[Chao Yu]

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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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,