public inbox for linux-ext4@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 00/10] A few fixes and cleanups to mballoc
@ 2023-08-01 14:31 Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 01/10] ext4: correct grp validation in ext4_mb_good_group Kemeng Shi
                   ` (9 more replies)
  0 siblings, 10 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

v2->v3:
-Correct comment of patch 10 on top of [1]. Remove the RVB from patch
10 as it changed.
-Collect RVG from Ritesh to Patch 1.
v1->v2:
Collect review-by from Ritesh and do improve as Ritesh suggested:
-Keep checks inside unlikely() in patch 1
-Add missed fixes tags in patch 1, 2 and 10
-Fix typo, fix conflic and kill one more return in patch 5
 
Hi all, this series contains some random fixes and cleanups to mballoc
which include correct grp validation, fix data overflow and so on.
More details can be found in respective patches.
Besides, 'kvm-xfstest smoke' runs successfully without error.

Thanks!

[1] https://lore.kernel.org/linux-ext4/20230630085927.140137-1-ojaswin@linux.ibm.com/

Kemeng Shi (10):
  ext4: correct grp validation in ext4_mb_good_group
  ext4: avoid potential data overflow in next_linear_group
  ext4: return found group directly in
    ext4_mb_choose_next_group_p2_aligned
  ext4: use is_power_of_2 helper in ext4_mb_regular_allocator
  ext4: remove unnecessary return for void function
  ext4: replace the traditional ternary conditional operator with with
    max()/min()
  ext4: remove unused ext4_{set}/{clear}_bit_atomic
  ext4: return found group directly in
    ext4_mb_choose_next_group_goal_fast
  ext4: return found group directly in
    ext4_mb_choose_next_group_best_avail
  ext4: correct some stale comment of criteria

 fs/ext4/ext4.h    |  2 --
 fs/ext4/mballoc.c | 89 ++++++++++++++++++-----------------------------
 2 files changed, 33 insertions(+), 58 deletions(-)

-- 
2.30.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 01/10] ext4: correct grp validation in ext4_mb_good_group
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
@ 2023-08-01 14:31 ` Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 02/10] ext4: avoid potential data overflow in next_linear_group Kemeng Shi
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Group corruption check will access memory of grp and will trigger kernel
crash if grp is NULL. So do NULL check before corruption check.

Fixes: 5354b2af3406 ("ext4: allow ext4_get_group_info() to fail")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 3e34e37392fb..556b7cd27f76 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2553,7 +2553,7 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac,
 
 	BUG_ON(cr < CR_POWER2_ALIGNED || cr >= EXT4_MB_NUM_CRS);
 
-	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp) || !grp))
+	if (unlikely(!grp || EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
 		return false;
 
 	free = grp->bb_free;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 02/10] ext4: avoid potential data overflow in next_linear_group
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 01/10] ext4: correct grp validation in ext4_mb_good_group Kemeng Shi
@ 2023-08-01 14:31 ` Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 03/10] ext4: return found group directly in ext4_mb_choose_next_group_p2_aligned Kemeng Shi
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

ngroups is ext4_group_t (unsigned int) while next_linear_group treat it
in int. If ngroups is bigger than max number described by int, it will
be treat as a negative number. Then "return group + 1 >= ngroups ? 0 :
group + 1;" may keep returning 0.
Switch int to ext4_group_t in next_linear_group to fix the overflow.

Fixes: 196e402adf2e ("ext4: improve cr 0 / cr 1 group scanning")
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 556b7cd27f76..a6635cba291a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1080,8 +1080,9 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac)
  * Return next linear group for allocation. If linear traversal should not be
  * performed, this function just returns the same group
  */
-static int
-next_linear_group(struct ext4_allocation_context *ac, int group, int ngroups)
+static ext4_group_t
+next_linear_group(struct ext4_allocation_context *ac, ext4_group_t group,
+		  ext4_group_t ngroups)
 {
 	if (!should_optimize_scan(ac))
 		goto inc_and_return;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 03/10] ext4: return found group directly in ext4_mb_choose_next_group_p2_aligned
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 01/10] ext4: correct grp validation in ext4_mb_good_group Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 02/10] ext4: avoid potential data overflow in next_linear_group Kemeng Shi
@ 2023-08-01 14:31 ` Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 04/10] ext4: use is_power_of_2 helper in ext4_mb_regular_allocator Kemeng Shi
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Return good group when it's found in loop to remove unnecessary NULL
initialization of grp and futher check if good group is found after loop.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a6635cba291a..4cbbe930e010 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -874,7 +874,7 @@ static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context
 			enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
-	struct ext4_group_info *iter, *grp;
+	struct ext4_group_info *iter;
 	int i;
 
 	if (ac->ac_status == AC_STATUS_FOUND)
@@ -883,7 +883,6 @@ static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context
 	if (unlikely(sbi->s_mb_stats && ac->ac_flags & EXT4_MB_CR_POWER2_ALIGNED_OPTIMIZED))
 		atomic_inc(&sbi->s_bal_p2_aligned_bad_suggestions);
 
-	grp = NULL;
 	for (i = ac->ac_2order; i < MB_NUM_ORDERS(ac->ac_sb); i++) {
 		if (list_empty(&sbi->s_mb_largest_free_orders[i]))
 			continue;
@@ -892,28 +891,22 @@ static void ext4_mb_choose_next_group_p2_aligned(struct ext4_allocation_context
 			read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
 			continue;
 		}
-		grp = NULL;
 		list_for_each_entry(iter, &sbi->s_mb_largest_free_orders[i],
 				    bb_largest_free_order_node) {
 			if (sbi->s_mb_stats)
 				atomic64_inc(&sbi->s_bal_cX_groups_considered[CR_POWER2_ALIGNED]);
 			if (likely(ext4_mb_good_group(ac, iter->bb_group, CR_POWER2_ALIGNED))) {
-				grp = iter;
-				break;
+				*group = iter->bb_group;
+				ac->ac_flags |= EXT4_MB_CR_POWER2_ALIGNED_OPTIMIZED;
+				read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
+				return;
 			}
 		}
 		read_unlock(&sbi->s_mb_largest_free_orders_locks[i]);
-		if (grp)
-			break;
 	}
 
-	if (!grp) {
-		/* Increment cr and search again */
-		*new_cr = CR_GOAL_LEN_FAST;
-	} else {
-		*group = grp->bb_group;
-		ac->ac_flags |= EXT4_MB_CR_POWER2_ALIGNED_OPTIMIZED;
-	}
+	/* Increment cr and search again if no group is found */
+	*new_cr = CR_GOAL_LEN_FAST;
 }
 
 /*
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 04/10] ext4: use is_power_of_2 helper in ext4_mb_regular_allocator
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (2 preceding siblings ...)
  2023-08-01 14:31 ` [PATCH v3 03/10] ext4: return found group directly in ext4_mb_choose_next_group_p2_aligned Kemeng Shi
@ 2023-08-01 14:31 ` Kemeng Shi
  2023-08-01 14:31 ` [PATCH v3 05/10] ext4: remove unnecessary return for void function Kemeng Shi
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Use intuitive is_power_of_2 helper in ext4_mb_regular_allocator.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 4cbbe930e010..acf8e3183e47 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2799,10 +2799,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 	 * requests upto maximum buddy size we have constructed.
 	 */
 	if (i >= sbi->s_mb_order2_reqs && i <= MB_NUM_ORDERS(sb)) {
-		/*
-		 * This should tell if fe_len is exactly power of 2
-		 */
-		if ((ac->ac_g_ex.fe_len & (~(1 << (i - 1)))) == 0)
+		if (is_power_of_2(ac->ac_g_ex.fe_len))
 			ac->ac_2order = array_index_nospec(i - 1,
 							   MB_NUM_ORDERS(sb));
 	}
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 05/10] ext4: remove unnecessary return for void function
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (3 preceding siblings ...)
  2023-08-01 14:31 ` [PATCH v3 04/10] ext4: use is_power_of_2 helper in ext4_mb_regular_allocator Kemeng Shi
@ 2023-08-01 14:31 ` Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 06/10] ext4: replace the traditional ternary conditional operator with with max()/min() Kemeng Shi
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:31 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

The return at end of void function is unnecessary, just remove it.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index acf8e3183e47..2f26a91595ca 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4876,7 +4876,6 @@ static void ext4_mb_generate_from_freelist(struct super_block *sb, void *bitmap,
 		mb_set_bits(bitmap, entry->efd_start_cluster, entry->efd_count);
 		n = rb_next(n);
 	}
-	return;
 }
 
 /*
@@ -5630,12 +5629,10 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
 #else
 static inline void ext4_mb_show_pa(struct super_block *sb)
 {
-	return;
 }
 static inline void ext4_mb_show_ac(struct ext4_allocation_context *ac)
 {
 	ext4_mb_show_pa(ac->ac_sb);
-	return;
 }
 #endif
 
@@ -5876,12 +5873,9 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
 	spin_unlock(&lg->lg_prealloc_lock);
 
 	/* Now trim the list to be not more than 8 elements */
-	if (lg_prealloc_count > 8) {
+	if (lg_prealloc_count > 8)
 		ext4_mb_discard_lg_preallocations(sb, lg,
 						  order, lg_prealloc_count);
-		return;
-	}
-	return ;
 }
 
 /*
@@ -6535,7 +6529,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 error_return:
 	brelse(bitmap_bh);
 	ext4_std_error(sb, err);
-	return;
 }
 
 /**
@@ -6638,7 +6631,6 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 	}
 
 	ext4_mb_clear_bb(handle, inode, block, count, flags);
-	return;
 }
 
 /**
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 06/10] ext4: replace the traditional ternary conditional operator with with max()/min()
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (4 preceding siblings ...)
  2023-08-01 14:31 ` [PATCH v3 05/10] ext4: remove unnecessary return for void function Kemeng Shi
@ 2023-08-01 14:32 ` Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 07/10] ext4: remove unused ext4_{set}/{clear}_bit_atomic Kemeng Shi
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Replace the traditional ternary conditional operator with with max()/min()

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2f26a91595ca..35b7a0ba25a3 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6820,8 +6820,7 @@ __releases(ext4_group_lock_ptr(sb, e4b->bd_group))
 	void *bitmap;
 
 	bitmap = e4b->bd_bitmap;
-	start = (e4b->bd_info->bb_first_free > start) ?
-		e4b->bd_info->bb_first_free : start;
+	start = max(e4b->bd_info->bb_first_free, start);
 	count = 0;
 	free_count = 0;
 
@@ -7038,8 +7037,7 @@ ext4_mballoc_query_range(
 
 	ext4_lock_group(sb, group);
 
-	start = (e4b.bd_info->bb_first_free > start) ?
-		e4b.bd_info->bb_first_free : start;
+	start = max(e4b.bd_info->bb_first_free, start);
 	if (end >= EXT4_CLUSTERS_PER_GROUP(sb))
 		end = EXT4_CLUSTERS_PER_GROUP(sb) - 1;
 
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 07/10] ext4: remove unused ext4_{set}/{clear}_bit_atomic
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (5 preceding siblings ...)
  2023-08-01 14:32 ` [PATCH v3 06/10] ext4: replace the traditional ternary conditional operator with with max()/min() Kemeng Shi
@ 2023-08-01 14:32 ` Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 08/10] ext4: return found group directly in ext4_mb_choose_next_group_goal_fast Kemeng Shi
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Remove ext4_set_bit_atomic and ext4_clear_bit_atomic which are defined but not
used.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/ext4.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ea87850c1bb6..a9b1eeb6bf09 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1249,10 +1249,8 @@ struct ext4_inode_info {
 
 #define ext4_test_and_set_bit		__test_and_set_bit_le
 #define ext4_set_bit			__set_bit_le
-#define ext4_set_bit_atomic		ext2_set_bit_atomic
 #define ext4_test_and_clear_bit		__test_and_clear_bit_le
 #define ext4_clear_bit			__clear_bit_le
-#define ext4_clear_bit_atomic		ext2_clear_bit_atomic
 #define ext4_test_bit			test_bit_le
 #define ext4_find_next_zero_bit		find_next_zero_bit_le
 #define ext4_find_next_bit		find_next_bit_le
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 08/10] ext4: return found group directly in ext4_mb_choose_next_group_goal_fast
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (6 preceding siblings ...)
  2023-08-01 14:32 ` [PATCH v3 07/10] ext4: remove unused ext4_{set}/{clear}_bit_atomic Kemeng Shi
@ 2023-08-01 14:32 ` Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 09/10] ext4: return found group directly in ext4_mb_choose_next_group_best_avail Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 10/10] ext4: correct some stale comment of criteria Kemeng Shi
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Return good group when it's found in loop to remove futher check if good
group is found after loop.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 35b7a0ba25a3..e9e37ffe5c10 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -959,16 +959,14 @@ static void ext4_mb_choose_next_group_goal_fast(struct ext4_allocation_context *
 	for (i = mb_avg_fragment_size_order(ac->ac_sb, ac->ac_g_ex.fe_len);
 	     i < MB_NUM_ORDERS(ac->ac_sb); i++) {
 		grp = ext4_mb_find_good_group_avg_frag_lists(ac, i);
-		if (grp)
-			break;
+		if (grp) {
+			*group = grp->bb_group;
+			ac->ac_flags |= EXT4_MB_CR_GOAL_LEN_FAST_OPTIMIZED;
+			return;
+		}
 	}
 
-	if (grp) {
-		*group = grp->bb_group;
-		ac->ac_flags |= EXT4_MB_CR_GOAL_LEN_FAST_OPTIMIZED;
-	} else {
-		*new_cr = CR_BEST_AVAIL_LEN;
-	}
+	*new_cr = CR_BEST_AVAIL_LEN;
 }
 
 /*
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 09/10] ext4: return found group directly in ext4_mb_choose_next_group_best_avail
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (7 preceding siblings ...)
  2023-08-01 14:32 ` [PATCH v3 08/10] ext4: return found group directly in ext4_mb_choose_next_group_goal_fast Kemeng Shi
@ 2023-08-01 14:32 ` Kemeng Shi
  2023-08-01 14:32 ` [PATCH v3 10/10] ext4: correct some stale comment of criteria Kemeng Shi
  9 siblings, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Return good group when it's found in loop to remove futher check if good
group is found after loop.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e9e37ffe5c10..bfaab173a3f4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1042,18 +1042,16 @@ static void ext4_mb_choose_next_group_best_avail(struct ext4_allocation_context
 							ac->ac_g_ex.fe_len);
 
 		grp = ext4_mb_find_good_group_avg_frag_lists(ac, frag_order);
-		if (grp)
-			break;
+		if (grp) {
+			*group = grp->bb_group;
+			ac->ac_flags |= EXT4_MB_CR_BEST_AVAIL_LEN_OPTIMIZED;
+			return;
+		}
 	}
 
-	if (grp) {
-		*group = grp->bb_group;
-		ac->ac_flags |= EXT4_MB_CR_BEST_AVAIL_LEN_OPTIMIZED;
-	} else {
-		/* Reset goal length to original goal length before falling into CR_GOAL_LEN_SLOW */
-		ac->ac_g_ex.fe_len = ac->ac_orig_goal_len;
-		*new_cr = CR_GOAL_LEN_SLOW;
-	}
+	/* Reset goal length to original goal length before falling into CR_GOAL_LEN_SLOW */
+	ac->ac_g_ex.fe_len = ac->ac_orig_goal_len;
+	*new_cr = CR_GOAL_LEN_SLOW;
 }
 
 static inline int should_optimize_scan(struct ext4_allocation_context *ac)
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 10/10] ext4: correct some stale comment of criteria
  2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
                   ` (8 preceding siblings ...)
  2023-08-01 14:32 ` [PATCH v3 09/10] ext4: return found group directly in ext4_mb_choose_next_group_best_avail Kemeng Shi
@ 2023-08-01 14:32 ` Kemeng Shi
  2023-08-02  1:18   ` Ritesh Harjani
  9 siblings, 1 reply; 14+ messages in thread
From: Kemeng Shi @ 2023-08-01 14:32 UTC (permalink / raw)
  To: tytso, adilger.kernel, ritesh.list, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

We named criteria with CR_XXX, correct stale comment to criteria with
raw number.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index bfaab173a3f4..1e8ce0ece47a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2782,8 +2782,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 
 	/*
 	 * ac->ac_2order is set only if the fe_len is a power of 2
-	 * if ac->ac_2order is set we also set criteria to 0 so that we
-	 * try exact allocation using buddy.
+	 * if ac->ac_2order is set we also set criteria to CR_POWER2_ALIGNED
+	 * so that we try exact allocation using buddy.
 	 */
 	i = fls(ac->ac_g_ex.fe_len);
 	ac->ac_2order = 0;
@@ -2840,8 +2840,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
 			/*
 			 * Batch reads of the block allocation bitmaps
 			 * to get multiple READs in flight; limit
-			 * prefetching at cr=0/1, otherwise mballoc can
-			 * spend a lot of time loading imperfect groups
+			 * prefetching at inexpensive CR, otherwise mballoc
+			 * can spend a lot of time loading imperfect groups
 			 */
 			if ((prefetch_grp == group) &&
 			    (ext4_mb_cr_expensive(cr) ||
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 10/10] ext4: correct some stale comment of criteria
  2023-08-01 14:32 ` [PATCH v3 10/10] ext4: correct some stale comment of criteria Kemeng Shi
@ 2023-08-02  1:18   ` Ritesh Harjani
  2023-08-02  1:25     ` Ritesh Harjani
  2023-08-02  1:25     ` Kemeng Shi
  0 siblings, 2 replies; 14+ messages in thread
From: Ritesh Harjani @ 2023-08-02  1:18 UTC (permalink / raw)
  To: Kemeng Shi, tytso, adilger.kernel, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Kemeng Shi <shikemeng@huaweicloud.com> writes:

> We named criteria with CR_XXX, correct stale comment to criteria with
> raw number.
>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index bfaab173a3f4..1e8ce0ece47a 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2782,8 +2782,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  
>  	/*
>  	 * ac->ac_2order is set only if the fe_len is a power of 2
> -	 * if ac->ac_2order is set we also set criteria to 0 so that we
> -	 * try exact allocation using buddy.
> +	 * if ac->ac_2order is set we also set criteria to CR_POWER2_ALIGNED
> +	 * so that we try exact allocation using buddy.
>  	 */
>  	i = fls(ac->ac_g_ex.fe_len);
>  	ac->ac_2order = 0;
> @@ -2840,8 +2840,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  			/*
>  			 * Batch reads of the block allocation bitmaps
>  			 * to get multiple READs in flight; limit
> -			 * prefetching at cr=0/1, otherwise mballoc can
> -			 * spend a lot of time loading imperfect groups
> +			 * prefetching at inexpensive CR, otherwise mballoc
> +			 * can spend a lot of time loading imperfect groups
>  			 */
>  			if ((prefetch_grp == group) &&
>  			    (ext4_mb_cr_expensive(cr) ||
Is this function defined at any place ^^^

-ritesh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 10/10] ext4: correct some stale comment of criteria
  2023-08-02  1:18   ` Ritesh Harjani
@ 2023-08-02  1:25     ` Ritesh Harjani
  2023-08-02  1:25     ` Kemeng Shi
  1 sibling, 0 replies; 14+ messages in thread
From: Ritesh Harjani @ 2023-08-02  1:25 UTC (permalink / raw)
  To: Kemeng Shi, tytso, adilger.kernel, ojaswin, linux-ext4,
	linux-kernel
  Cc: shikemeng

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Kemeng Shi <shikemeng@huaweicloud.com> writes:
>
>> We named criteria with CR_XXX, correct stale comment to criteria with
>> raw number.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/mballoc.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index bfaab173a3f4..1e8ce0ece47a 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -2782,8 +2782,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>>  
>>  	/*
>>  	 * ac->ac_2order is set only if the fe_len is a power of 2
>> -	 * if ac->ac_2order is set we also set criteria to 0 so that we
>> -	 * try exact allocation using buddy.
>> +	 * if ac->ac_2order is set we also set criteria to CR_POWER2_ALIGNED
>> +	 * so that we try exact allocation using buddy.
>>  	 */
>>  	i = fls(ac->ac_g_ex.fe_len);
>>  	ac->ac_2order = 0;
>> @@ -2840,8 +2840,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>>  			/*
>>  			 * Batch reads of the block allocation bitmaps
>>  			 * to get multiple READs in flight; limit
>> -			 * prefetching at cr=0/1, otherwise mballoc can
>> -			 * spend a lot of time loading imperfect groups
>> +			 * prefetching at inexpensive CR, otherwise mballoc
>> +			 * can spend a lot of time loading imperfect groups
>>  			 */
>>  			if ((prefetch_grp == group) &&
>>  			    (ext4_mb_cr_expensive(cr) ||
> Is this function defined at any place ^^^

aah ok got it. Here [1]

...which you have also mentioned in your cover letetr. So your patch series
currently is depenedent of above patch [1]

[1]: https://lore.kernel.org/linux-ext4/20230630085927.140137-1-ojaswin@linux.ibm.com/


Feel free to add - 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


>
> -ritesh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 10/10] ext4: correct some stale comment of criteria
  2023-08-02  1:18   ` Ritesh Harjani
  2023-08-02  1:25     ` Ritesh Harjani
@ 2023-08-02  1:25     ` Kemeng Shi
  1 sibling, 0 replies; 14+ messages in thread
From: Kemeng Shi @ 2023-08-02  1:25 UTC (permalink / raw)
  To: Ritesh Harjani, tytso, adilger.kernel, ojaswin, linux-ext4,
	linux-kernel



on 8/2/2023 9:18 AM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@huaweicloud.com> writes:
> 
>> We named criteria with CR_XXX, correct stale comment to criteria with
>> raw number.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/mballoc.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index bfaab173a3f4..1e8ce0ece47a 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -2782,8 +2782,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>>  
>>  	/*
>>  	 * ac->ac_2order is set only if the fe_len is a power of 2
>> -	 * if ac->ac_2order is set we also set criteria to 0 so that we
>> -	 * try exact allocation using buddy.
>> +	 * if ac->ac_2order is set we also set criteria to CR_POWER2_ALIGNED
>> +	 * so that we try exact allocation using buddy.
>>  	 */
>>  	i = fls(ac->ac_g_ex.fe_len);
>>  	ac->ac_2order = 0;
>> @@ -2840,8 +2840,8 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>>  			/*
>>  			 * Batch reads of the block allocation bitmaps
>>  			 * to get multiple READs in flight; limit
>> -			 * prefetching at cr=0/1, otherwise mballoc can
>> -			 * spend a lot of time loading imperfect groups
>> +			 * prefetching at inexpensive CR, otherwise mballoc
>> +			 * can spend a lot of time loading imperfect groups
>>  			 */
>>  			if ((prefetch_grp == group) &&
>>  			    (ext4_mb_cr_expensive(cr) ||
> Is this function defined at any place ^^^
> 
> -ritesh
> 
Hi Ritesh, sorry for the bother. I actually menthioned this in v2->v3 change in cover
letter that patch 10 is on top of [1] which has reviewed before.

[1] https://lore.kernel.org/linux-ext4/20230630085927.140137-1-ojaswin@linux.ibm.com/
-- 
Best wishes
Kemeng Shi


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-08-02  1:26 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-01 14:31 [PATCH v3 00/10] A few fixes and cleanups to mballoc Kemeng Shi
2023-08-01 14:31 ` [PATCH v3 01/10] ext4: correct grp validation in ext4_mb_good_group Kemeng Shi
2023-08-01 14:31 ` [PATCH v3 02/10] ext4: avoid potential data overflow in next_linear_group Kemeng Shi
2023-08-01 14:31 ` [PATCH v3 03/10] ext4: return found group directly in ext4_mb_choose_next_group_p2_aligned Kemeng Shi
2023-08-01 14:31 ` [PATCH v3 04/10] ext4: use is_power_of_2 helper in ext4_mb_regular_allocator Kemeng Shi
2023-08-01 14:31 ` [PATCH v3 05/10] ext4: remove unnecessary return for void function Kemeng Shi
2023-08-01 14:32 ` [PATCH v3 06/10] ext4: replace the traditional ternary conditional operator with with max()/min() Kemeng Shi
2023-08-01 14:32 ` [PATCH v3 07/10] ext4: remove unused ext4_{set}/{clear}_bit_atomic Kemeng Shi
2023-08-01 14:32 ` [PATCH v3 08/10] ext4: return found group directly in ext4_mb_choose_next_group_goal_fast Kemeng Shi
2023-08-01 14:32 ` [PATCH v3 09/10] ext4: return found group directly in ext4_mb_choose_next_group_best_avail Kemeng Shi
2023-08-01 14:32 ` [PATCH v3 10/10] ext4: correct some stale comment of criteria Kemeng Shi
2023-08-02  1:18   ` Ritesh Harjani
2023-08-02  1:25     ` Ritesh Harjani
2023-08-02  1:25     ` Kemeng Shi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox