Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] ext4: put grp related checks into ext4_mb_good_group()
To:     brookxu <brookxu.cn@gmail.com>, adilger.kernel@dilger.ca,
        tytso@mit.edu, linux-ext4@vger.kernel.org
References: <a53fe585-2b31-3a2e-f3eb-edc6f80ad85f@gmail.com>
From:   Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Thu, 13 Aug 2020 20:48:50 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <a53fe585-2b31-3a2e-f3eb-edc6f80ad85f@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <20200813151851.5E455A404D@d06av23.portsmouth.uk.ibm.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


On 8/7/20 7:31 PM, brookxu wrote:
> We will make these judgments in ext4_mb_good_group(), maybe there
> is no need to repeat judgments here.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>

Nack. This could essentially cause performance issues.
But then maybe we should add a comment saying these extra checks are
intentionally done here without explicit ext4_lock_group() for
performance optimizations.

-ritesh


> ---
>   fs/ext4/mballoc.c | 16 ++--------------
>   1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4304113..84871f7 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2178,21 +2178,8 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
>   	struct ext4_group_info *grp = ext4_get_group_info(ac->ac_sb, group);
>   	struct super_block *sb = ac->ac_sb;
>   	bool should_lock = ac->ac_flags & EXT4_MB_STRICT_CHECK;
> -	ext4_grpblk_t free;
>   	int ret = 0;
> 
> -	if (should_lock)
> -		ext4_lock_group(sb, group);
> -	free = grp->bb_free;
> -	if (free == 0)
> -		goto out;
> -	if (cr <= 2 && free < ac->ac_g_ex.fe_len)
> -		goto out;
> -	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
> -		goto out;
> -	if (should_lock)
> -		ext4_unlock_group(sb, group);
> -
>   	/* We only do this if the grp has never been initialized */
>   	if (unlikely(EXT4_MB_GRP_NEED_INIT(grp))) {
>   		ret = ext4_mb_init_group(ac->ac_sb, group, GFP_NOFS);
> @@ -2202,8 +2189,9 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
> 
>   	if (should_lock)
>   		ext4_lock_group(sb, group);
> +
>   	ret = ext4_mb_good_group(ac, group, cr);
> -out:
> +
>   	if (should_lock)
>   		ext4_unlock_group(sb, group);
>   	return ret;
>