Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 12:39:36 +0200
From:   Jan Kara <jack@suse.cz>
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>
Cc:     linux-ext4@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
        Ritesh Harjani <riteshh@linux.ibm.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jan Kara <jack@suse.cz>, Kemeng Shi <shikemeng@huaweicloud.com>
Subject: Re: [PATCH v2 12/12] ext4: Give symbolic names to mballoc criterias
Message-ID: <20230607103936.wqtcrc76tqpbc2ya@quack3>
References: <cover.1685449706.git.ojaswin@linux.ibm.com>
 <a2dc6ec5aea5e5e68cf8e788c2a964ffead9c8b0.1685449706.git.ojaswin@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a2dc6ec5aea5e5e68cf8e788c2a964ffead9c8b0.1685449706.git.ojaswin@linux.ibm.com>
Precedence: bulk

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@suse.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

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@suse.com>
SUSE Labs, CR