| Message ID | 20240326213823.528302-6-shikemeng@huaweicloud.com |
|---|---|
| State | New |
| Headers | show |
| Series | Minor improvements and cleanups to ext4 mballoc | expand |
On Wed, Mar 27, 2024 at 05:38:23AM +0800, Kemeng Shi wrote: > Expand next_linear_group to remove repat check for linear scan. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > --- > fs/ext4/mballoc.c | 37 ++++++------------------------------- > 1 file changed, 6 insertions(+), 31 deletions(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 0f8a34513bf6..561780a274cd 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -1075,31 +1075,6 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac) > return 1; > } > > -/* > - * Return next linear group for allocation. If linear traversal should not be > - * performed, this function just returns the same group > - */ > -static ext4_group_t > -next_linear_group(struct ext4_allocation_context *ac, ext4_group_t group, > - ext4_group_t ngroups) > -{ > - if (!should_optimize_scan(ac)) > - goto inc_and_return; > - > - if (ac->ac_groups_linear_remaining) { > - ac->ac_groups_linear_remaining--; > - goto inc_and_return; > - } > - > - return group; > -inc_and_return: > - /* > - * Artificially restricted ngroups for non-extent > - * files makes group > ngroups possible on first loop. > - */ > - return group + 1 >= ngroups ? 0 : group + 1; > -} > - > /* > * ext4_mb_choose_next_group: choose next group for allocation. > * > @@ -1118,12 +1093,12 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac, > { > *new_cr = ac->ac_criteria; > > - if (!should_optimize_scan(ac) || ac->ac_groups_linear_remaining) { > - *group = next_linear_group(ac, *group, ngroups); > - return; > - } > - > - if (*new_cr == CR_POWER2_ALIGNED) { > + if (!should_optimize_scan(ac)) > + *group = *group + 1 >= ngroups ? 0 : *group + 1; > + else if (ac->ac_groups_linear_remaining) { > + ac->ac_groups_linear_remaining--; > + *group = *group + 1 >= ngroups ? 0 : *group + 1; > + } else if (*new_cr == CR_POWER2_ALIGNED) { Hi Kemeng, thanks for the cleanups I feel that open coding this logic and having a single if for linear scan and non linear scan cases is making the code a bit more harder to follow and we are losing some comments as well. Since our main aim is to avoid the double checking, maybe we can keep next_linear_group() strictly for getting the next linear group correctly and rest of the checks outside. So something like: static ext4_group_t next_linear_group(ext4_group_t group, ext4_group_t ngroups) { /* * Artificially restricted ngroups for non-extent * files makes group > ngroups possible on first loop. */ return group + 1 >= ngroups ? 0 : group + 1; } static void ext4_mb_choose_next_group(...) { ... /* * Fallback to linear scan when optimized scanning is disabled */ if (!should_optimize_scan(ac)) { *group = next_linear_group(*group, ngroups); return; } /* * Optimized scanning can return non adjacent groups which can cause * seek overhead for rotational disks. So try few linear groups before * trying optimized scan. */ if (ac->ac_groups_linear_remaining) { *group = next_linear_group(*group, ngroups); ac->ac_groups_linear_remaining--; return; } ... } Let me know your thought. Regards, ojaswin > ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group); > } else if (*new_cr == CR_GOAL_LEN_FAST) { > ext4_mb_choose_next_group_goal_fast(ac, new_cr, group); > -- > 2.30.0 >
on 3/29/2024 3:14 PM, Ojaswin Mujoo wrote: > On Wed, Mar 27, 2024 at 05:38:23AM +0800, Kemeng Shi wrote: >> Expand next_linear_group to remove repat check for linear scan. >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >> --- >> fs/ext4/mballoc.c | 37 ++++++------------------------------- >> 1 file changed, 6 insertions(+), 31 deletions(-) >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index 0f8a34513bf6..561780a274cd 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -1075,31 +1075,6 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac) >> return 1; >> } >> >> -/* >> - * Return next linear group for allocation. If linear traversal should not be >> - * performed, this function just returns the same group >> - */ >> -static ext4_group_t >> -next_linear_group(struct ext4_allocation_context *ac, ext4_group_t group, >> - ext4_group_t ngroups) >> -{ >> - if (!should_optimize_scan(ac)) >> - goto inc_and_return; >> - >> - if (ac->ac_groups_linear_remaining) { >> - ac->ac_groups_linear_remaining--; >> - goto inc_and_return; >> - } >> - >> - return group; >> -inc_and_return: >> - /* >> - * Artificially restricted ngroups for non-extent >> - * files makes group > ngroups possible on first loop. >> - */ >> - return group + 1 >= ngroups ? 0 : group + 1; >> -} >> - >> /* >> * ext4_mb_choose_next_group: choose next group for allocation. >> * >> @@ -1118,12 +1093,12 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac, >> { >> *new_cr = ac->ac_criteria; >> >> - if (!should_optimize_scan(ac) || ac->ac_groups_linear_remaining) { >> - *group = next_linear_group(ac, *group, ngroups); >> - return; >> - } >> - >> - if (*new_cr == CR_POWER2_ALIGNED) { >> + if (!should_optimize_scan(ac)) >> + *group = *group + 1 >= ngroups ? 0 : *group + 1; >> + else if (ac->ac_groups_linear_remaining) { >> + ac->ac_groups_linear_remaining--; >> + *group = *group + 1 >= ngroups ? 0 : *group + 1; >> + } else if (*new_cr == CR_POWER2_ALIGNED) { > > > Hi Kemeng, thanks for the cleanups > > I feel that open coding this logic and having a single if for linear scan and > non linear scan cases is making the code a bit more harder to follow and we are > losing some comments as well. > > Since our main aim is to avoid the double checking, maybe we can keep > next_linear_group() strictly for getting the next linear group correctly and > rest of the checks outside. So something like: > > static ext4_group_t > next_linear_group(ext4_group_t group, ext4_group_t ngroups) > { > > /* > * Artificially restricted ngroups for non-extent > * files makes group > ngroups possible on first loop. > */ > return group + 1 >= ngroups ? 0 : group + 1; > } > > static void ext4_mb_choose_next_group(...) > { > ... > > /* > * Fallback to linear scan when optimized scanning is disabled > */ > if (!should_optimize_scan(ac)) { > *group = next_linear_group(*group, ngroups); > return; > } > > /* > * Optimized scanning can return non adjacent groups which can cause > * seek overhead for rotational disks. So try few linear groups before > * trying optimized scan. > */ > if (ac->ac_groups_linear_remaining) { > *group = next_linear_group(*group, ngroups); > ac->ac_groups_linear_remaining--; > return; > } > > ... > } > > Let me know your thought. This make senses to me. I will do in next version. Thanks for the advise. Kemeng > > Regards, > ojaswin > >> ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group); >> } else if (*new_cr == CR_GOAL_LEN_FAST) { >> ext4_mb_choose_next_group_goal_fast(ac, new_cr, group); >> -- >> 2.30.0 >> > > >
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 0f8a34513bf6..561780a274cd 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -1075,31 +1075,6 @@ static inline int should_optimize_scan(struct ext4_allocation_context *ac) return 1; } -/* - * Return next linear group for allocation. If linear traversal should not be - * performed, this function just returns the same group - */ -static ext4_group_t -next_linear_group(struct ext4_allocation_context *ac, ext4_group_t group, - ext4_group_t ngroups) -{ - if (!should_optimize_scan(ac)) - goto inc_and_return; - - if (ac->ac_groups_linear_remaining) { - ac->ac_groups_linear_remaining--; - goto inc_and_return; - } - - return group; -inc_and_return: - /* - * Artificially restricted ngroups for non-extent - * files makes group > ngroups possible on first loop. - */ - return group + 1 >= ngroups ? 0 : group + 1; -} - /* * ext4_mb_choose_next_group: choose next group for allocation. * @@ -1118,12 +1093,12 @@ static void ext4_mb_choose_next_group(struct ext4_allocation_context *ac, { *new_cr = ac->ac_criteria; - if (!should_optimize_scan(ac) || ac->ac_groups_linear_remaining) { - *group = next_linear_group(ac, *group, ngroups); - return; - } - - if (*new_cr == CR_POWER2_ALIGNED) { + if (!should_optimize_scan(ac)) + *group = *group + 1 >= ngroups ? 0 : *group + 1; + else if (ac->ac_groups_linear_remaining) { + ac->ac_groups_linear_remaining--; + *group = *group + 1 >= ngroups ? 0 : *group + 1; + } else if (*new_cr == CR_POWER2_ALIGNED) { ext4_mb_choose_next_group_p2_aligned(ac, new_cr, group); } else if (*new_cr == CR_GOAL_LEN_FAST) { ext4_mb_choose_next_group_goal_fast(ac, new_cr, group);
Expand next_linear_group to remove repat check for linear scan. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/ext4/mballoc.c | 37 ++++++------------------------------- 1 file changed, 6 insertions(+), 31 deletions(-)