on 3/29/2024 3:14 PM, Ojaswin Mujoo wrote:
> On Wed, Mar 27, 2024 at 05:38:23AM +0800, Kemeng Shi wrote:
>> Expand next_linear_group to remove repat check for linear scan.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
>> ---
>> fs/ext4/mballoc.c | 37 ++++++-------------------------------
>> 1 file changed, 6 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 0f8a34513bf6..561780a274cd 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -1075,31 +1075,6 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac)
>> return 1;
>> }
>>
>> -/*
>> - * Return next linear group for allocation. If linear traversal should not be
>> - * performed, this function just returns the same group
>> - */
>> -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;
>> -
>> - if (ac->ac_groups_linear_remaining) {
>> - ac->ac_groups_linear_remaining--;
>> - goto inc_and_return;
>> - }
>> -
>> - return group;
>> -inc_and_return:
>> - /*
>> - * Artificially restricted ngroups for non-extent
>> - * files makes group > ngroups possible on first loop.
>> - */
>> - return group + 1 >= ngroups ? 0 : group + 1;
>> -}
>> -
>> /*
>> * ext4_mb_choose_next_group: choose next group for allocation.
>> *
>> @@ -1118,12 +1093,12 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac,
>> {
>> *new_cr = ac->ac_criteria;
>>
>> - if (!should_optimize_scan(ac) || ac->ac_groups_linear_remaining) {
>> - *group = next_linear_group(ac, *group, ngroups);
>> - return;
>> - }
>> -
>> - if (*new_cr == CR_POWER2_ALIGNED) {
>> + if (!should_optimize_scan(ac))
>> + *group = *group + 1 >= ngroups ? 0 : *group + 1;
>> + else if (ac->ac_groups_linear_remaining) {
>> + ac->ac_groups_linear_remaining--;
>> + *group = *group + 1 >= ngroups ? 0 : *group + 1;
>> + } else if (*new_cr == CR_POWER2_ALIGNED) {
>
>
> Hi Kemeng, thanks for the cleanups
>
> I feel that open coding this logic and having a single if for linear scan and
> non linear scan cases is making the code a bit more harder to follow and we are
> losing some comments as well.
>
> Since our main aim is to avoid the double checking, maybe we can keep
> next_linear_group() strictly for getting the next linear group correctly and
> rest of the checks outside. So something like:
>
> static ext4_group_t
> next_linear_group(ext4_group_t group, ext4_group_t ngroups)
> {
>
> /*
> * Artificially restricted ngroups for non-extent
> * files makes group > ngroups possible on first loop.
> */
> return group + 1 >= ngroups ? 0 : group + 1;
> }
>
> static void ext4_mb_choose_next_group(...)
> {
> ...
>
> /*
> * Fallback to linear scan when optimized scanning is disabled
> */
> if (!should_optimize_scan(ac)) {
> *group = next_linear_group(*group, ngroups);
> return;
> }
>
> /*
> * Optimized scanning can return non adjacent groups which can cause
> * seek overhead for rotational disks. So try few linear groups before
> * trying optimized scan.
> */
> if (ac->ac_groups_linear_remaining) {
> *group = next_linear_group(*group, ngroups);
> ac->ac_groups_linear_remaining--;
> return;
> }
>
> ...
> }
>
> Let me know your thought.
This make senses to me. I will do in next version. Thanks for the advise.
Kemeng
>
> Regards,
> ojaswin
>
>> ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group);
>> } else if (*new_cr == CR_GOAL_LEN_FAST) {
>> ext4_mb_choose_next_group_goal_fast(ac, new_cr, group);
>> --
>> 2.30.0
>>
>
>
>
