diff mbox series

ext4: Replace CR_FAST macro with inline function for readability

Message ID 20230629134719.108104-1-ojaswin@linux.ibm.com
State New
Headers
Series ext4: Replace CR_FAST macro with inline function for readability |

Commit Message

Ojaswin Mujoo June 29, 2023, 1:47 p.m. UTC 
  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>
---
 fs/ext4/ext4.h    | 7 ++++---
 fs/ext4/mballoc.c | 8 ++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

Comments

Jan Kara June 29, 2023, 2 p.m. UTC | #1 
On Thu 29-06-23 19:17:19, Ojaswin Mujoo wrote:
> 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>

Thanks for this cleanup! Feel free to add:

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

Just one suggestion for consideration below:

> @@ -2630,7 +2630,7 @@ 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)
> +	if (cr <= CR_GOAL_LEN_SLOW && free < ac->ac_g_ex.fe_len)

Maybe this could be (!ext4_mb_cr_expensive(cr) || cr == CR_GOAL_LEN_SLOW)?
Or maybe more explanatory would be (cr < CR_ANY_FREE) because AFAIU that's
the only scan where we bother scanning groups that have no chance of
satisfying the full allocation? Anyway a short comment explaining this
might be useful. And in either case we can get rid of a bit confusing
CR_FAST define.

								Honza
Ojaswin Mujoo June 29, 2023, 2:41 p.m. UTC | #2 
On Thu, Jun 29, 2023 at 04:00:18PM +0200, Jan Kara wrote:
> On Thu 29-06-23 19:17:19, Ojaswin Mujoo wrote:
> > 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>
> 
> Thanks for this cleanup! Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just one suggestion for consideration below:
> 
> > @@ -2630,7 +2630,7 @@ 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)
> > +	if (cr <= CR_GOAL_LEN_SLOW && free < ac->ac_g_ex.fe_len)
> 
> Maybe this could be (!ext4_mb_cr_expensive(cr) || cr == CR_GOAL_LEN_SLOW)?
> Or maybe more explanatory would be (cr < CR_ANY_FREE) because AFAIU that's
> the only scan where we bother scanning groups that have no chance of
> satisfying the full allocation? Anyway a short comment explaining this
> might be useful. And in either case we can get rid of a bit confusing
> CR_FAST define.
> 
> 								Honza

Thanks for the review Jan! I actually had the same idea since it 
felt like (cr <= CR_GOAL_LEN_SLOW) doesnt clearly express the intent of this
check. I think I ultimately decided to leave it untouched to keep things
simple.

However, I like the idea of making it (cr < CR_ANY_FREE) with a comment
to explain the intent behind this condition. If it's fine with everyone I can
address it in v2.

Regards,
ojaswin
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
diff mbox series

Patch

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..94fdcc757aa9 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,7 @@  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)
+	if (cr <= CR_GOAL_LEN_SLOW && free < ac->ac_g_ex.fe_len)
 		goto out;
 	if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp)))
 		goto out;
@@ -2654,7 +2654,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 +2848,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)) {