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/[email protected]/
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
Remove ext4_set_bit_atomic and ext4_clear_bit_atomic which are defined but not
used.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Ritesh Harjani (IBM) <[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 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
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 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
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]>
Reviewed-by: Ritesh Harjani (IBM) <[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 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
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]>
Reviewed-by: Ritesh Harjani (IBM) <[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 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
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(-)
>
> 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
on 8/2/2023 9:18 AM, Ritesh Harjani wrote:
> 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(-)
>>
>> 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/[email protected]/
--
Best wishes
Kemeng Shi
Ritesh Harjani (IBM) <[email protected]> writes:
> 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(-)
>>
>> 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/[email protected]/
Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <[email protected]>
>
> -ritesh