Re: [PATCH 5/5] ext4: expand next_linear_group to remove repeat check for linear scan.

[Kemeng Shi]

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
>>
> 
> 
>