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!
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 | 85 ++++++++++++++++++-----------------------------
2 files changed, 32 insertions(+), 55 deletions(-)
--
2.30.0
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.
Signed-off-by: Kemeng Shi <[email protected]>
---
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 90ffabac100b..33ee3991f62c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1081,8 +1081,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
The return at end of void function is nunecessary, just remove it.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/ext4/mballoc.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b838944b5f09..78160bf5b533 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4880,7 +4880,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;
}
/*
@@ -5634,12 +5633,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
@@ -5885,7 +5882,6 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
order, lg_prealloc_count);
return;
}
- return ;
}
/*
@@ -6470,7 +6466,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
}
error_return:
ext4_std_error(sb, err);
- return;
}
/**
@@ -6573,7 +6568,6 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
}
ext4_mb_clear_bb(handle, inode, block, count, flags);
- return;
}
/**
--
2.30.0
Remove ext4_set_bit_atomic and ext4_clear_bit_atomic which are defined but not
used.
Signed-off-by: Kemeng Shi <[email protected]>
---
fs/ext4/ext4.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0a2d55faa095..7166edb2e4a7 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1252,10 +1252,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
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 <[email protected]>
---
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 33ee3991f62c..4031f8e2a660 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -875,7 +875,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)
@@ -884,7 +884,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;
@@ -893,28 +892,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
Replace the traditional ternary conditional operator with with max()/min()
Signed-off-by: Kemeng Shi <[email protected]>
---
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 78160bf5b533..412d335583fe 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6699,8 +6699,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;
@@ -6917,8 +6916,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
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 <[email protected]>
---
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 6f8e804905d5..b04eceeab967 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1043,18 +1043,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
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 <[email protected]>
---
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 412d335583fe..6f8e804905d5 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -960,16 +960,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
Use intuitive is_power_of_2 helper in ext4_mb_regular_allocator.
Signed-off-by: Kemeng Shi <[email protected]>
---
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 4031f8e2a660..b838944b5f09 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2795,10 +2795,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
We named criteria with CR_XXX, correct stale comment to criteria with
raw number.
Signed-off-by: Kemeng Shi <[email protected]>
---
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 b04eceeab967..e30494f3d289 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2778,8 +2778,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;
@@ -2836,8 +2836,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 cr below CR_FAST, otherwise mballoc
+ * can spend a lot of time loading imperfect groups
*/
if ((prefetch_grp == group) &&
(cr >= CR_FAST ||
--
2.30.0
Group corruption check will access memory of grp and will trigger kernel
crash if grp is NULL. So do NULL check before corruption check.
Signed-off-by: Kemeng Shi <[email protected]>
---
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 3ab37533349f..90ffabac100b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2554,7 +2554,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 (!grp || unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
return false;
free = grp->bb_free;
--
2.30.0
Kemeng Shi <[email protected]> writes:
> 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 <[email protected]>
> ---
> 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 3ab37533349f..90ffabac100b 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2554,7 +2554,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 (!grp || unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
maybe like below?
if (unlikely(!grp || EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
-ritesh
> return false;
>
> free = grp->bb_free;
> --
> 2.30.0
Kemeng Shi <[email protected]> writes:
> 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")
With that feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> 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 90ffabac100b..33ee3991f62c 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1081,8 +1081,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
Kemeng Shi <[email protected]> writes:
> 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 <[email protected]>
> ---
> fs/ext4/mballoc.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
Makes it simpler. Thanks for the cleanup.
Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 33ee3991f62c..4031f8e2a660 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -875,7 +875,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)
> @@ -884,7 +884,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;
> @@ -893,28 +892,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
Kemeng Shi <[email protected]> writes:
> Replace the traditional ternary conditional operator with with max()/min()
sure.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/ext4/mballoc.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
Looks good to me, feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 78160bf5b533..412d335583fe 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6699,8 +6699,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;
>
> @@ -6917,8 +6916,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
Kemeng Shi <[email protected]> writes:
> Remove ext4_set_bit_atomic and ext4_clear_bit_atomic which are defined but not
> used.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/ext4/ext4.h | 2 --
> 1 file changed, 2 deletions(-)
>
Looks good to me, feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 0a2d55faa095..7166edb2e4a7 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1252,10 +1252,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
Kemeng Shi <[email protected]> writes:
> The return at end of void function is nunecessary, just remove it.
s/nunecessary/unnecessary
Note, while applying this patch series on ted's dev branch, I got a
conflict in this patch. It's eaier to resolve, however you might want to
make sure that it is cleanly applicable on a given tree in v2.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/ext4/mballoc.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index b838944b5f09..78160bf5b533 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4880,7 +4880,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;
> }
>
> /*
> @@ -5634,12 +5633,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
>
> @@ -5885,7 +5882,6 @@ static void ext4_mb_add_n_trim(struct ext4_allocation_context *ac)
> order, lg_prealloc_count);
> return;
Why not kill this ^^ return too?
> }
> - return ;
> }
>
> /*
> @@ -6470,7 +6466,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
> }
> error_return:
> ext4_std_error(sb, err);
> - return;
> }
>
> /**
> @@ -6573,7 +6568,6 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
> }
>
> ext4_mb_clear_bb(handle, inode, block, count, flags);
> - return;
> }
>
> /**
> --
> 2.30.0
With above addressed, feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
Kemeng Shi <[email protected]> writes:
> Use intuitive is_power_of_2 helper in ext4_mb_regular_allocator.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/ext4/mballoc.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
Looks good to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4031f8e2a660..b838944b5f09 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2795,10 +2795,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
Kemeng Shi <[email protected]> writes:
> 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 <[email protected]>
> ---
> fs/ext4/mballoc.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
Looks good to me, feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 412d335583fe..6f8e804905d5 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -960,16 +960,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
Kemeng Shi <[email protected]> writes:
> 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 <[email protected]>
> ---
> fs/ext4/mballoc.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
Looks good to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 6f8e804905d5..b04eceeab967 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1043,18 +1043,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
Kemeng Shi <[email protected]> writes:
> We named criteria with CR_XXX, correct stale comment to criteria with
> raw number.
>
> Signed-off-by: Kemeng Shi <[email protected]>
> ---
> fs/ext4/mballoc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Looks good to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
-ritesh
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index b04eceeab967..e30494f3d289 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2778,8 +2778,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;
> @@ -2836,8 +2836,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 cr below CR_FAST, otherwise mballoc
> + * can spend a lot of time loading imperfect groups
> */
> if ((prefetch_grp == group) &&
> (cr >= CR_FAST ||
> --
> 2.30.0
Kemeng Shi <[email protected]> writes:
> 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 Kemeng for the cleanup series. Looks good to me with few minor
nits which I have commented in individual patches.
Note that I couldn't cleanly apply the series on ted's dev branch
(Patch-05 gave some minor conflict). Maybe you might have based your
changes on top of linux master or something. Anyways it was just a minor
conflict and I don't know what is Ted's general preference here, but I
thought of doing an FYI -
-ritesh
>
> Thanks!
>
> 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 | 85 ++++++++++++++++++-----------------------------
> 2 files changed, 32 insertions(+), 55 deletions(-)
>
> --
> 2.30.0
on 7/21/2023 11:55 PM, Ritesh Harjani wrote:
> Kemeng Shi <[email protected]> writes:
>
>> 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 Kemeng for the cleanup series. Looks good to me with few minor
> nits which I have commented in individual patches.
>
Hi Ritesh, thanks for the review and comments. I went through all your
suggestions and they help a lot. I will send v2 patchset to handle these.
> Note that I couldn't cleanly apply the series on ted's dev branch
> (Patch-05 gave some minor conflict). Maybe you might have based your
> changes on top of linux master or something. Anyways it was just a minor
> conflict and I don't know what is Ted's general preference here, but I
> thought of doing an FYI -
Sorry for the conflict, this patchset is based on cleanup patchset [1]
which you have noticed. As [1] still needs a lot more review, I will
make this patchset based on current dev branch.
Thanks!
[1] https://lore.kernel.org/linux-ext4/[email protected]/T/#m66729346a76498079278df3e132a89910860f8ff
--
Best wishes
Kemeng Shi