On Tue 30-05-23 18:03:50, Ojaswin Mujoo wrote:
> mballoc criterias have historically been called by numbers
> like CR0, CR1... however this makes it confusing to understand
> what each criteria is about.
>
> Change these criterias from numbers to symbolic names and add
> relevant comments. While we are at it, also reformat and add some
> comments to ext4_seq_mb_stats_show() for better readability.
>
> Additionally, define CR_FAST which signifies the criteria
> below which we can make quicker decisions like:
> * quitting early if (free block < requested len)
> * avoiding to scan free extents smaller than required len.
> * avoiding to initialize buddy cache and work with existing cache
> * limiting prefetches
>
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
Thanks for doing this!
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 942e97026a60..c29a4e1fcd5d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -135,16 +135,45 @@ enum SHIFT_DIRECTION {
> */
> #define EXT4_MB_NUM_CRS 5
> /*
> - * All possible allocation criterias for mballoc
> + * All possible allocation criterias for mballoc. Lower are faster.
> */
> enum criteria {
> - CR0,
> - CR1,
> - CR1_5,
> - CR2,
> - CR3,
> + /*
> + * Used when number of blocks needed is a power of 2. This doesn't
> + * trigger any disk IO except prefetch and is the fastest criteria.
> + */
> + CR_POWER2_ALIGNED,
> +
> + /*
> + * Tries to lookup in-memory data structures to find the most suitable
> + * group that satisfies goal request. No disk IO except block prefetch.
> + */
> + CR_GOAL_LEN_FAST,
> +
> + /*
> + * Same as CR_GOAL_LEN_FAST but is allowed to reduce the goal length to
> + * the best available length for faster allocation.
Some whitespace damage here...
> + */
> + CR_BEST_AVAIL_LEN,
> +
> + /*
> + * Reads each block group sequentially, performing disk IO if necessary, to
> + * find find_suitable block group. Tries to allocate goal length but might trim
Too long line here.
> + * the request if nothing is found after enough tries.
> + */
> + CR_GOAL_LEN_SLOW,
> +
> + /*
> + * Finds the first free set of blocks and allocates those. This is only
> + * used in rare cases when CR_GOAL_LEN_SLOW also fails to allocate
> + * anything.
> + */
> + CR_ANY_FREE,
> };
>
> +/* criteria below which we use fast block scanning and avoid unnecessary IO */
> +#define CR_FAST CR_GOAL_LEN_SLOW
> +
Maybe instead of defining CR_FAST value we could define
static inline bool mballoc_cr_expensive(enum criteria cr)
{
return cr >= CR_GOAL_LEN_SLOW;
}
And use this. I think it will make the conditions more understandable.
...
> @@ -1064,7 +1068,7 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac)
> {
> if (unlikely(!test_opt2(ac->ac_sb, MB_OPTIMIZE_SCAN)))
> return 0;
> - if (ac->ac_criteria >= CR2)
> + if (ac->ac_criteria >= CR_GOAL_LEN_SLOW)
Maybe we should use CR_FAST (or the new function) here?
Otherwise the patch looks good!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
