MIME-Version: 1.0
References: <20220106024518.8161-1-yinxin.x@bytedance.com> <20220106024518.8161-3-yinxin.x@bytedance.com>
In-Reply-To: <20220106024518.8161-3-yinxin.x@bytedance.com>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Fri, 7 Jan 2022 12:30:11 -0800
Message-ID: <CAD+ocbzTrYxtt6SBmzGBK3QvP+-oAtKwy4AfpPP7V-RQCgxfiA@mail.gmail.com>
Subject: Re: [PATCH 2/2] ext4: modify the logic of ext4_mb_new_blocks_simple
To:     Xin Yin <yinxin.x@bytedance.com>
Cc:     "Theodore Y. Ts'o" <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Looks good to me.

Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

On Wed, Jan 5, 2022 at 6:45 PM Xin Yin <yinxin.x@bytedance.com> wrote:
>
> for now in ext4_mb_new_blocks_simple, if we found a block which
> should be excluded then will switch to next group, this may
> probably cause 'group' run out of range.
>
> change to check next block in the same group when get a block should
> be excluded. Also change the searche range to EXT4_CLUSTERS_PER_GROUP
> and add error checking.
>
> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
> ---
>  fs/ext4/mballoc.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 215b7068f548..31a00b473f3e 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -5757,7 +5757,8 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle,
>         struct super_block *sb = ar->inode->i_sb;
>         ext4_group_t group;
>         ext4_grpblk_t blkoff;
> -       int i = sb->s_blocksize;
> +       ext4_grpblk_t max = EXT4_CLUSTERS_PER_GROUP(sb);
> +       ext4_grpblk_t i = 0;
>         ext4_fsblk_t goal, block;
>         struct ext4_super_block *es = EXT4_SB(sb)->s_es;
>
> @@ -5779,19 +5780,26 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle,
>                 ext4_get_group_no_and_offset(sb,
>                         max(ext4_group_first_block_no(sb, group), goal),
>                         NULL, &blkoff);
> -               i = mb_find_next_zero_bit(bitmap_bh->b_data, sb->s_blocksize,
> +               while (1) {
> +                       i = mb_find_next_zero_bit(bitmap_bh->b_data, max,
>                                                 blkoff);
> +                       if (i >= max)
> +                               break;
> +                       if (ext4_fc_replay_check_excluded(sb,
> +                               ext4_group_first_block_no(sb, group) + i)) {
> +                               blkoff = i + 1;
> +                       } else
> +                               break;
> +               }
>                 brelse(bitmap_bh);
> -               if (i >= sb->s_blocksize)
> -                       continue;
> -               if (ext4_fc_replay_check_excluded(sb,
> -                       ext4_group_first_block_no(sb, group) + i))
> -                       continue;
> -               break;
> +               if (i < max)
> +                       break;
>         }
>
> -       if (group >= ext4_get_groups_count(sb) && i >= sb->s_blocksize)
> +       if (group >= ext4_get_groups_count(sb) || i >= max) {
> +               *errp = -ENOSPC;
>                 return 0;
> +       }
>
>         block = ext4_group_first_block_no(sb, group) + i;
>         ext4_mb_mark_bb(sb, block, 1, 1);
> --
> 2.20.1
>