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@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxx> [1] https://lore.kernel.org/linux-ext4/Y+OGkVvzPN0RMv0O@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > 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 >
