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 <[email protected]>
---
fs/ext4/ext4.h | 7 ++++---
fs/ext4/mballoc.c | 8 ++++----
2 files changed, 8 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..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)) {
--
2.31.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 <[email protected]>
Thanks for this cleanup! Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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
--
Jan Kara <[email protected]>
SUSE Labs, CR
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 <[email protected]>
>
> Thanks for this cleanup! Feel free to add:
>
> Reviewed-by: Jan Kara <[email protected]>
>
> 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 <[email protected]>
> SUSE Labs, CR