Re: [RFC 04/11] ext4: Convert mballoc cr (criteria) to enum

[Jan Kara <jack@suse.cz>]

On Fri 27-01-23 18:07:31, Ojaswin Mujoo wrote:
> Convert criteria to be an enum so it easier to maintain. This change
> also makes it easier to insert new criterias in the future.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Just two small comments below:

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b8b00457da8d..6037b8e0af86 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -126,6 +126,14 @@ enum SHIFT_DIRECTION {
>  	SHIFT_RIGHT,
>  };
>  
> +/*
> + * Number of criterias defined. For each criteria, mballoc has slightly
> + * different way of finding the required blocks nad usually, higher the
						   ^^^ and

> + * criteria the slower the allocation. We start at lower criterias and keep
> + * falling back to higher ones if we are not able to find any blocks.
> + */
> +#define EXT4_MB_NUM_CRS 4
> +

So defining this in a different header than the enum itself is fragile. I
understand you need it in ext4_sb_info declaration so probably I'd move the
enum declaration to ext4.h. Alternatively I suppose we could move a lot of
mballoc stuff out of ext4_sb_info into a separate struct because there's a
lot of it. But that would be much larger undertaking.

Also when going for symbolic allocator scan names maybe we could actually
make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED,
CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could
deal with ordered comparisons like in:

                if (cr < 2 &&
                    (!sbi->s_log_groups_per_flex ||
                     ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) &
                    !(ext4_has_group_desc_csum(sb) &&
                      (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))))
                        return 0;

to declare CR_FAST_SCAN = 2, or something like that. What do you think?

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