Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1685449706.git.ojaswin@linux.ibm.com> <ddcae9658e46880dfec2fb0aa61d01fb3353d202.1685449706.git.ojaswin@linux.ibm.com>
In-Reply-To: <ddcae9658e46880dfec2fb0aa61d01fb3353d202.1685449706.git.ojaswin@linux.ibm.com>
Reply-To: sedat.dilek@gmail.com
From:   Sedat Dilek <sedat.dilek@gmail.com>
Date:   Tue, 30 May 2023 18:28:22 +0200
Message-ID: <CA+icZUXDFbxRvx8-pvEwsZAu+-28bX4VDTj6ZTPtvn4gWqGnCg@mail.gmail.com>
Subject: Re: [PATCH v2 01/12] Revert "ext4: remove ac->ac_found >
 sbi->s_mb_min_to_scan dead check in ext4_mb_check_limits"
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc:     linux-ext4@vger.kernel.org, "Theodore Ts'o" <tytso@mit.edu>,
        Ritesh Harjani <riteshh@linux.ibm.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jan Kara <jack@suse.cz>,
        Kemeng Shi <shikemeng@huaweicloud.com>,
        Ritesh Harjani <ritesh.list@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 30, 2023 at 3:25=E2=80=AFPM Ojaswin Mujoo <ojaswin@linux.ibm.co=
m> wrote:
>
> This reverts commit 32c0869370194ae5ac9f9f501953ef693040f6a1.
>
> The reverted commit was intended to remove a dead check however it was ob=
served
> that this check was actually being used to exit early instead of looping
> sbi->s_mb_max_to_scan times when we are able to find a free extent bigger=
 than
> the goal extent. Due to this, a my performance tests (fsmark, parallel fi=
le
> writes in a highly fragmented FS) were seeing a 2x-3x regression.
>
> Example, the default value of the following variables is:
>
> sbi->s_mb_max_to_scan =3D 200
> sbi->s_mb_min_to_scan =3D 10
>
> In ext4_mb_check_limits() if we find an extent smaller than goal, then we=
 return
> early and try again. This loop will go on until we have processed
> sbi->s_mb_max_to_scan(=3D200) number of free extents at which point we ex=
it and
> just use whatever we have even if it is smaller than goal extent.
>
> Now, the regression comes when we find an extent bigger than goal. Earlie=
r, in
> this case we would loop only sbi->s_mb_min_to_scan(=3D10) times and then =
just use
> the bigger extent. However with commit 32c08693 that check was removed an=
d hence
> we would loop sbi->s_mb_max_to_scan(=3D200) times even though we have a b=
ig enough
> free extent to satisfy the request. The only time we would exit early wou=
ld be
> when the free extent is *exactly* the size of our goal, which is pretty u=
ncommon
> occurrence and so we would almost always end up looping 200 times.
>
> Hence, revert the commit by adding the check back to fix the regression. =
Also
> add a comment to outline this policy.
>

Hi,

I applied this single patch of your series v2 on top of Linux v6.4-rc4.

So, if this is a regression I ask myself if this is material for Linux 6.4?

Can you comment on this, please?

Thanks.

Regards,
-Sedat-


> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Reviewed-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index d4b6a2c1881d..7ac6d3524f29 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2063,7 +2063,7 @@ static void ext4_mb_check_limits(struct ext4_alloca=
tion_context *ac,
>         if (bex->fe_len < gex->fe_len)
>                 return;
>
> -       if (finish_group)
> +       if (finish_group || ac->ac_found > sbi->s_mb_min_to_scan)
>                 ext4_mb_use_best_found(ac, e4b);
>  }
>
> @@ -2075,6 +2075,20 @@ static void ext4_mb_check_limits(struct ext4_alloc=
ation_context *ac,
>   * in the context. Later, the best found extent will be used, if
>   * mballoc can't find good enough extent.
>   *
> + * The algorithm used is roughly as follows:
> + *
> + * * If free extent found is exactly as big as goal, then
> + *   stop the scan and use it immediately
> + *
> + * * If free extent found is smaller than goal, then keep retrying
> + *   upto a max of sbi->s_mb_max_to_scan times (default 200). After
> + *   that stop scanning and use whatever we have.
> + *
> + * * If free extent found is bigger than goal, then keep retrying
> + *   upto a max of sbi->s_mb_min_to_scan times (default 10) before
> + *   stopping the scan and using the extent.
> + *
> + *
>   * FIXME: real allocation policy is to be designed yet!
>   */
>  static void ext4_mb_measure_extent(struct ext4_allocation_context *ac,
> --
> 2.31.1
>