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;
Date:   Fri, 21 Jul 2023 00:52:50 +0530
Message-Id: <875y6eo0ad.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Baokun Li <libaokun1@huawei.com>, linux-ext4@vger.kernel.org
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        ojaswin@linux.ibm.com, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, yangerkun@huawei.com, yukuai3@huawei.com,
        libaokun1@huawei.com
Subject: Re: [PATCH 4/4] ext4: avoid prealloc space being skipped due to overflow
In-Reply-To: <20230718131052.283350-5-libaokun1@huawei.com>
Precedence: bulk

Baokun Li <libaokun1@huawei.com> writes:

> If there is a pa in the i_prealloc_list of an inode with a tmp_pa_end
> of 4294967296(0x100000000), since tmp_pa_end is of type ext4_lblk_t,
> tmp_pa_end will be recognized as 0 due to overflow, which causes
> (ac->ac_o_ ex.fe_logical >= tmp_pa_end) always holds, so that pa will
> always be skipped. This then triggers the regular allocation process,
> and if the excess tail of the free extent from that allocation is put
> into the i_prealloc_list again, it will again not be used. This ends up
> leaving us with a lot of free physical blocks in the i_prealloc_list.
>
> We avoid this problem by using pa_end() to compute tmp_pa_end and
> declaring tmp_pa_end to be of type loff_t.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good to me with fex_end() and pa_end() dropped.

-ritesh


> ---
>  fs/ext4/mballoc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 77d47af525d9..06db40fb29d6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4765,7 +4765,8 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
>  	struct ext4_inode_info *ei = EXT4_I(ac->ac_inode);
>  	struct ext4_locality_group *lg;
>  	struct ext4_prealloc_space *tmp_pa, *cpa = NULL;
> -	ext4_lblk_t tmp_pa_start, tmp_pa_end;
> +	ext4_lblk_t tmp_pa_start;
> +	loff_t tmp_pa_end;
>  	struct rb_node *iter;
>  	ext4_fsblk_t goal_block;
>  
> @@ -4784,7 +4785,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
>  		/* all fields in this condition don't change,
>  		 * so we can skip locking for them */
>  		tmp_pa_start = tmp_pa->pa_lstart;
> -		tmp_pa_end = tmp_pa->pa_lstart + EXT4_C2B(sbi, tmp_pa->pa_len);
> +		tmp_pa_end = pa_end(sbi, tmp_pa);
>  
>  		/* original request start doesn't lie in this PA */
>  		if (ac->ac_o_ex.fe_logical < tmp_pa_start ||
> -- 
> 2.31.1