Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Apr 2023 01:01:45 +0530
From:   Ojaswin Mujoo <ojaswin@linux.ibm.com>
To:     Kemeng Shi <shikemeng@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/8] ext4: add EXT4_MB_HINT_GOAL_ONLY test in
 ext4_mb_use_preallocated
Message-ID: <ZC3MoWn2UO6p+Swp@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <20230321161220.418652-1-shikemeng@huaweicloud.com>
 <20230321161220.418652-7-shikemeng@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321161220.418652-7-shikemeng@huaweicloud.com>
Precedence: bulk

On Wed, Mar 22, 2023 at 12:12:18AM +0800, Kemeng Shi wrote:
> ext4_mb_use_preallocated will ignore the demand to alloc goal blocks,
> although the EXT4_MB_HINT_GOAL_ONLY is requested.
> For group pa, ext4_mb_group_or_file will not set EXT4_MB_HINT_GROUP_ALLOC
> if EXT4_MB_HINT_GOAL_ONLY is set. So we will not alloc goal blocks from
> group pa if EXT4_MB_HINT_GOAL_ONLY is set.
> For inode pa, ext4_mb_pa_goal_check is added to check if free extent in
> found inode pa meets goal blocks when EXT4_MB_HINT_GOAL_ONLY is set.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Suggested-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/mballoc.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index b963111eeec6..5b837918c624 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4351,6 +4351,37 @@ ext4_mb_check_group_pa(ext4_fsblk_t goal_block,
>  	return pa;
>  }
>  
> +/*
> + * check if found pa meets EXT4_MB_HINT_GOAL_ONLY
> + */
> +static bool
> +ext4_mb_pa_goal_check(struct ext4_allocation_context *ac,
> +		      struct ext4_prealloc_space *pa)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> +	ext4_fsblk_t start;
> +
> +	if (likely(!(ac->ac_flags & EXT4_MB_HINT_GOAL_ONLY)))
> +		return true;
> +
> +	/*
> +	 * If EXT4_MB_HINT_GOAL_ONLY is set, ac_g_ex will not be adjusted
> +	 * in ext4_mb_normalize_request and will keep same with ac_o_ex
> +	 * from ext4_mb_initialize_context. Choose ac_g_ex here to keep
> +	 * consistent with ext4_mb_find_by_goal.
> +	 */
> +	start = pa->pa_pstart +
> +		(ac->ac_g_ex.fe_logical - pa->pa_lstart);
> +	if (ext4_grp_offs_to_block(ac->ac_sb, &ac->ac_g_ex) != start)
> +		return false;
> +
> +	if (ac->ac_g_ex.fe_len > pa->pa_len -
> +	    EXT4_B2C(sbi, ac->ac_g_ex.fe_logical - pa->pa_lstart))
> +		return false;
> +
> +	return true;
> +}
> +
>  /*
>   * search goal blocks in preallocated space
>   */
> @@ -4387,7 +4418,8 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)
>  
>  		/* found preallocated blocks, use them */
>  		spin_lock(&pa->pa_lock);
> -		if (pa->pa_deleted == 0 && pa->pa_free) {
> +		if (pa->pa_deleted == 0 && pa->pa_free &&
> +		    likely(ext4_mb_pa_goal_check(ac, pa))) {
Alright so this looks good to me, I just have one concern. 

Consider the following scenario:

		1. We find a PA that cover our logical blocks but the physical blocks in
			 the PA don't match our goal and we return false.

		2. We then proceed in ext4_mb_regular_allocator and actually manage to
			 allocate the goal blocks.

		3. Now the logical blocks of the file are associated to the blocks
			 allocated in Step 2 and the logical blocks of the PA would actually never get
			 utilized, and hence they are wasted space.

That being said, I think fixing this would be a bit tricky and I'm not sure if
it's even worth the trouble as GOAL_ONLY is not a very common case anyways. Would like
to hear other people's thoughts on this.


Regards,
ojaswin
>  			atomic_inc(&pa->pa_count);
>  			ext4_mb_use_inode_pa(ac, pa);
>  			spin_unlock(&pa->pa_lock);
> -- 
> 2.30.0
>