Date:   Tue, 14 Feb 2023 01:40:47 +0530
From:   Ojaswin Mujoo <ojaswin@linux.ibm.com>
To:     Kemeng Shi <shikemeng@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 19/21] ext4: remove repeat assignment to ac_f_ex
Message-ID: <Y+qZR8ayTYSch9KT@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <20230209194825.511043-1-shikemeng@huaweicloud.com>
 <20230209194825.511043-20-shikemeng@huaweicloud.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230209194825.511043-20-shikemeng@huaweicloud.com>
MIME-Version: 1.0
Precedence: bulk

On Fri, Feb 10, 2023 at 03:48:23AM +0800, Kemeng Shi wrote:
> Call trace to assign ac_f_ex:
> ext4_mb_use_best_found
> 	ac->ac_f_ex = ac->ac_b_ex;
> 	ext4_mb_new_preallocation
> 		ext4_mb_new_group_pa
> 			ac->ac_f_ex = ac->ac_b_ex;
> 		ext4_mb_new_inode_pa
> 			ac->ac_f_ex = ac->ac_b_ex;
> 
> Actually allocated blocks is already stored in ac_f_ex in
> ext4_mb_use_best_found, so there is no need to assign ac_f_ex
> in ext4_mb_new_group_pa and ext4_mb_new_inode_pa.
> Just remove repeat assignment to ac_f_ex in ext4_mb_new_group_pa
> and ext4_mb_new_inode_pa.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7d6991af50d8..dec716f331ac 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4635,10 +4635,6 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac)
>  		BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len);
>  	}
>  
> -	/* preallocation can change ac_b_ex, thus we store actually
> -	 * allocated blocks for history */
> -	ac->ac_f_ex = ac->ac_b_ex;
> -
Alright, so we should note that the ac_b_ex being assigned to ac_f_ex
here can differ from the last allocation of ac_f_ex in
ext4_mb_use_best_found() in the case that bex_len < gex_len and we enter
the PA window adjustment if condition that is just above this line.

In such a case, we should maybe see which of the bex do we actually want
to store in fex. IMO, this patch does the right thing as the allocation
in ext4_mb_use_best_found() gives us an idea of how much the allocator
was exactly able to allocate before other PA related logics start making
changes to the best ex. Further, following the discussion here [1], we
might end up rewriting the PA window adjustment logic in future, and
that could also change the length of ac_b_ex, which we would not want to
reflect in ac_f_ex (again since i see ac_f_ex as the record of actual ex
that allocator was able to find).

So this patch looks like the right thing to do for me. Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> 

[1]
https://lore.kernel.org/linux-ext4/Y+OGkVvzPN0RMv0O@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
>  	pa->pa_lstart = ac->ac_b_ex.fe_logical;
>  	pa->pa_pstart = ext4_grp_offs_to_block(sb, &ac->ac_b_ex);
>  	pa->pa_len = ac->ac_b_ex.fe_len;
> @@ -4689,10 +4685,6 @@ ext4_mb_new_group_pa(struct ext4_allocation_context *ac)
>  
>  	pa = ac->ac_pa;
>  
> -	/* preallocation can change ac_b_ex, thus we store actually
> -	 * allocated blocks for history */
> -	ac->ac_f_ex = ac->ac_b_ex;
> -
>  	pa->pa_pstart = ext4_grp_offs_to_block(sb, &ac->ac_b_ex);
>  	pa->pa_lstart = pa->pa_pstart;
>  	pa->pa_len = ac->ac_b_ex.fe_len;
> -- 
> 2.30.0
>