Re: [PATCH v2 02/16] ext4: remove unnecessary s_mb_last_start

Jan Kara
2025-06-27T18:15:11Z

On Mon 23-06-25 15:32:50, Baokun Li wrote:
> ac->ac_g_ex.fe_start is only used in ext4_mb_find_by_goal(), but STREAM
> ALLOC is activated after ext4_mb_find_by_goal() fails, so there's no need
> to update ac->ac_g_ex.fe_start, remove the unnecessary s_mb_last_start.
>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

I'd just note that ac->ac_g_ex.fe_start is also used in
ext4_mb_collect_stats() so this change may impact the statistics gathered
there. OTOH it is questionable whether we even want to account streaming
allocation as a goal hit... Anyway, I'm fine with this, I'd just mention it
in the changelog.

Also one nit below but feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> @@ -2849,7 +2848,6 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
> /* TBD: may be hot point */
> spin_lock(&sbi->s_md_lock);
> ac->ac_g_ex.fe_group = sbi->s_mb_last_group;
> - ac->ac_g_ex.fe_start = sbi->s_mb_last_start;

Maybe reset ac->ac_g_ex.fe_start to 0 instead of leaving it at some random
value? Just for the sake of defensive programming...

Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR