On Tue, May 30, 2023 at 06:28:22PM +0200, Sedat Dilek wrote: > On Tue, May 30, 2023 at 3:25 PM Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx> wrote: > > > > This reverts commit 32c0869370194ae5ac9f9f501953ef693040f6a1. > > > > The reverted commit was intended to remove a dead check however it was observed > > 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 file > > 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 = 200 > > sbi->s_mb_min_to_scan = 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(=200) number of free extents at which point we exit 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. Earlier, in > > this case we would loop only sbi->s_mb_min_to_scan(=10) times and then just use > > the bigger extent. However with commit 32c08693 that check was removed and hence > > we would loop sbi->s_mb_max_to_scan(=200) times even though we have a big enough > > free extent to satisfy the request. The only time we would exit early would be > > when the free extent is *exactly* the size of our goal, which is pretty uncommon > > 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- Hi Sedat, Since this patch fixes a regression I think it should ideally go in Linux 6.4 Regards, ojaswin > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx> > > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> > > Reviewed-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> > > --- > > 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_allocation_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_allocation_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 > >
