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, 02 Aug 2023 07:27:06 +0530
Message-Id: <87pm46w6j1.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     Ojaswin Mujoo <ojaswin@linux.ibm.com>, linux-ext4@vger.kernel.org,
        Theodore Ts'o <tytso@mit.edu>
Cc:     Ritesh Harjani <riteshh@linux.ibm.com>,
        linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>,
        Kemeng Shi <shikemeng@huaweicloud.com>
Subject: Re: [PATCH v2] ext4: Replace CR_FAST macro with inline function for readability
In-Reply-To: <20230630085927.140137-1-ojaswin@linux.ibm.com>
Precedence: bulk

Ojaswin Mujoo <ojaswin@linux.ibm.com> writes:

> Replace CR_FAST with ext4_mb_cr_expensive() inline function for better
> readability. This function returns true if the criteria is one of the
> expensive/slower ones where lots of disk IO/prefetching is acceptable.
>
> No functional changes are intended in this patch.
>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>
> Changes since v1 [1] 
>
> * reworded an if condition in ext_mb_good_group_nolock
>   and added a comment to better describe the intent of the 
> 	condition
> * Picked up RVB from Jan.
>
> [1]
> https://lore.kernel.org/linux-ext4/ZJ2YIr5EVbz4ezIc@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/T/#ma9428de15c4084937274f4d66dcd4d53f505d32a
>
>  fs/ext4/ext4.h    |  7 ++++---
>  fs/ext4/mballoc.c | 13 +++++++++----
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 45a531446ea2..e404169a2858 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -176,9 +176,6 @@ enum criteria {
>  	EXT4_MB_NUM_CRS
>  };
>  
> -/* criteria below which we use fast block scanning and avoid unnecessary IO */
> -#define CR_FAST CR_GOAL_LEN_SLOW
> -
>  /*
>   * Flags used in mballoc's allocation_context flags field.
>   *
> @@ -2924,6 +2921,10 @@ extern int ext4_trim_fs(struct super_block *, struct fstrim_range *);
>  extern void ext4_process_freed_data(struct super_block *sb, tid_t commit_tid);
>  extern void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
>  		       int len, int state);
> +static inline bool ext4_mb_cr_expensive(enum criteria cr)
> +{
> +	return cr >= CR_GOAL_LEN_SLOW;
> +}
>  
>  /* inode.c */
>  void ext4_inode_csum_set(struct inode *inode, struct ext4_inode *raw,
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index a2475b8c9fb5..542577f621c6 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2446,7 +2446,7 @@ void ext4_mb_complex_scan_group(struct ext4_allocation_context *ac,
>  			break;
>  		}
>  
> -		if (ac->ac_criteria < CR_FAST) {
> +		if (!ext4_mb_cr_expensive(ac->ac_criteria)) {
>  			/*
>  			 * In CR_GOAL_LEN_FAST and CR_BEST_AVAIL_LEN, we are
>  			 * sure that this group will have a large enough
> @@ -2630,7 +2630,12 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
>  	free = grp->bb_free;
>  	if (free == 0)
>  		goto out;
> -	if (cr <= CR_FAST && free < ac->ac_g_ex.fe_len)
> +	/*
> +	 * In all criterias except CR_ANY_FREE we try to avoid groups that
> +	 * can't possibly satisfy the full goal request due to insufficient
> +	 * free blocks.
> +	 */
> +	if (cr < CR_ANY_FREE && free < ac->ac_g_ex.fe_len)
>  		goto out;

cr CR_BEST_AVAIL_LEN, is the one where we can allocate something smaller
than the original goal length. But in that we alter the goal length
itself while selecting the criteria.
So checking free < ac->ac_g_ex.fe_len should work here. 


Looks good to me. Feel free to add -

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


>  	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
>  		goto out;
> @@ -2654,7 +2659,7 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac,
>  		 * sure we locate metadata blocks in the first block group in
>  		 * the flex_bg if possible.
>  		 */
> -		if (cr < CR_FAST &&
> +		if (!ext4_mb_cr_expensive(cr) &&
>  		    (!sbi->s_log_groups_per_flex ||
>  		     ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) &&
>  		    !(ext4_has_group_desc_csum(sb) &&
> @@ -2848,7 +2853,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>  			 * spend a lot of time loading imperfect groups
>  			 */
>  			if ((prefetch_grp == group) &&
> -			    (cr >= CR_FAST ||
> +			    (ext4_mb_cr_expensive(cr) ||
>  			     prefetch_ios < sbi->s_mb_prefetch_limit)) {
>  				nr = sbi->s_mb_prefetch;
>  				if (ext4_has_feature_flex_bg(sb)) {
> -- 
> 2.31.1

-ritesh