Expand next_linear_group to remove repat check for linear scan.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 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) {
 		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

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@huaweicloud.com>
> ---
>  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. 

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
>

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@huaweicloud.com>
>> ---
>>  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
>>
> 
> 
>