| Message ID | 9670431b31aa62e83509fa2802aad364910ee52e.1674822311.git.ojaswin@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | multiblock allocator improvements | expand |
On Fri 27-01-23 18:07:31, Ojaswin Mujoo wrote: > Convert criteria to be an enum so it easier to maintain. This change > also makes it easier to insert new criterias in the future. > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> Just two small comments below: > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index b8b00457da8d..6037b8e0af86 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -126,6 +126,14 @@ enum SHIFT_DIRECTION { > SHIFT_RIGHT, > }; > > +/* > + * Number of criterias defined. For each criteria, mballoc has slightly > + * different way of finding the required blocks nad usually, higher the ^^^ and > + * criteria the slower the allocation. We start at lower criterias and keep > + * falling back to higher ones if we are not able to find any blocks. > + */ > +#define EXT4_MB_NUM_CRS 4 > + So defining this in a different header than the enum itself is fragile. I understand you need it in ext4_sb_info declaration so probably I'd move the enum declaration to ext4.h. Alternatively I suppose we could move a lot of mballoc stuff out of ext4_sb_info into a separate struct because there's a lot of it. But that would be much larger undertaking. Also when going for symbolic allocator scan names maybe we could actually make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could deal with ordered comparisons like in: if (cr < 2 && (!sbi->s_log_groups_per_flex || ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & !(ext4_has_group_desc_csum(sb) && (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) return 0; to declare CR_FAST_SCAN = 2, or something like that. What do you think? Honza
On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote: > On Fri 27-01-23 18:07:31, Ojaswin Mujoo wrote: > > Convert criteria to be an enum so it easier to maintain. This change > > also makes it easier to insert new criterias in the future. > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > Just two small comments below: Hi Jan, Thanks for the review. > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > > index b8b00457da8d..6037b8e0af86 100644 > > --- a/fs/ext4/ext4.h > > +++ b/fs/ext4/ext4.h > > @@ -126,6 +126,14 @@ enum SHIFT_DIRECTION { > > SHIFT_RIGHT, > > }; > > > > +/* > > + * Number of criterias defined. For each criteria, mballoc has slightly > > + * different way of finding the required blocks nad usually, higher the > ^^^ and > > > + * criteria the slower the allocation. We start at lower criterias and keep > > + * falling back to higher ones if we are not able to find any blocks. > > + */ > > +#define EXT4_MB_NUM_CRS 4 > > + > > So defining this in a different header than the enum itself is fragile. I > understand you need it in ext4_sb_info declaration so probably I'd move the > enum declaration to ext4.h. Alternatively I suppose we could move a lot of Got it, I'll try to keep them in the same file. > mballoc stuff out of ext4_sb_info into a separate struct because there's a > lot of it. But that would be much larger undertaking. Right, we did notice that as well, but as you said, that's out of scope of this patchset. > > Also when going for symbolic allocator scan names maybe we could actually > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could > deal with ordered comparisons like in: I like this idea, it should make the code a bit more easier to understand. However just wondering if I should do it as a part of this series or a separate patch since we'll be touching code all around and I don't want to confuse people with the noise :) > > if (cr < 2 && > (!sbi->s_log_groups_per_flex || > ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & > !(ext4_has_group_desc_csum(sb) && > (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) > return 0; > > to declare CR_FAST_SCAN = 2, or something like that. What do you think? About this, wont it be better to just use something like cr < CR_BEST_LENGTH_ALL instead of defining a new CR_FAST_SCAN = 2. The only concern is that if we add a new "fast" CR (say between CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make sure we also update CR_FAST_SCAN to 3 which is easy to miss. Regards, Ojaswin > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Fri 17-03-23 15:56:46, Ojaswin Mujoo wrote: > On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote: > > Also when going for symbolic allocator scan names maybe we could actually > > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, > > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could > > deal with ordered comparisons like in: > I like this idea, it should make the code a bit more easier to > understand. However just wondering if I should do it as a part of this > series or a separate patch since we'll be touching code all around and > I don't want to confuse people with the noise :) I guess a mechanical rename should not be really confusing. It just has to be a separate patch. > > > > if (cr < 2 && > > (!sbi->s_log_groups_per_flex || > > ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & > > !(ext4_has_group_desc_csum(sb) && > > (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) > > return 0; > > > > to declare CR_FAST_SCAN = 2, or something like that. What do you think? > About this, wont it be better to just use something like > > cr < CR_BEST_LENGTH_ALL > > instead of defining a new CR_FAST_SCAN = 2. Yeah, that works as well. > The only concern is that if we add a new "fast" CR (say between > CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make > sure we also update CR_FAST_SCAN to 3 which is easy to miss. Well, you have that problem with any naming scheme (and even with numbers). So as long as names are all defined together, there's reasonable chance you'll remember to verify the limits still hold :) Honza
On Thu, Mar 23, 2023 at 11:55:37AM +0100, Jan Kara wrote: > On Fri 17-03-23 15:56:46, Ojaswin Mujoo wrote: > > On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote: > > > Also when going for symbolic allocator scan names maybe we could actually > > > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, > > > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could > > > deal with ordered comparisons like in: > > I like this idea, it should make the code a bit more easier to > > understand. However just wondering if I should do it as a part of this > > series or a separate patch since we'll be touching code all around and > > I don't want to confuse people with the noise :) > > I guess a mechanical rename should not be really confusing. It just has to > be a separate patch. Alright, got it. > > > > > > > if (cr < 2 && > > > (!sbi->s_log_groups_per_flex || > > > ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & > > > !(ext4_has_group_desc_csum(sb) && > > > (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) > > > return 0; > > > > > > to declare CR_FAST_SCAN = 2, or something like that. What do you think? > > About this, wont it be better to just use something like > > > > cr < CR_BEST_LENGTH_ALL > > > > instead of defining a new CR_FAST_SCAN = 2. > > Yeah, that works as well. > > > The only concern is that if we add a new "fast" CR (say between > > CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make > > sure we also update CR_FAST_SCAN to 3 which is easy to miss. > > Well, you have that problem with any naming scheme (and even with numbers). > So as long as names are all defined together, there's reasonable chance > you'll remember to verify the limits still hold :) haha that's true. Anyways, I'll try a few things and see what looks good. Thanks for the suggestions. Regards, ojaswin > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Sat, Mar 25, 2023 at 08:12:36PM +0530, Ojaswin Mujoo wrote: > On Thu, Mar 23, 2023 at 11:55:37AM +0100, Jan Kara wrote: > > On Fri 17-03-23 15:56:46, Ojaswin Mujoo wrote: > > > On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote: > > > > Also when going for symbolic allocator scan names maybe we could actually > > > > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, > > > > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could > > > > deal with ordered comparisons like in: > > > I like this idea, it should make the code a bit more easier to > > > understand. However just wondering if I should do it as a part of this > > > series or a separate patch since we'll be touching code all around and > > > I don't want to confuse people with the noise :) > > > > I guess a mechanical rename should not be really confusing. It just has to > > be a separate patch. > Alright, got it. > > > > > > > > > > if (cr < 2 && > > > > (!sbi->s_log_groups_per_flex || > > > > ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & > > > > !(ext4_has_group_desc_csum(sb) && > > > > (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) > > > > return 0; > > > > > > > > to declare CR_FAST_SCAN = 2, or something like that. What do you think? > > > About this, wont it be better to just use something like > > > > > > cr < CR_BEST_LENGTH_ALL > > > > > > instead of defining a new CR_FAST_SCAN = 2. > > > > Yeah, that works as well. > > > > > The only concern is that if we add a new "fast" CR (say between > > > CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make > > > sure we also update CR_FAST_SCAN to 3 which is easy to miss. > > > > Well, you have that problem with any naming scheme (and even with numbers). > > So as long as names are all defined together, there's reasonable chance > > you'll remember to verify the limits still hold :) > haha that's true. Anyways, I'll try a few things and see what looks > good. Thanks for the suggestions. Hey Jan, So I was playing around with this and I prepare a patch to convert CR numbers to symbolic names and it looks good as far as things like these are concerned: if (cr < CR_POWER2_ALIGNED) ... However there's one problem that this numeric naming scheme is used in several places like struct member names, function names, traces and comments. The issue is that replacing it everywhere is making some of the names very long for example: atomic_read(&sbi->s_bal_cr0_bad_suggestions)); becomes: atomic_read(&sbi->s_bal_cr_power2_aligned_bad_suggestions)); And this is kind of making the code look messy at a lot of places. So right now there are a few options we can consider: 1. Use symbolic names everywhere at the cost of readability 2. Keep function names/members as is but change criterias enums to symbolic names. This again is not ideal as it makes things ambiguous. 3. Keep the enums as is i.e. CR0, CR1.. etc and add the documentation in the enum declaration on what these enum means. (we can still use CR_FAST_SCAN). Let me know you thoughts on this, or incase you'd like to look at the patch I can attach that as well. Regards, ojaswin > > Regards, > ojaswin > > > > Honza > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR
On Thu 20-04-23 12:02:44, Ojaswin Mujoo wrote: > On Sat, Mar 25, 2023 at 08:12:36PM +0530, Ojaswin Mujoo wrote: > > On Thu, Mar 23, 2023 at 11:55:37AM +0100, Jan Kara wrote: > > > On Fri 17-03-23 15:56:46, Ojaswin Mujoo wrote: > > > > On Thu, Mar 09, 2023 at 01:11:22PM +0100, Jan Kara wrote: > > > > > Also when going for symbolic allocator scan names maybe we could actually > > > > > make names sensible instead of CR[0-4]? Perhaps like CR_ORDER2_ALIGNED, > > > > > CR_BEST_LENGHT_FAST, CR_BEST_LENGTH_ALL, CR_ANY_FREE. And probably we could > > > > > deal with ordered comparisons like in: > > > > I like this idea, it should make the code a bit more easier to > > > > understand. However just wondering if I should do it as a part of this > > > > series or a separate patch since we'll be touching code all around and > > > > I don't want to confuse people with the noise :) > > > > > > I guess a mechanical rename should not be really confusing. It just has to > > > be a separate patch. > > Alright, got it. > > > > > > > > > > > > > if (cr < 2 && > > > > > (!sbi->s_log_groups_per_flex || > > > > > ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) & > > > > > !(ext4_has_group_desc_csum(sb) && > > > > > (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT)))) > > > > > return 0; > > > > > > > > > > to declare CR_FAST_SCAN = 2, or something like that. What do you think? > > > > About this, wont it be better to just use something like > > > > > > > > cr < CR_BEST_LENGTH_ALL > > > > > > > > instead of defining a new CR_FAST_SCAN = 2. > > > > > > Yeah, that works as well. > > > > > > > The only concern is that if we add a new "fast" CR (say between > > > > CR_BEST_LENGTH_FAST and CR_BEST_LENGTH_ALL) then we'll need to make > > > > sure we also update CR_FAST_SCAN to 3 which is easy to miss. > > > > > > Well, you have that problem with any naming scheme (and even with numbers). > > > So as long as names are all defined together, there's reasonable chance > > > you'll remember to verify the limits still hold :) > > haha that's true. Anyways, I'll try a few things and see what looks > > good. Thanks for the suggestions. > Hey Jan, > > So I was playing around with this and I prepare a patch to convert CR > numbers to symbolic names and it looks good as far as things like these > are concerned: > > if (cr < CR_POWER2_ALIGNED) > ... > > However there's one problem that this numeric naming scheme is used in > several places like struct member names, function names, traces and > comments. The issue is that replacing it everywhere is making some of > the names very long for example: > > atomic_read(&sbi->s_bal_cr0_bad_suggestions)); > > becomes: > > atomic_read(&sbi->s_bal_cr_power2_aligned_bad_suggestions)); > > And this is kind of making the code look messy at a lot of places. So > right now there are a few options we can consider: > > 1. Use symbolic names everywhere at the cost of readability Can we maybe go with 1b) being: Use symbolic names in variables / members / ... but shortened? Like s_bal_p2aligned_bad_suggestions? Not sure how many things are like this but from a quick looks it seems we need to come up with a sensible shortcut only for cr0 and cr1? Honza
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index b8b00457da8d..6037b8e0af86 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -126,6 +126,14 @@ enum SHIFT_DIRECTION { SHIFT_RIGHT, }; +/* + * Number of criterias defined. For each criteria, mballoc has slightly + * different way of finding the required blocks nad usually, higher the + * criteria the slower the allocation. We start at lower criterias and keep + * falling back to higher ones if we are not able to find any blocks. + */ +#define EXT4_MB_NUM_CRS 4 + /* * Flags used in mballoc's allocation_context flags field. * @@ -1631,9 +1639,9 @@ struct ext4_sb_info { atomic_t s_bal_2orders; /* 2^order hits */ atomic_t s_bal_cr0_bad_suggestions; atomic_t s_bal_cr1_bad_suggestions; - atomic64_t s_bal_cX_groups_considered[4]; - atomic64_t s_bal_cX_hits[4]; - atomic64_t s_bal_cX_failed[4]; /* cX loop didn't find blocks */ + atomic64_t s_bal_cX_groups_considered[EXT4_MB_NUM_CRS]; + atomic64_t s_bal_cX_hits[EXT4_MB_NUM_CRS]; + atomic64_t s_bal_cX_failed[EXT4_MB_NUM_CRS]; /* cX loop didn't find blocks */ atomic_t s_mb_buddies_generated; /* number of buddies generated */ atomic64_t s_mb_generation_time; atomic_t s_mb_lost_chunks; diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 8b22cc07b054..323604a2ff45 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -409,7 +409,7 @@ static void ext4_mb_generate_from_freelist(struct super_block *sb, void *bitmap, static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac); static bool ext4_mb_good_group(struct ext4_allocation_context *ac, - ext4_group_t group, int cr); + ext4_group_t group, enum criteria cr); static int ext4_try_to_trim_range(struct super_block *sb, struct ext4_buddy *e4b, ext4_grpblk_t start, @@ -857,7 +857,7 @@ mb_update_avg_fragment_size(struct super_block *sb, struct ext4_group_info *grp) * cr level needs an update. */ static void ext4_mb_choose_next_group_cr0(struct ext4_allocation_context *ac, - int *new_cr, ext4_group_t *group, ext4_group_t ngroups) + enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups) { struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb); struct ext4_group_info *iter, *grp; @@ -882,8 +882,8 @@ static void ext4_mb_choose_next_group_cr0(struct ext4_allocation_context *ac, list_for_each_entry(iter, &sbi->s_mb_largest_free_orders[i], bb_largest_free_order_node) { if (sbi->s_mb_stats) - atomic64_inc(&sbi->s_bal_cX_groups_considered[0]); - if (likely(ext4_mb_good_group(ac, iter->bb_group, 0))) { + atomic64_inc(&sbi->s_bal_cX_groups_considered[CR0]); + if (likely(ext4_mb_good_group(ac, iter->bb_group, CR0))) { grp = iter; break; } @@ -895,7 +895,7 @@ static void ext4_mb_choose_next_group_cr0(struct ext4_allocation_context *ac, if (!grp) { /* Increment cr and search again */ - *new_cr = 1; + *new_cr = CR1; } else { *group = grp->bb_group; ac->ac_flags |= EXT4_MB_CR0_OPTIMIZED; @@ -907,7 +907,7 @@ static void ext4_mb_choose_next_group_cr0(struct ext4_allocation_context *ac, * order. Updates *new_cr if cr level needs an update. */ static void ext4_mb_choose_next_group_cr1(struct ext4_allocation_context *ac, - int *new_cr, ext4_group_t *group, ext4_group_t ngroups) + enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups) { struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb); struct ext4_group_info *grp = NULL, *iter; @@ -930,8 +930,8 @@ static void ext4_mb_choose_next_group_cr1(struct ext4_allocation_context *ac, list_for_each_entry(iter, &sbi->s_mb_avg_fragment_size[i], bb_avg_fragment_size_node) { if (sbi->s_mb_stats) - atomic64_inc(&sbi->s_bal_cX_groups_considered[1]); - if (likely(ext4_mb_good_group(ac, iter->bb_group, 1))) { + atomic64_inc(&sbi->s_bal_cX_groups_considered[CR1]); + if (likely(ext4_mb_good_group(ac, iter->bb_group, CR1))) { grp = iter; break; } @@ -945,7 +945,7 @@ static void ext4_mb_choose_next_group_cr1(struct ext4_allocation_context *ac, *group = grp->bb_group; ac->ac_flags |= EXT4_MB_CR1_OPTIMIZED; } else { - *new_cr = 2; + *new_cr = CR2; } } @@ -953,7 +953,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 >= 2) + if (ac->ac_criteria >= CR2) return 0; if (!ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS)) return 0; @@ -998,7 +998,7 @@ next_linear_group(struct ext4_allocation_context *ac, int group, int ngroups) * @ngroups Total number of groups */ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac, - int *new_cr, ext4_group_t *group, ext4_group_t ngroups) + enum criteria *new_cr, ext4_group_t *group, ext4_group_t ngroups) { *new_cr = ac->ac_criteria; @@ -1007,9 +1007,9 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac, return; } - if (*new_cr == 0) { + if (*new_cr == CR0) { ext4_mb_choose_next_group_cr0(ac, new_cr, group, ngroups); - } else if (*new_cr == 1) { + } else if (*new_cr == CR1) { ext4_mb_choose_next_group_cr1(ac, new_cr, group, ngroups); } else { /* @@ -2378,13 +2378,13 @@ void ext4_mb_scan_aligned(struct ext4_allocation_context *ac, * for the allocation or not. */ static bool ext4_mb_good_group(struct ext4_allocation_context *ac, - ext4_group_t group, int cr) + ext4_group_t group, enum criteria cr) { ext4_grpblk_t free, fragments; int flex_size = ext4_flex_bg_size(EXT4_SB(ac->ac_sb)); struct ext4_group_info *grp = ext4_get_group_info(ac->ac_sb, group); - BUG_ON(cr < 0 || cr >= 4); + BUG_ON(cr < CR0 || cr >= EXT4_MB_NUM_CRS); if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) return false; @@ -2398,7 +2398,7 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac, return false; switch (cr) { - case 0: + case CR0: BUG_ON(ac->ac_2order == 0); /* Avoid using the first bg of a flexgroup for data files */ @@ -2417,15 +2417,15 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac, return false; return true; - case 1: + case CR1: if ((free / fragments) >= ac->ac_g_ex.fe_len) return true; break; - case 2: + case CR2: if (free >= ac->ac_g_ex.fe_len) return true; break; - case 3: + case CR3: return true; default: BUG(); @@ -2446,7 +2446,7 @@ static bool ext4_mb_good_group(struct ext4_allocation_context *ac, * out"! */ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac, - ext4_group_t group, int cr) + ext4_group_t group, enum criteria cr) { struct ext4_group_info *grp = ext4_get_group_info(ac->ac_sb, group); struct super_block *sb = ac->ac_sb; @@ -2464,7 +2464,7 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac, free = grp->bb_free; if (free == 0) goto out; - if (cr <= 2 && free < ac->ac_g_ex.fe_len) + if (cr <= CR2 && free < ac->ac_g_ex.fe_len) goto out; if (unlikely(EXT4_MB_GRP_BBITMAP_CORRUPT(grp))) goto out; @@ -2479,7 +2479,7 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac, ext4_get_group_desc(sb, group, NULL); int ret; - /* cr=0/1 is a very optimistic search to find large + /* cr=CR0/CR1 is a very optimistic search to find large * good chunks almost for free. If buddy data is not * ready, then this optimization makes no sense. But * we never skip the first block group in a flex_bg, @@ -2487,7 +2487,7 @@ static int ext4_mb_good_group_nolock(struct ext4_allocation_context *ac, * and we want to make sure we locate metadata blocks * in the first block group in the flex_bg if possible. */ - if (cr < 2 && + if (cr < CR2 && (!sbi->s_log_groups_per_flex || ((group & ((1 << sbi->s_log_groups_per_flex) - 1)) != 0)) && !(ext4_has_group_desc_csum(sb) && @@ -2593,7 +2593,7 @@ static noinline_for_stack int ext4_mb_regular_allocator(struct ext4_allocation_context *ac) { ext4_group_t prefetch_grp = 0, ngroups, group, i; - int cr = -1, new_cr; + enum criteria cr, new_cr; int err = 0, first_err = 0; unsigned int nr = 0, prefetch_ios = 0; struct ext4_sb_info *sbi; @@ -2651,13 +2651,13 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) } /* Let's just scan groups to find more-less suitable blocks */ - cr = ac->ac_2order ? 0 : 1; + cr = ac->ac_2order ? CR0 : CR1; /* - * cr == 0 try to get exact allocation, - * cr == 3 try to get anything + * cr == CR0 try to get exact allocation, + * cr == CR3 try to get anything */ repeat: - for (; cr < 4 && ac->ac_status == AC_STATUS_CONTINUE; cr++) { + for (; cr < EXT4_MB_NUM_CRS && ac->ac_status == AC_STATUS_CONTINUE; cr++) { ac->ac_criteria = cr; /* * searching for the right group start @@ -2684,7 +2684,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) * spend a lot of time loading imperfect groups */ if ((prefetch_grp == group) && - (cr > 1 || + (cr > CR1 || prefetch_ios < sbi->s_mb_prefetch_limit)) { unsigned int curr_ios = prefetch_ios; @@ -2726,9 +2726,9 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) } ac->ac_groups_scanned++; - if (cr == 0) + if (cr == CR0) ext4_mb_simple_scan_group(ac, &e4b); - else if (cr == 1 && sbi->s_stripe && + else if (cr == CR1 && sbi->s_stripe && !(ac->ac_g_ex.fe_len % sbi->s_stripe)) ext4_mb_scan_aligned(ac, &e4b); else @@ -2768,7 +2768,7 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) ac->ac_b_ex.fe_len = 0; ac->ac_status = AC_STATUS_CONTINUE; ac->ac_flags |= EXT4_MB_HINT_FIRST; - cr = 3; + cr = CR3; goto repeat; } } @@ -2891,36 +2891,36 @@ int ext4_seq_mb_stats_show(struct seq_file *seq, void *offset) seq_printf(seq, "\tgroups_scanned: %u\n", atomic_read(&sbi->s_bal_groups_scanned)); seq_puts(seq, "\tcr0_stats:\n"); - seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[0])); + seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[CR0])); seq_printf(seq, "\t\tgroups_considered: %llu\n", - atomic64_read(&sbi->s_bal_cX_groups_considered[0])); + atomic64_read(&sbi->s_bal_cX_groups_considered[CR0])); seq_printf(seq, "\t\tuseless_loops: %llu\n", - atomic64_read(&sbi->s_bal_cX_failed[0])); + atomic64_read(&sbi->s_bal_cX_failed[CR0])); seq_printf(seq, "\t\tbad_suggestions: %u\n", atomic_read(&sbi->s_bal_cr0_bad_suggestions)); seq_puts(seq, "\tcr1_stats:\n"); - seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[1])); + seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[CR1])); seq_printf(seq, "\t\tgroups_considered: %llu\n", - atomic64_read(&sbi->s_bal_cX_groups_considered[1])); + atomic64_read(&sbi->s_bal_cX_groups_considered[CR1])); seq_printf(seq, "\t\tuseless_loops: %llu\n", - atomic64_read(&sbi->s_bal_cX_failed[1])); + atomic64_read(&sbi->s_bal_cX_failed[CR1])); seq_printf(seq, "\t\tbad_suggestions: %u\n", atomic_read(&sbi->s_bal_cr1_bad_suggestions)); seq_puts(seq, "\tcr2_stats:\n"); - seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[2])); + seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[CR2])); seq_printf(seq, "\t\tgroups_considered: %llu\n", - atomic64_read(&sbi->s_bal_cX_groups_considered[2])); + atomic64_read(&sbi->s_bal_cX_groups_considered[CR2])); seq_printf(seq, "\t\tuseless_loops: %llu\n", - atomic64_read(&sbi->s_bal_cX_failed[2])); + atomic64_read(&sbi->s_bal_cX_failed[CR2])); seq_puts(seq, "\tcr3_stats:\n"); - seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[3])); + seq_printf(seq, "\t\thits: %llu\n", atomic64_read(&sbi->s_bal_cX_hits[CR3])); seq_printf(seq, "\t\tgroups_considered: %llu\n", - atomic64_read(&sbi->s_bal_cX_groups_considered[3])); + atomic64_read(&sbi->s_bal_cX_groups_considered[CR3])); seq_printf(seq, "\t\tuseless_loops: %llu\n", - atomic64_read(&sbi->s_bal_cX_failed[3])); + atomic64_read(&sbi->s_bal_cX_failed[CR3])); seq_printf(seq, "\textents_scanned: %u\n", atomic_read(&sbi->s_bal_ex_scanned)); seq_printf(seq, "\t\tgoal_hits: %u\n", atomic_read(&sbi->s_bal_goals)); seq_printf(seq, "\t\t2^n_hits: %u\n", atomic_read(&sbi->s_bal_2orders)); diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h index 165a17893c81..f0087a85e366 100644 --- a/fs/ext4/mballoc.h +++ b/fs/ext4/mballoc.h @@ -95,6 +95,16 @@ */ #define MB_NUM_ORDERS(sb) ((sb)->s_blocksize_bits + 2) +/* + * All possible allocation criterias for mballoc + */ +enum criteria { + CR0, + CR1, + CR2, + CR3, +}; + struct ext4_free_data { /* this links the free block information from sb_info */ struct list_head efd_list;