[PATCH v3 00/20] Some bugfix and cleanup to mballoc

[PATCH v3 00/20] Some bugfix and cleanup to mballoc

[Kemeng Shi]

Hi, this series contain some random cleanup patches and some bugfix
patches to make EXT4_MB_HINT_GOAL_ONLY work properly, protect pa->pa_free
from race and so on. More details can be found in git log.
Thanks!

---
V3:
-patch 01/20 "ext4: set goal start correctly in
ext4_mb_normalize_request" correctly record goal in ac_g_ex instead of
ac_f_ex, this also trigger another original bug that wanted goal maybe
out of valid data block range. Add valid range check in patch 01/20
to fully fix the bug.
-run "kvm-xfstests smoke" and all tests are passed except generic/454.
This test also failed in running kernel without this patchset, so
it's unlikely caused by this patchset. I'm trying to figure out the
reason but it may take a while as I'm not family with kvm-xfstests.
Just send this series for early review and maybe some help to pass
the failed test case.
---
V2:
-Add signed-off from Ritesh and Ojaswin to patch 3/20 "ext4: get correct
ext4_group_info in ext4_mb_prefetch_fini" as this is a duplicate of
a patch under reviewing.
-Split out original patch "ext4: avoid to use preallocated blocks if
EXT4_MB_HINT_GOAL_ONLY is set" which will be resend after improved.
-Improve log information of patch 20.
-Collect Reviewed-by from Ojaswin and Ritesh. Now only patch 3, 12 and
20 need futher review.
---


Kemeng Shi (20):
  ext4: set goal start correctly in ext4_mb_normalize_request
  ext4: allow to find by goal if EXT4_MB_HINT_GOAL_ONLY is set
  ext4: get correct ext4_group_info in ext4_mb_prefetch_fini
  ext4: correct calculation of s_mb_preallocated
  ext4: correct start of used group pa for debug in ext4_mb_use_group_pa
  ext4: protect pa->pa_free in ext4_discard_allocated_blocks
  ext4: add missed brelse in ext4_free_blocks_simple
  ext4: remove unused return value of ext4_mb_try_best_found and
    ext4_mb_free_metadata
  ext4: Remove unnecessary release when memory allocation failed in
    ext4_mb_init_cache
  ext4: remove unnecessary e4b->bd_buddy_page check in
    ext4_mb_load_buddy_gfp
  ext4: remove unnecessary check in ext4_mb_new_blocks
  ext4: remove dead check in mb_buddy_mark_free
  ext4: remove ac->ac_found > sbi->s_mb_min_to_scan dead check in
    ext4_mb_check_limits
  ext4: use best found when complex scan of group finishs
  ext4: remove unnecessary exit_meta_group_info tag
  ext4: remove unnecessary count2 in ext4_free_data_in_buddy
  ext4: remove unnecessary goto in ext4_mb_mark_diskspace_used
  ext4: remove repeat assignment to ac_f_ex
  ext4: remove comment code ext4_discard_preallocations
  ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

 fs/ext4/mballoc.c | 111 ++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 68 deletions(-)

-- 
2.30.0

[PATCH v3 01/20] ext4: set goal start correctly in ext4_mb_normalize_request

[Kemeng Shi]

We need to set ac_g_ex to notify the goal start used in
ext4_mb_find_by_goal. Set ac_g_ex instead of ac_f_ex in
ext4_mb_normalize_request.
Besides we should assure goal start is in range [first_data_block,
blocks_count) as ext4_mb_initialize_context does.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 5b2ae37a8b80..36cd545f5ab4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3993,6 +3993,7 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
 				struct ext4_allocation_request *ar)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
+	struct ext4_super_block *es = sbi->s_es;
 	int bsbits, max;
 	ext4_lblk_t end;
 	loff_t size, start_off;
@@ -4188,18 +4189,20 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
 	ac->ac_g_ex.fe_len = EXT4_NUM_B2C(sbi, size);
 
 	/* define goal start in order to merge */
-	if (ar->pright && (ar->lright == (start + size))) {
+	if (ar->pright && (ar->lright == (start + size)) &&
+	    ar->pright - size >= le32_to_cpu(es->s_first_data_block)) {
 		/* merge to the right */
 		ext4_get_group_no_and_offset(ac->ac_sb, ar->pright - size,
-						&ac->ac_f_ex.fe_group,
-						&ac->ac_f_ex.fe_start);
+						&ac->ac_g_ex.fe_group,
+						&ac->ac_g_ex.fe_start);
 		ac->ac_flags |= EXT4_MB_HINT_TRY_GOAL;
 	}
-	if (ar->pleft && (ar->lleft + 1 == start)) {
+	if (ar->pleft && (ar->lleft + 1 == start) &&
+	    ar->pleft + 1 < ext4_blocks_count(es)) {
 		/* merge to the left */
 		ext4_get_group_no_and_offset(ac->ac_sb, ar->pleft + 1,
-						&ac->ac_f_ex.fe_group,
-						&ac->ac_f_ex.fe_start);
+						&ac->ac_g_ex.fe_group,
+						&ac->ac_g_ex.fe_start);
 		ac->ac_flags |= EXT4_MB_HINT_TRY_GOAL;
 	}
 
-- 
2.30.0

Re: [PATCH v3 01/20] ext4: set goal start correctly in ext4_mb_normalize_request

[Ritesh Harjani]

Kemeng Shi <shikemeng@huaweicloud.com> writes:

> We need to set ac_g_ex to notify the goal start used in
> ext4_mb_find_by_goal. Set ac_g_ex instead of ac_f_ex in
> ext4_mb_normalize_request.
> Besides we should assure goal start is in range [first_data_block,
> blocks_count) as ext4_mb_initialize_context does.

Thanks for looking into the failed test case.
Patch looks good to me. After going through the change, I also feel we
should be updating ac_g_ex instead of ac_f_ex.

Good spotting! Please feel free to add -

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

>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 5b2ae37a8b80..36cd545f5ab4 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3993,6 +3993,7 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  				struct ext4_allocation_request *ar)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> +	struct ext4_super_block *es = sbi->s_es;
>  	int bsbits, max;
>  	ext4_lblk_t end;
>  	loff_t size, start_off;
> @@ -4188,18 +4189,20 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  	ac->ac_g_ex.fe_len = EXT4_NUM_B2C(sbi, size);
>
>  	/* define goal start in order to merge */
> -	if (ar->pright && (ar->lright == (start + size))) {
> +	if (ar->pright && (ar->lright == (start + size)) &&
> +	    ar->pright - size >= le32_to_cpu(es->s_first_data_block)) {
>  		/* merge to the right */
>  		ext4_get_group_no_and_offset(ac->ac_sb, ar->pright - size,
> -						&ac->ac_f_ex.fe_group,
> -						&ac->ac_f_ex.fe_start);
> +						&ac->ac_g_ex.fe_group,
> +						&ac->ac_g_ex.fe_start);
>  		ac->ac_flags |= EXT4_MB_HINT_TRY_GOAL;
>  	}
> -	if (ar->pleft && (ar->lleft + 1 == start)) {
> +	if (ar->pleft && (ar->lleft + 1 == start) &&
> +	    ar->pleft + 1 < ext4_blocks_count(es)) {
>  		/* merge to the left */
>  		ext4_get_group_no_and_offset(ac->ac_sb, ar->pleft + 1,
> -						&ac->ac_f_ex.fe_group,
> -						&ac->ac_f_ex.fe_start);
> +						&ac->ac_g_ex.fe_group,
> +						&ac->ac_g_ex.fe_start);
>  		ac->ac_flags |= EXT4_MB_HINT_TRY_GOAL;
>  	}
>
> --
> 2.30.0

Re: [PATCH v3 1/20] ext4: set goal start correctly in ext4_mb_normalize_request

[Theodore Ts'o]

On Sat, Mar 04, 2023 at 01:21:01AM +0800, Kemeng Shi wrote:
> We need to set ac_g_ex to notify the goal start used in
> ext4_mb_find_by_goal. Set ac_g_ex instead of ac_f_ex in
> ext4_mb_normalize_request.
> Besides we should assure goal start is in range [first_data_block,
> blocks_count) as ext4_mb_initialize_context does.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/ext4/mballoc.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 5b2ae37a8b80..36cd545f5ab4 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3993,6 +3993,7 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  				struct ext4_allocation_request *ar)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
> +	struct ext4_super_block *es = sbi->s_es;
>  	int bsbits, max;
>  	ext4_lblk_t end;
>  	loff_t size, start_off;
> @@ -4188,18 +4189,20 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  	ac->ac_g_ex.fe_len = EXT4_NUM_B2C(sbi, size);
>  
>  	/* define goal start in order to merge */
> -	if (ar->pright && (ar->lright == (start + size))) {
> +	if (ar->pright && (ar->lright == (start + size)) &&
> +	    ar->pright - size >= le32_to_cpu(es->s_first_data_block)) {
>  		/* merge to the right */

I had to ammend this commit to add this check:

 	/* define goal start in order to merge */
 	if (ar->pright && (ar->lright == (start + size)) &&
+	    ar->pright >= size &&
 	    ar->pright - size >= le32_to_cpu(es->s_first_data_block)) {

Without this check, it's possible for ar->pright - size to go negative
(well, underflow since it's an unsigned value).  This will later
trigger a BUG_ON, which was easily reproduced via:

   kvm-xfstests -c ext4/ext3conv generic/231

Cheers,

							- Ted

[PATCH v3 02/20] ext4: allow to find by goal if EXT4_MB_HINT_GOAL_ONLY is set

[Kemeng Shi]

If EXT4_MB_HINT_GOAL_ONLY is set, ext4_mb_regular_allocator will only
allocate blocks from ext4_mb_find_by_goal. Allow to find by goal in
ext4_mb_find_by_goal if EXT4_MB_HINT_GOAL_ONLY is set or allocation
with EXT4_MB_HINT_GOAL_ONLY set will always fail.

EXT4_MB_HINT_GOAL_ONLY is not used at all, so the problem is not
found for now.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 36cd545f5ab4..6122278e91eb 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2162,7 +2162,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac,
 	struct ext4_group_info *grp = ext4_get_group_info(ac->ac_sb, group);
 	struct ext4_free_extent ex;
 
-	if (!(ac->ac_flags & EXT4_MB_HINT_TRY_GOAL))
+	if (!(ac->ac_flags & (EXT4_MB_HINT_TRY_GOAL | EXT4_MB_HINT_GOAL_ONLY)))
 		return 0;
 	if (grp->bb_free == 0)
 		return 0;
-- 
2.30.0

[PATCH v3 03/20] ext4: get correct ext4_group_info in ext4_mb_prefetch_fini

[Kemeng Shi]

We always get ext4_group_desc with group + 1 and ext4_group_info with
group to check if we need do initialize ext4_group_info for the group.
Just get ext4_group_desc with group for ext4_group_info initialization
check.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 6122278e91eb..d1c5b6ea57da 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2569,14 +2569,14 @@ ext4_group_t ext4_mb_prefetch(struct super_block *sb, ext4_group_t group,
 void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group,
 			   unsigned int nr)
 {
-	while (nr-- > 0) {
-		struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group,
-								  NULL);
-		struct ext4_group_info *grp = ext4_get_group_info(sb, group);
+	struct ext4_group_desc *gdp;
+	struct ext4_group_info *grp;
 
+	while (nr-- > 0) {
 		if (!group)
 			group = ext4_get_groups_count(sb);
 		group--;
+		gdp = ext4_get_group_desc(sb, group, NULL);
 		grp = ext4_get_group_info(sb, group);
 
 		if (EXT4_MB_GRP_NEED_INIT(grp) &&
-- 
2.30.0

[PATCH v3 04/20] ext4: correct calculation of s_mb_preallocated

[Kemeng Shi]

We will add pa_free to s_mb_preallocated when new ext4_prealloc_space is
created. In ext4_mb_new_inode_pa, we will call ext4_mb_use_inode_pa
before adding pa_free to s_mb_preallocated. However, ext4_mb_use_inode_pa
will consume pa_free for block allocation which triggerred the creation
of ext4_prealloc_space. Add pa_free to s_mb_preallocated before
ext4_mb_use_inode_pa to correct calculation of s_mb_preallocated.
There is no such problem in ext4_mb_new_group_pa as pa_free of group pa
is consumed in ext4_mb_release_context instead of ext4_mb_use_group_pa.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index d1c5b6ea57da..d7ea3c2014ff 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4670,8 +4670,8 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac)
 		 pa->pa_len, pa->pa_lstart);
 	trace_ext4_mb_new_inode_pa(ac, pa);
 
-	ext4_mb_use_inode_pa(ac, pa);
 	atomic_add(pa->pa_free, &sbi->s_mb_preallocated);
+	ext4_mb_use_inode_pa(ac, pa);
 
 	ei = EXT4_I(ac->ac_inode);
 	grp = ext4_get_group_info(sb, ac->ac_b_ex.fe_group);
-- 
2.30.0

[PATCH v3 05/20] ext4: correct start of used group pa for debug in ext4_mb_use_group_pa

[Kemeng Shi]

As we don't correct pa_lstart here, so there is no need to subtract
pa_lstart with consumed len.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index d7ea3c2014ff..d6a27e47584a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4322,7 +4322,7 @@ static void ext4_mb_use_group_pa(struct ext4_allocation_context *ac,
 	 * Other CPUs are prevented from allocating from this pa by lg_mutex
 	 */
 	mb_debug(ac->ac_sb, "use %u/%u from group pa %p\n",
-		 pa->pa_lstart-len, len, pa);
+		 pa->pa_lstart, len, pa);
 }
 
 /*
-- 
2.30.0

[PATCH v3 06/20] ext4: protect pa->pa_free in ext4_discard_allocated_blocks

[Kemeng Shi]

If ext4_mb_mark_diskspace_used fails in ext4_mb_new_blocks, we may
discard pa already in list. Protect pa with pa_lock to avoid race.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index d6a27e47584a..56f35a25842c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4266,8 +4266,11 @@ static void ext4_discard_allocated_blocks(struct ext4_allocation_context *ac)
 		ext4_mb_unload_buddy(&e4b);
 		return;
 	}
-	if (pa->pa_type == MB_INODE_PA)
+	if (pa->pa_type == MB_INODE_PA) {
+		spin_lock(&pa->pa_lock);
 		pa->pa_free += ac->ac_b_ex.fe_len;
+		spin_unlock(&pa->pa_lock);
+	}
 }
 
 /*
-- 
2.30.0

[PATCH v3 07/20] ext4: add missed brelse in ext4_free_blocks_simple

[Kemeng Shi]

Release bitmap buffer_head we got if error occurs.
Besides, this patch remove unused assignment to err.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 56f35a25842c..fca11be28fcb 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5848,13 +5848,12 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
 	ext4_get_group_no_and_offset(sb, block, &group, &blkoff);
 	bitmap_bh = ext4_read_block_bitmap(sb, group);
 	if (IS_ERR(bitmap_bh)) {
-		err = PTR_ERR(bitmap_bh);
 		pr_warn("Failed to read block bitmap\n");
 		return;
 	}
 	gdp = ext4_get_group_desc(sb, group, &gdp_bh);
 	if (!gdp)
-		return;
+		goto err_out;
 
 	for (i = 0; i < count; i++) {
 		if (!mb_test_bit(blkoff + i, bitmap_bh->b_data))
@@ -5863,7 +5862,7 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
 	mb_clear_bits(bitmap_bh->b_data, blkoff, count);
 	err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
 	if (err)
-		return;
+		goto err_out;
 	ext4_free_group_clusters_set(
 		sb, gdp, ext4_free_group_clusters(sb, gdp) +
 		count - already_freed);
@@ -5872,6 +5871,8 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
 	ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
 	sync_dirty_buffer(bitmap_bh);
 	sync_dirty_buffer(gdp_bh);
+
+err_out:
 	brelse(bitmap_bh);
 }
 
-- 
2.30.0

[PATCH v3 08/20] ext4: remove unused return value of ext4_mb_try_best_found and ext4_mb_free_metadata

[Kemeng Shi]

Return value static function ext4_mb_try_best_found and
ext4_mb_free_metadata is not used. Just remove unused return value.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index fca11be28fcb..df76f859f3ff 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2124,7 +2124,7 @@ static void ext4_mb_measure_extent(struct ext4_allocation_context *ac,
 }
 
 static noinline_for_stack
-int ext4_mb_try_best_found(struct ext4_allocation_context *ac,
+void ext4_mb_try_best_found(struct ext4_allocation_context *ac,
 					struct ext4_buddy *e4b)
 {
 	struct ext4_free_extent ex = ac->ac_b_ex;
@@ -2135,7 +2135,7 @@ int ext4_mb_try_best_found(struct ext4_allocation_context *ac,
 	BUG_ON(ex.fe_len <= 0);
 	err = ext4_mb_load_buddy(ac->ac_sb, group, e4b);
 	if (err)
-		return err;
+		return;
 
 	ext4_lock_group(ac->ac_sb, group);
 	max = mb_find_extent(e4b, ex.fe_start, ex.fe_len, &ex);
@@ -2147,8 +2147,6 @@ int ext4_mb_try_best_found(struct ext4_allocation_context *ac,
 
 	ext4_unlock_group(ac->ac_sb, group);
 	ext4_mb_unload_buddy(e4b);
-
-	return 0;
 }
 
 static noinline_for_stack
@@ -5699,7 +5697,7 @@ static void ext4_try_merge_freed_extent(struct ext4_sb_info *sbi,
 	kmem_cache_free(ext4_free_data_cachep, entry);
 }
 
-static noinline_for_stack int
+static noinline_for_stack void
 ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
 		      struct ext4_free_data *new_entry)
 {
@@ -5742,7 +5740,7 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
 				EXT4_C2B(sbi, cluster),
 				"Block already on to-be-freed list");
 			kmem_cache_free(ext4_free_data_cachep, new_entry);
-			return 0;
+			return;
 		}
 	}
 
@@ -5768,7 +5766,6 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
 	list_add_tail(&new_entry->efd_list, &sbi->s_freed_data_list);
 	sbi->s_mb_free_pending += clusters;
 	spin_unlock(&sbi->s_md_lock);
-	return 0;
 }
 
 /*
-- 
2.30.0

[PATCH v3 09/20] ext4: Remove unnecessary release when memory allocation failed in ext4_mb_init_cache

[Kemeng Shi]

If we alloc array of buffer_head failed, there is no resource need to be
freed and we can simpily return error.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index df76f859f3ff..9e998b5d4037 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1168,10 +1168,8 @@ static int ext4_mb_init_cache(struct page *page, char *incore, gfp_t gfp)
 	if (groups_per_page > 1) {
 		i = sizeof(struct buffer_head *) * groups_per_page;
 		bh = kzalloc(i, gfp);
-		if (bh == NULL) {
-			err = -ENOMEM;
-			goto out;
-		}
+		if (bh == NULL)
+			return -ENOMEM;
 	} else
 		bh = &bhs;
 
-- 
2.30.0

[PATCH v3 10/20] ext4: remove unnecessary e4b->bd_buddy_page check in ext4_mb_load_buddy_gfp

[Kemeng Shi]

e4b->bd_buddy_page is only set if we initialize ext4_buddy successfully. So
e4b->bd_buddy_page is always NULL in error handle branch. Just remove the
dead check.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9e998b5d4037..6eea1296b82a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1555,8 +1555,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 		put_page(page);
 	if (e4b->bd_bitmap_page)
 		put_page(e4b->bd_bitmap_page);
-	if (e4b->bd_buddy_page)
-		put_page(e4b->bd_buddy_page);
+
 	e4b->bd_buddy = NULL;
 	e4b->bd_bitmap = NULL;
 	return ret;
-- 
2.30.0

[PATCH v3 11/20] ext4: remove unnecessary check in ext4_mb_new_blocks

[Kemeng Shi]

1. remove unnecessary ac check:
We always go to out tag before ac is successfully allocated, then we can
move out tag after free of ac and remove NULL check of ac.

2. remove unnecessary *errp check:
We always go to errout tag if *errp is non-zero, then we can move errout
tag into error handle if *errp is non-zero.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 6eea1296b82a..e53bf9d5a48d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5641,16 +5641,15 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
 		*errp = -ENOSPC;
 	}
 
-errout:
 	if (*errp) {
+errout:
 		ac->ac_b_ex.fe_len = 0;
 		ar->len = 0;
 		ext4_mb_show_ac(ac);
 	}
 	ext4_mb_release_context(ac);
+	kmem_cache_free(ext4_ac_cachep, ac);
 out:
-	if (ac)
-		kmem_cache_free(ext4_ac_cachep, ac);
 	if (inquota && ar->len < inquota)
 		dquot_free_block(ar->inode, EXT4_C2B(sbi, inquota - ar->len));
 	if (!ar->len) {
-- 
2.30.0

[PATCH v3 12/20] ext4: remove dead check in mb_buddy_mark_free

[Kemeng Shi]

We always adjust first to even number and adjust last to odd number, so
first == last will never happen. Remove this dead check.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e53bf9d5a48d..16f926675006 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1718,7 +1718,8 @@ static void mb_buddy_mark_free(struct ext4_buddy *e4b, int first, int last)
 			break;
 		order++;
 
-		if (first == last || !(buddy2 = mb_find_buddy(e4b, order, &max))) {
+		buddy2 = mb_find_buddy(e4b, order, &max);
+		if (!buddy2) {
 			mb_clear_bits(buddy, first, last - first + 1);
 			e4b->bd_info->bb_counters[order - 1] += last - first + 1;
 			break;
-- 
2.30.0

[PATCH v3 13/20] ext4: remove ac->ac_found > sbi->s_mb_min_to_scan dead check in ext4_mb_check_limits

[Kemeng Shi]

Only call trace of ext4_mb_check_limits is as following:
ext4_mb_complex_scan_group
	ext4_mb_measure_extent
		ext4_mb_check_limits(ac, e4b, 0);
	ext4_mb_check_limits(ac, e4b, 1);

If the first ac->ac_found > sbi->s_mb_max_to_scan check in
ext4_mb_check_limits is met, we will set ac_status to
AC_STATUS_BREAK and call ext4_mb_try_best_found to try to use
ac->ac_b_ex.
If ext4_mb_try_best_found successes, then block allocation finishs,
the removed ac->ac_found > sbi->s_mb_min_to_scan check is not reachable.
If ext4_mb_try_best_found fails, then we set EXT4_MB_HINT_FIRST and
reset ac->ac_b_ex to retry block allocation. We will use any found
free extent in ext4_mb_measure_extent before reach the removed
ac->ac_found > sbi->s_mb_min_to_scan check.
In summary, the removed ac->ac_found > sbi->s_mb_min_to_scan check is
not reachable and we can remove that dead check.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 16f926675006..5315c67905f6 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2039,8 +2039,7 @@ static void ext4_mb_check_limits(struct ext4_allocation_context *ac,
 	if (bex->fe_len < gex->fe_len)
 		return;
 
-	if ((finish_group || ac->ac_found > sbi->s_mb_min_to_scan)
-			&& bex->fe_group == e4b->bd_group) {
+	if (finish_group && bex->fe_group == e4b->bd_group) {
 		/* recheck chunk's availability - we don't know
 		 * when it was found (within this lock-unlock
 		 * period or not) */
-- 
2.30.0

[PATCH v3 14/20] ext4: use best found when complex scan of group finishs

[Kemeng Shi]

If any bex which meets bex->fe_len >= gex->fe_len is found, then it will
always be used when complex scan of group that bex belongs to finishs.
So there will not be any lock-unlock period.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 5315c67905f6..60b39fba2fce 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2019,8 +2019,6 @@ static void ext4_mb_check_limits(struct ext4_allocation_context *ac,
 	struct ext4_sb_info *sbi = EXT4_SB(ac->ac_sb);
 	struct ext4_free_extent *bex = &ac->ac_b_ex;
 	struct ext4_free_extent *gex = &ac->ac_g_ex;
-	struct ext4_free_extent ex;
-	int max;
 
 	if (ac->ac_status == AC_STATUS_FOUND)
 		return;
@@ -2039,16 +2037,8 @@ static void ext4_mb_check_limits(struct ext4_allocation_context *ac,
 	if (bex->fe_len < gex->fe_len)
 		return;
 
-	if (finish_group && bex->fe_group == e4b->bd_group) {
-		/* recheck chunk's availability - we don't know
-		 * when it was found (within this lock-unlock
-		 * period or not) */
-		max = mb_find_extent(e4b, bex->fe_start, gex->fe_len, &ex);
-		if (max >= gex->fe_len) {
-			ext4_mb_use_best_found(ac, e4b);
-			return;
-		}
-	}
+	if (finish_group)
+		ext4_mb_use_best_found(ac, e4b);
 }
 
 /*
-- 
2.30.0

[PATCH v3 15/20] ext4: remove unnecessary exit_meta_group_info tag

[Kemeng Shi]

We goto exit_meta_group_info only to return -ENOMEM. Return -ENOMEM
directly instead of goto to remove this unnecessary tag.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 60b39fba2fce..f05af818c14e 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3069,7 +3069,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
 		if (meta_group_info == NULL) {
 			ext4_msg(sb, KERN_ERR, "can't allocate mem "
 				 "for a buddy group");
-			goto exit_meta_group_info;
+			return -ENOMEM;
 		}
 		rcu_read_lock();
 		rcu_dereference(sbi->s_group_info)[idx] = meta_group_info;
@@ -3123,7 +3123,6 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group,
 		group_info[idx] = NULL;
 		rcu_read_unlock();
 	}
-exit_meta_group_info:
 	return -ENOMEM;
 } /* ext4_mb_add_groupinfo */
 
-- 
2.30.0

[PATCH v3 16/20] ext4: remove unnecessary count2 in ext4_free_data_in_buddy

[Kemeng Shi]

count2 is always 1 in mb_debug, just remove unnecessary count2.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f05af818c14e..731810d01bbb 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3590,7 +3590,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb,
 {
 	struct ext4_buddy e4b;
 	struct ext4_group_info *db;
-	int err, count = 0, count2 = 0;
+	int err, count = 0;
 
 	mb_debug(sb, "gonna free %u blocks in group %u (0x%p):",
 		 entry->efd_count, entry->efd_group, entry);
@@ -3606,7 +3606,6 @@ static void ext4_free_data_in_buddy(struct super_block *sb,
 	db = e4b.bd_info;
 	/* there are blocks to put in buddy to make them really free */
 	count += entry->efd_count;
-	count2++;
 	ext4_lock_group(sb, entry->efd_group);
 	/* Take it out of per group rb tree */
 	rb_erase(&entry->efd_node, &(db->bb_free_root));
@@ -3631,8 +3630,7 @@ static void ext4_free_data_in_buddy(struct super_block *sb,
 	ext4_unlock_group(sb, entry->efd_group);
 	ext4_mb_unload_buddy(&e4b);
 
-	mb_debug(sb, "freed %d blocks in %d structures\n", count,
-		 count2);
+	mb_debug(sb, "freed %d blocks in 1 structures\n", count);
 }
 
 /*
-- 
2.30.0

[PATCH v3 17/20] ext4: remove unnecessary goto in ext4_mb_mark_diskspace_used

[Kemeng Shi]

When ext4_read_block_bitmap fails, we can return PTR_ERR(bitmap_bh) to
remove unnecessary NULL check of bitmap_bh.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/mballoc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 731810d01bbb..b9e69f3a678f 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3739,9 +3739,7 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 
 	bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group);
 	if (IS_ERR(bitmap_bh)) {
-		err = PTR_ERR(bitmap_bh);
-		bitmap_bh = NULL;
-		goto out_err;
+		return PTR_ERR(bitmap_bh);
 	}
 
 	BUFFER_TRACE(bitmap_bh, "getting write access");
-- 
2.30.0

[PATCH v3 18/20] ext4: remove repeat assignment to ac_f_ex

[Kemeng Shi]

Call trace to assign ac_f_ex:
ext4_mb_use_best_found
	ac->ac_f_ex = ac->ac_b_ex;
	ext4_mb_new_preallocation
		ext4_mb_new_group_pa
			ac->ac_f_ex = ac->ac_b_ex;
		ext4_mb_new_inode_pa
			ac->ac_f_ex = ac->ac_b_ex;

Actually allocated blocks is already stored in ac_f_ex in
ext4_mb_use_best_found, so there is no need to assign ac_f_ex
in ext4_mb_new_group_pa and ext4_mb_new_inode_pa.
Just remove repeat assignment to ac_f_ex in ext4_mb_new_group_pa
and ext4_mb_new_inode_pa.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b9e69f3a678f..d4e226c16baf 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4635,10 +4635,6 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac)
 		BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len);
 	}
 
-	/* preallocation can change ac_b_ex, thus we store actually
-	 * allocated blocks for history */
-	ac->ac_f_ex = ac->ac_b_ex;
-
 	pa->pa_lstart = ac->ac_b_ex.fe_logical;
 	pa->pa_pstart = ext4_grp_offs_to_block(sb, &ac->ac_b_ex);
 	pa->pa_len = ac->ac_b_ex.fe_len;
@@ -4689,10 +4685,6 @@ ext4_mb_new_group_pa(struct ext4_allocation_context *ac)
 
 	pa = ac->ac_pa;
 
-	/* preallocation can change ac_b_ex, thus we store actually
-	 * allocated blocks for history */
-	ac->ac_f_ex = ac->ac_b_ex;
-
 	pa->pa_pstart = ext4_grp_offs_to_block(sb, &ac->ac_b_ex);
 	pa->pa_lstart = pa->pa_pstart;
 	pa->pa_len = ac->ac_b_ex.fe_len;
-- 
2.30.0

[PATCH v3 19/20] ext4: remove comment code ext4_discard_preallocations

[Kemeng Shi]

Just remove comment code in ext4_discard_preallocations.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 fs/ext4/mballoc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index d4e226c16baf..1103d35b31cb 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4925,7 +4925,6 @@ void ext4_discard_preallocations(struct inode *inode, unsigned int needed)
 	int err;
 
 	if (!S_ISREG(inode->i_mode)) {
-		/*BUG_ON(!list_empty(&ei->i_prealloc_list));*/
 		return;
 	}
 
-- 
2.30.0

[PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

[Kemeng Shi]

We try to allocate a block from goal in ext4_mb_new_blocks_simple. We
only need get blkoff in first group with goal and set blkoff to 0 for
the rest groups.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 1103d35b31cb..85d5e219933f 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5772,9 +5772,6 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle,
 			return 0;
 		}
 
-		ext4_get_group_no_and_offset(sb,
-			max(ext4_group_first_block_no(sb, group), goal),
-			NULL, &blkoff);
 		while (1) {
 			i = mb_find_next_zero_bit(bitmap_bh->b_data, max,
 						blkoff);
@@ -5789,6 +5786,8 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle,
 		brelse(bitmap_bh);
 		if (i < max)
 			break;
+
+		blkoff = 0;
 	}
 
 	if (group >= ext4_get_groups_count(sb) || i >= max) {
-- 
2.30.0

Re: [PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

[Theodore Ts'o]

On Sat, Mar 04, 2023 at 01:21:20AM +0800, Kemeng Shi wrote:
> We try to allocate a block from goal in ext4_mb_new_blocks_simple. We
> only need get blkoff in first group with goal and set blkoff to 0 for
> the rest groups.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

While this patch is OK as a simplification, there's a bigger problem
with ext4_mb_new_blocks_simple(), and that is that we start looking
for free blocks starting at the goal block, and then if we can't any
free blocks by the time we get to the last block group.... we stop,
and return ENOSPC.

This function is only used in the fast commit replay path, but for a
very full file system, this could cause a fast commit replay to fail
unnecesarily.  What we need to do is to try wrapping back to the
beginning of the file system, and stop when we hit the original group
(of the goal block) minus one.

We can fix this up in a separate patch, since this doesn't make things
any worse, but essentially we need to do something like this:

    	maxgroups = ext4_get_groups_count(sb);
	for (g = 0; g < maxgroups ; g++) {
	
		...
		group++;
		if (group > maxgroups)
			group = 0;
	}

					- Ted

Re: [PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

[Kemeng Shi]

on 3/16/2023 1:07 PM, Theodore Ts'o wrote:
> On Sat, Mar 04, 2023 at 01:21:20AM +0800, Kemeng Shi wrote:
>> We try to allocate a block from goal in ext4_mb_new_blocks_simple. We
>> only need get blkoff in first group with goal and set blkoff to 0 for
>> the rest groups.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> While this patch is OK as a simplification, there's a bigger problem
> with ext4_mb_new_blocks_simple(), and that is that we start looking
> for free blocks starting at the goal block, and then if we can't any
> free blocks by the time we get to the last block group.... we stop,
> and return ENOSPC.
> 
> This function is only used in the fast commit replay path, but for a
> very full file system, this could cause a fast commit replay to fail
> unnecesarily.  What we need to do is to try wrapping back to the
> beginning of the file system, and stop when we hit the original group
> (of the goal block) minus one.
> 
> We can fix this up in a separate patch, since this doesn't make things
> any worse, but essentially we need to do something like this:
Hi Theodore, thanks for feedback. I will submit another patchset for
mballoc and I would like to include this fix if no one else does. As
new patches may be conflicted with old ones I submited, I would submit
the new patchset after the old ones are fully reviewed and applied
if this fix is not in rush. Thanks!
> 
>     	maxgroups = ext4_get_groups_count(sb);
> 	for (g = 0; g < maxgroups ; g++) {
> 	
> 		...
> 		group++;
> 		if (group > maxgroups)
> 			group = 0;
> 	}
> 
> 					- Ted
> 

-- 
Best wishes
Kemeng Shi

Re: [PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

[Theodore Ts'o]

On Thu, Mar 16, 2023 at 06:19:40PM +0800, Kemeng Shi wrote:
> Hi Theodore, thanks for feedback. I will submit another patchset for
> mballoc and I would like to include this fix if no one else does. As
> new patches may be conflicted with old ones I submited, I would submit
> the new patchset after the old ones are fully reviewed and applied
> if this fix is not in rush. Thanks!

Hi, I've already taken the your patches into the dev branch; were
there any changes you were intending to make to your patches?

If you could submit a separate fix for the bug that I noticed, that
would be great.

Also, if you are interested in doing some more work in mballoc.c, I
was wondering if you would be interested in adding some Kunit tests
for mballoc.c.  A simple example Kunit test for ext4 can be found in
fs/ext4/inode_test.c.  (The convention is to place tests for foo.c in
foo_test.c.)

[1] https://docs.kernel.org/dev-tools/kunit/

In order to add mballoc Kunit tests, we will need to add some "mock"[2]
functions to simulate what happens when mballoc.c tries reading a
block bitmap.  My thinking was to have a test provide an array of some
data structure like this:

struct test_bitmap {
       unsigned int	start;
       unsigned int	len;
};

[2] https://en.wikipedia.org/wiki/Mock_object

... which indicates the starting block, and the length of a run of
blocks that are marked as in use, where the list of blocks are sorted
by starting block number, and where a starting block of ~0 indicates
the end of the list of block extents.

We would also need have a set of utility ext4 Kunit functions to
create "fake" ext4 superblocks and ext4_sb_info structures.

I was originally thinking that obvious starting Kunit tests would be
for fs/ext4/hash.c and fs/ext4/extents_status.c, since they require
the little or no "mocking" support.  However, there are so many
changes in fs/ext4/mballoc.c, the urgency in having unit tests for it
is getting more urgent --- since if there is a bug in one of these
functions, such as the one that I noted in
ext4_mb_new_blocks_simple(), since it's harder to exhaustively test
some of these smaller sub-functions in integration tests such as those
found in xfstests.  Unit tests are the best way to make sure we're
testing all of the code paths in a complex module such as mballoc.c

Cheers,

						- Ted

Re: [PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple

[Kemeng Shi]

on 3/17/2023 11:50 PM, Theodore Ts'o wrote:
> On Thu, Mar 16, 2023 at 06:19:40PM +0800, Kemeng Shi wrote:
>> Hi Theodore, thanks for feedback. I will submit another patchset for
>> mballoc and I would like to include this fix if no one else does. As
>> new patches may be conflicted with old ones I submited, I would submit
>> the new patchset after the old ones are fully reviewed and applied
>> if this fix is not in rush. Thanks!
> 
> Hi, I've already taken the your patches into the dev branch; were
> there any changes you were intending to make to your patches?
> 
> If you could submit a separate fix for the bug that I noticed, that
> would be great.
Hi, I was stuck in some urgent work recently and I will do this ASAP and
it should be done in this week.
> Also, if you are interested in doing some more work in mballoc.c, I
> was wondering if you would be interested in adding some Kunit tests
> for mballoc.c.  A simple example Kunit test for ext4 can be found in
> fs/ext4/inode_test.c.  (The convention is to place tests for foo.c in
> foo_test.c.)

> [1] https://docs.kernel.org/dev-tools/kunit/
> 
> In order to add mballoc Kunit tests, we will need to add some "mock"[2]
> functions to simulate what happens when mballoc.c tries reading a
> block bitmap.  My thinking was to have a test provide an array of some
> data structure like this:
> 
> struct test_bitmap {
>        unsigned int	start;
>        unsigned int	len;
> };
> 
> [2] https://en.wikipedia.org/wiki/Mock_object
> 
> ... which indicates the starting block, and the length of a run of
> blocks that are marked as in use, where the list of blocks are sorted
> by starting block number, and where a starting block of ~0 indicates
> the end of the list of block extents.
> We would also need have a set of utility ext4 Kunit functions to
> create "fake" ext4 superblocks and ext4_sb_info structures.
The Kunit tests thing sounds interesting and I would like to this. But
I still need some time to get basic knowledge then I maybe able to discuss
detais. Of couse, anyone is also interesting in this and can make this work
soon is fine.:)
> I was originally thinking that obvious starting Kunit tests would be
> for fs/ext4/hash.c and fs/ext4/extents_status.c, since they require
> the little or no "mocking" support.  However, there are so many
> changes in fs/ext4/mballoc.c, the urgency in having unit tests for it
> is getting more urgent --- since if there is a bug in one of these
> functions, such as the one that I noted in
> ext4_mb_new_blocks_simple(), since it's harder to exhaustively test
> some of these smaller sub-functions in integration tests such as those
> found in xfstests.  Unit tests are the best way to make sure we're
> testing all of the code paths in a complex module such as mballoc.c
Yes, I can't agree more and this may be able to find other exsiting bugs.

-- 
Best wishes
Kemeng Shi

Re: [PATCH v3 00/20] Some bugfix and cleanup to mballoc

[Kemeng Shi]

on 3/4/2023 1:21 AM, Kemeng Shi wrote:
> Hi, this series contain some random cleanup patches and some bugfix
> patches to make EXT4_MB_HINT_GOAL_ONLY work properly, protect pa->pa_free
> from race and so on. More details can be found in git log.
> Thanks!
> 
> ---
> V3:
> -patch 01/20 "ext4: set goal start correctly in
> ext4_mb_normalize_request" correctly record goal in ac_g_ex instead of
> ac_f_ex, this also trigger another original bug that wanted goal maybe
> out of valid data block range. Add valid range check in patch 01/20
> to fully fix the bug.
> -run "kvm-xfstests smoke" and all tests are passed except generic/454.
> This test also failed in running kernel without this patchset, so
> it's unlikely caused by this patchset. I'm trying to figure out the
> reason but it may take a while as I'm not family with kvm-xfstests.
> Just send this series for early review and maybe some help to pass
> the failed test case.
I re-run "kvm-xfstests smoke" with newly root_fs.img and all tests pass.
It may be the early test which triggerred bug and corruptted the metadata
casues failure.
> ---
> V2:
> -Add signed-off from Ritesh and Ojaswin to patch 3/20 "ext4: get correct
> ext4_group_info in ext4_mb_prefetch_fini" as this is a duplicate of
> a patch under reviewing.
> -Split out original patch "ext4: avoid to use preallocated blocks if
> EXT4_MB_HINT_GOAL_ONLY is set" which will be resend after improved.
> -Improve log information of patch 20.
> -Collect Reviewed-by from Ojaswin and Ritesh. Now only patch 3, 12 and
> 20 need futher review.
> ---
> 
> 
> Kemeng Shi (20):
>   ext4: set goal start correctly in ext4_mb_normalize_request
>   ext4: allow to find by goal if EXT4_MB_HINT_GOAL_ONLY is set
>   ext4: get correct ext4_group_info in ext4_mb_prefetch_fini
>   ext4: correct calculation of s_mb_preallocated
>   ext4: correct start of used group pa for debug in ext4_mb_use_group_pa
>   ext4: protect pa->pa_free in ext4_discard_allocated_blocks
>   ext4: add missed brelse in ext4_free_blocks_simple
>   ext4: remove unused return value of ext4_mb_try_best_found and
>     ext4_mb_free_metadata
>   ext4: Remove unnecessary release when memory allocation failed in
>     ext4_mb_init_cache
>   ext4: remove unnecessary e4b->bd_buddy_page check in
>     ext4_mb_load_buddy_gfp
>   ext4: remove unnecessary check in ext4_mb_new_blocks
>   ext4: remove dead check in mb_buddy_mark_free
>   ext4: remove ac->ac_found > sbi->s_mb_min_to_scan dead check in
>     ext4_mb_check_limits
>   ext4: use best found when complex scan of group finishs
>   ext4: remove unnecessary exit_meta_group_info tag
>   ext4: remove unnecessary count2 in ext4_free_data_in_buddy
>   ext4: remove unnecessary goto in ext4_mb_mark_diskspace_used
>   ext4: remove repeat assignment to ac_f_ex
>   ext4: remove comment code ext4_discard_preallocations
>   ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple
> 
>  fs/ext4/mballoc.c | 111 ++++++++++++++++++----------------------------
>  1 file changed, 43 insertions(+), 68 deletions(-)
> 

-- 
Best wishes
Kemeng Shi

Re: [PATCH v3 00/20] Some bugfix and cleanup to mballoc

[Theodore Ts'o]

On Sat, 4 Mar 2023 01:21:00 +0800, Kemeng Shi wrote:
> patches to make EXT4_MB_HINT_GOAL_ONLY work properly, protect pa->pa_free
> from race and so on. More details can be found in git log.
> Thanks!
> 

Applied, thanks!

[01/20] ext4: set goal start correctly in ext4_mb_normalize_request
        commit: 98860659740354352783cb55c8443e0e46e0978c
[02/20] ext4: allow to find by goal if EXT4_MB_HINT_GOAL_ONLY is set
        commit: cace8c0f41382effffa8a14cfc4666a5ae171d03
[03/20] ext4: get correct ext4_group_info in ext4_mb_prefetch_fini
        commit: 36a366185c1c733ce43e8d6d8ca43997c5b601ce
[04/20] ext4: correct calculation of s_mb_preallocated
        commit: a53b8e3f47d790cb8b520b89b59cf0b65aee7e38
[05/20] ext4: correct start of used group pa for debug in ext4_mb_use_group_pa
        commit: 043a9c31c51a26c5926fd62bdd9d7a057cc4acd1
[06/20] ext4: protect pa->pa_free in ext4_discard_allocated_blocks
        commit: 5ddfc1b03d405d2890dc75eee6e1659c0e022407
[07/20] ext4: add missed brelse in ext4_free_blocks_simple
        commit: a8735a61d23ce9f91c965041eb3ff2a8501853f2
[08/20] ext4: remove unused return value of ext4_mb_try_best_found and ext4_mb_free_metadata
        commit: 1104c3ce5375bbc1229a8e56f7ac84e23953b887
[09/20] ext4: Remove unnecessary release when memory allocation failed in ext4_mb_init_cache
        commit: d5e46843e6db39ae386dd7c728861caca3bb0e6e
[10/20] ext4: remove unnecessary e4b->bd_buddy_page check in ext4_mb_load_buddy_gfp
        commit: d53c2664698f218acd8ce8848427be4da039dee9
[11/20] ext4: remove unnecessary check in ext4_mb_new_blocks
        commit: 4e91261632b0b1d0dee4a9bd99ac70d49d35ebc5
[12/20] ext4: remove dead check in mb_buddy_mark_free
        commit: 0b6d4554bdc97d40692abf05a0f087656acfa876
[13/20] ext4: remove ac->ac_found > sbi->s_mb_min_to_scan dead check in ext4_mb_check_limits
        commit: 38121fdf014cbd631e83fe9aa20a93a57670c2fe
[14/20] ext4: use best found when complex scan of group finishs
        commit: be52c043d5d18d23605980c3485ae41de19cc99c
[15/20] ext4: remove unnecessary exit_meta_group_info tag
        commit: 1a5500baa0235477ac6eef76d75c70540522e886
[16/20] ext4: remove unnecessary count2 in ext4_free_data_in_buddy
        commit: 8df82ffee909b633f727414a4b50c2638de9c3d4
[17/20] ext4: remove unnecessary goto in ext4_mb_mark_diskspace_used
        commit: 101cbefae32efa89e717c89919a9164c4ea2b0a5
[18/20] ext4: remove repeat assignment to ac_f_ex
        commit: b9a0763df46aaa1d7c33859e24ff0ebd4fea009c
[19/20] ext4: remove comment code ext4_discard_preallocations
        commit: e0057b7ceade573f52a51a5bce75de8a1abe60b0
[20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple
        commit: 7bb392f6f9c465182aea60cc55287595bf28f866

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>