Received-SPF: pass (google.com: domain of linux-ext4+bounces-1776-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 29 Mar 2024 12:44:04 +0530
From: Ojaswin Mujoo <ojaswin@linux.ibm.com>
To: Kemeng Shi <shikemeng@huaweicloud.com>
Cc: tytso@mit.edu, adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
        linux-kernel@vger.kernel.org, jack@suse.cz, ritesh.list@gmail.com
Subject: Re: [PATCH 5/5] ext4: expand next_linear_group to remove repeat
 check for linear scan.
Message-ID: <ZgZqPJbEBG09dzSh@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <20240326213823.528302-1-shikemeng@huaweicloud.com>
 <20240326213823.528302-6-shikemeng@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326213823.528302-6-shikemeng@huaweicloud.com>

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
>