[PATCH v5 0/8] cleanups and unit test for mballoc

[PATCH v5 0/8] cleanups and unit test for mballoc

[Kemeng Shi]

v4->v5
1. WARN on free blocks to uninitialized group is removed as normal
fast commit route may triggers this, see [1] for details. The review
tag from Ojaswin of changed patch is also removed and a futher review
is needed.

v3->v4:
1. Collect Reviewed-by from Ojaswin
2. Do improve as Ojaswin kindly suggested: Fix typo in commit,
WARN if try to clear bit of uninitialized group and improve
refactoring of AGGRESSIVE_CHECK code.
3. Fix conflic on patch 16
4. Improve git log in patch 16,17

v2->v3:
1. Fix build warnings on "ext4: add some kunit stub for mballoc kunit
test" and "ext4: add first unit test for ext4_mb_new_blocks_simple in
mballoc"

This series is a new version of unmerged patches from [1]. The first 6
patches of this series factor out codes to mark blocks used or freed
which will update on disk block bitmap and group descriptor. Several
reasons to do this:
1. pair behavior of alloc/free bits. For example,
ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups
in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this.
2. remove repeat code to read from disk, update and write back to disk.
3. reduce future unit test mocks to avoid real IO to update structure
on disk.

The last 2 patches add a unit test for mballoc. Before more unit tests
are added, there are something should be discussed:
1. How to test static function in mballoc.c
Currently, include mballoc-test.c in mballoc.c to test static function
in mballoc.c from mballoc-test.c which is one way suggested in [2].
Not sure if there is any more elegant way to test static function without
touch mballoc.c.
2. How to add mocks to function in mballoc.c which may issue IO to disk
Currently, KUNIT_STATIC_STUB_REDIRECT is added to functions as suggested
in kunit document [3].
3. How to simulate a block bitmap.
Currently, a fake buffer_head with bitmap data is returned, then no
futher change is needed.
If we simulate a block bitmap with an array of data structure like:
struct test_bitmap {
       unsigned int	start;
       unsigned int	len;
}
which is suggested by Theodore in [4], then we need to add mocks to
function which expected bitmap from bitmap_bh->b_data, like
mb_find_next_bit, mb_find_next_zero_bit and maybe more.

Would like to hear any suggestion! Thanks!

I run kvm-xfstest with config "ext4/all" and "-g auto" together with
patchset for resize, you can see detail report in [6].

Unit test result is as followings:
# ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig --raw_output
[18:44:39] Configuring KUnit Kernel ...
[18:44:39] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make ARCH=um O=.kunit --jobs=88
[18:44:47] Starting KUnit Kernel (1/1)...
KTAP version 1
1..2
    KTAP version 1
    # Subtest: ext4_mballoc_test
    1..1
    ok 1 test_new_blocks_simple
ok 1 ext4_mballoc_test
    KTAP version 1
    # Subtest: ext4_inode_test
    1..1
        KTAP version 1
        # Subtest: inode_test_xtimestamp_decoding
        ok 1 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
        ok 2 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits
        ok 3 1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits
        ok 4 2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits
        ok 5 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on
        ok 6 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on
        ok 7 2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on
        ok 8 2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on
        ok 9 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on
        ok 10 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on
        ok 11 2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on
        ok 12 2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on
        ok 13 2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns
        ok 14 2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns
        ok 15 2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on
        ok 16 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on
    # inode_test_xtimestamp_decoding: pass:16 fail:0 skip:0 total:16
    ok 1 inode_test_xtimestamp_decoding
# Totals: pass:16 fail:0 skip:0 total:16
ok 2 ext4_inode_test
[18:44:48] Elapsed time: 8.602s total, 0.001s configuring, 8.483s building, 0.072s running

[1]
https://lore.kernel.org/linux-ext4/20230603150327.3596033-1-shikemeng@huaweicloud.com/T/#m5ff8e3a058ce1cb272dfef3262cd3202ce6e4358
[2]
https://lore.kernel.org/linux-ext4/ZC3MoWn2UO6p+Swp@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com/
[3]
https://docs.kernel.org/dev-tools/kunit/usage.html#testing-static-functions
[4]
https://docs.kernel.org/dev-tools/kunit/api/functionredirection.html#c.KUNIT_STATIC_STUB_REDIRECT
[5]
https://lore.kernel.org/linux-ext4/20230317155047.GB3270589@mit.edu/
[6]
https://lore.kernel.org/linux-ext4/20230629120044.1261968-1-shikemeng@huaweicloud.com/T/#mcc8fb0697fd54d9267c02c027e1eb3468026ae56

Kemeng Shi (8):
  ext4: factor out codes to update block bitmap and group descriptor on
    disk from ext4_mb_mark_bb
  ext4: call ext4_mb_mark_group_bb in ext4_free_blocks_simple
  ext4: extent ext4_mb_mark_group_bb to support allocation under journal
  ext4: call ext4_mb_mark_group_bb in ext4_mb_mark_diskspace_used
  ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb
  ext4: call ext4_mb_mark_group_bb in ext4_group_add_blocks
  ext4: add some kunit stub for mballoc kunit test
  ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

 fs/ext4/balloc.c       |  16 ++
 fs/ext4/mballoc-test.c | 323 ++++++++++++++++++++++++++
 fs/ext4/mballoc.c      | 506 ++++++++++++++++-------------------------
 3 files changed, 535 insertions(+), 310 deletions(-)
 create mode 100644 fs/ext4/mballoc-test.c

-- 
2.30.0

[PATCH v5 1/8] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb

[Kemeng Shi]

There are several reasons to add a general function to update block
bitmap and group descriptor on disk:
1. pair behavior of alloc/free bits. For example,
ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups
in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this.
2. remove repeat code to read from disk, update and write back to disk.
3. reduce future unit test mocks to catch real IO to update structure
on disk.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a2475b8c9fb5..58864a9116c0 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3948,6 +3948,86 @@ void ext4_exit_mballoc(void)
 	ext4_groupinfo_destroy_slabs();
 }
 
+struct ext4_mark_context {
+	struct super_block *sb;
+	int state;
+};
+
+static int
+ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
+		      ext4_grpblk_t blkoff, ext4_grpblk_t len)
+{
+	struct super_block *sb = mc->sb;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct buffer_head *bitmap_bh = NULL;
+	struct ext4_group_desc *gdp;
+	struct buffer_head *gdp_bh;
+	int err;
+	unsigned int i, already, changed;
+
+	bitmap_bh = ext4_read_block_bitmap(sb, group);
+	if (IS_ERR(bitmap_bh))
+		return PTR_ERR(bitmap_bh);
+
+	err = -EIO;
+	gdp = ext4_get_group_desc(sb, group, &gdp_bh);
+	if (!gdp)
+		goto out_err;
+
+	ext4_lock_group(sb, group);
+	if (ext4_has_group_desc_csum(sb) &&
+	    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
+		gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
+		ext4_free_group_clusters_set(sb, gdp,
+			ext4_free_clusters_after_init(sb, group, gdp));
+	}
+
+	already = 0;
+	for (i = 0; i < len; i++)
+		if (mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
+				mc->state)
+			already++;
+	changed = len - already;
+
+	if (mc->state) {
+		mb_set_bits(bitmap_bh->b_data, blkoff, len);
+		ext4_free_group_clusters_set(sb, gdp,
+			ext4_free_group_clusters(sb, gdp) - changed);
+	} else {
+		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
+		ext4_free_group_clusters_set(sb, gdp,
+			ext4_free_group_clusters(sb, gdp) + changed);
+	}
+
+	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
+	ext4_group_desc_csum_set(sb, group, gdp);
+	ext4_unlock_group(sb, group);
+
+	if (sbi->s_log_groups_per_flex) {
+		ext4_group_t flex_group = ext4_flex_group(sbi, group);
+		struct flex_groups *fg = sbi_array_rcu_deref(sbi,
+					   s_flex_groups, flex_group);
+
+		if (mc->state)
+			atomic64_sub(changed, &fg->free_clusters);
+		else
+			atomic64_add(changed, &fg->free_clusters);
+	}
+
+	err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
+	if (err)
+		goto out_err;
+	err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
+	if (err)
+		goto out_err;
+
+	sync_dirty_buffer(bitmap_bh);
+	sync_dirty_buffer(gdp_bh);
+
+out_err:
+	brelse(bitmap_bh);
+	return err;
+}
 
 /*
  * Check quota and mark chosen space (ac->ac_b_ex) non-free in bitmaps
@@ -4074,15 +4154,15 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
 			int len, int state)
 {
-	struct buffer_head *bitmap_bh = NULL;
-	struct ext4_group_desc *gdp;
-	struct buffer_head *gdp_bh;
+	struct ext4_mark_context mc = {
+		.sb = sb,
+		.state = state,
+	};
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	ext4_group_t group;
 	ext4_grpblk_t blkoff;
-	int i, err;
-	int already;
-	unsigned int clen, clen_changed, thisgrp_len;
+	int err;
+	unsigned int clen, thisgrp_len;
 
 	while (len > 0) {
 		ext4_get_group_no_and_offset(sb, block, &group, &blkoff);
@@ -4103,80 +4183,17 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
 			ext4_error(sb, "Marking blocks in system zone - "
 				   "Block = %llu, len = %u",
 				   block, thisgrp_len);
-			bitmap_bh = NULL;
 			break;
 		}
 
-		bitmap_bh = ext4_read_block_bitmap(sb, group);
-		if (IS_ERR(bitmap_bh)) {
-			err = PTR_ERR(bitmap_bh);
-			bitmap_bh = NULL;
-			break;
-		}
-
-		err = -EIO;
-		gdp = ext4_get_group_desc(sb, group, &gdp_bh);
-		if (!gdp)
-			break;
-
-		ext4_lock_group(sb, group);
-		already = 0;
-		for (i = 0; i < clen; i++)
-			if (!mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
-					 !state)
-				already++;
-
-		clen_changed = clen - already;
-		if (state)
-			mb_set_bits(bitmap_bh->b_data, blkoff, clen);
-		else
-			mb_clear_bits(bitmap_bh->b_data, blkoff, clen);
-		if (ext4_has_group_desc_csum(sb) &&
-		    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
-			gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
-			ext4_free_group_clusters_set(sb, gdp,
-			     ext4_free_clusters_after_init(sb, group, gdp));
-		}
-		if (state)
-			clen = ext4_free_group_clusters(sb, gdp) - clen_changed;
-		else
-			clen = ext4_free_group_clusters(sb, gdp) + clen_changed;
-
-		ext4_free_group_clusters_set(sb, gdp, clen);
-		ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
-		ext4_group_desc_csum_set(sb, group, gdp);
-
-		ext4_unlock_group(sb, group);
-
-		if (sbi->s_log_groups_per_flex) {
-			ext4_group_t flex_group = ext4_flex_group(sbi, group);
-			struct flex_groups *fg = sbi_array_rcu_deref(sbi,
-						   s_flex_groups, flex_group);
-
-			if (state)
-				atomic64_sub(clen_changed, &fg->free_clusters);
-			else
-				atomic64_add(clen_changed, &fg->free_clusters);
-
-		}
-
-		err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
-		if (err)
-			break;
-		sync_dirty_buffer(bitmap_bh);
-		err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
-		sync_dirty_buffer(gdp_bh);
+		err = ext4_mb_mark_group_bb(&mc, group, blkoff, clen);
 		if (err)
 			break;
 
 		block += thisgrp_len;
 		len -= thisgrp_len;
-		brelse(bitmap_bh);
 		BUG_ON(len < 0);
 	}
-
-	if (err)
-		brelse(bitmap_bh);
 }
 
 /*
-- 
2.30.0

[PATCH v5 2/8] ext4: call ext4_mb_mark_group_bb in ext4_free_blocks_simple

[Kemeng Shi]

call ext4_mb_mark_group_bb in ext4_free_blocks_simple to:
1. remove repeat code
2. pair update of free_clusters in ext4_mb_new_blocks_simple.
3. add missing ext4_lock_group/ext4_unlock_group protection.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 58864a9116c0..6a527cd4b92e 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6312,43 +6312,16 @@ ext4_mb_free_metadata(handle_t *handle, struct ext4_buddy *e4b,
 static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
 					unsigned long count)
 {
-	struct buffer_head *bitmap_bh;
+	struct ext4_mark_context mc = {
+		.sb = inode->i_sb,
+		.state = 0,
+	};
 	struct super_block *sb = inode->i_sb;
-	struct ext4_group_desc *gdp;
-	struct buffer_head *gdp_bh;
 	ext4_group_t group;
 	ext4_grpblk_t blkoff;
-	int already_freed = 0, err, i;
 
 	ext4_get_group_no_and_offset(sb, block, &group, &blkoff);
-	bitmap_bh = ext4_read_block_bitmap(sb, group);
-	if (IS_ERR(bitmap_bh)) {
-		pr_warn("Failed to read block bitmap\n");
-		return;
-	}
-	gdp = ext4_get_group_desc(sb, group, &gdp_bh);
-	if (!gdp)
-		goto err_out;
-
-	for (i = 0; i < count; i++) {
-		if (!mb_test_bit(blkoff + i, bitmap_bh->b_data))
-			already_freed++;
-	}
-	mb_clear_bits(bitmap_bh->b_data, blkoff, count);
-	err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
-	if (err)
-		goto err_out;
-	ext4_free_group_clusters_set(
-		sb, gdp, ext4_free_group_clusters(sb, gdp) +
-		count - already_freed);
-	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
-	ext4_group_desc_csum_set(sb, group, gdp);
-	ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
-	sync_dirty_buffer(bitmap_bh);
-	sync_dirty_buffer(gdp_bh);
-
-err_out:
-	brelse(bitmap_bh);
+	ext4_mb_mark_group_bb(&mc, group, blkoff, count);
 }
 
 /**
-- 
2.30.0

[PATCH v5 3/8] ext4: extent ext4_mb_mark_group_bb to support allocation under journal

[Kemeng Shi]

Previously, ext4_mb_mark_group_bb is only called under fast commit
replay path, so there is no valid handle when we update block bitmap
and group descriptor. This patch try to extent ext4_mb_mark_group_bb
to be used by code under journal. There are several improves:
1. add "handle_t *handle" to struct ext4_mark_context to accept handle
to journal block bitmap and group descriptor update inside
ext4_mb_mark_group_bb (the added journal caode is based on journal
code in ext4_mb_mark_diskspace_used where ext4_mb_mark_group_bb
is going to be used.)
2. add EXT4_MB_BITMAP_MARKED_CHECK flag to control check if bits in block
bitmap are already marked as allocation code under journal asserts that
all bits to be changed are not marked before.
3. add "ext4_grpblk_t changed" to struct ext4_mark_context to notify number
of bits in block bitmap has changed.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 6a527cd4b92e..b1d8e140a3fa 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3948,32 +3948,54 @@ void ext4_exit_mballoc(void)
 	ext4_groupinfo_destroy_slabs();
 }
 
+#define EXT4_MB_BITMAP_MARKED_CHECK 0x0001
+#define EXT4_MB_SYNC_UPDATE 0x0002
 struct ext4_mark_context {
+	handle_t *handle;
 	struct super_block *sb;
 	int state;
+	ext4_grpblk_t changed;
 };
 
 static int
 ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
-		      ext4_grpblk_t blkoff, ext4_grpblk_t len)
+		      ext4_grpblk_t blkoff, ext4_grpblk_t len, int flags)
 {
+	handle_t *handle = mc->handle;
 	struct super_block *sb = mc->sb;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct buffer_head *bitmap_bh = NULL;
 	struct ext4_group_desc *gdp;
 	struct buffer_head *gdp_bh;
 	int err;
-	unsigned int i, already, changed;
+	unsigned int i, already, changed = len;
 
+	mc->changed = 0;
 	bitmap_bh = ext4_read_block_bitmap(sb, group);
 	if (IS_ERR(bitmap_bh))
 		return PTR_ERR(bitmap_bh);
 
+	if (handle) {
+		BUFFER_TRACE(bitmap_bh, "getting write access");
+		err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
+						    EXT4_JTR_NONE);
+		if (err)
+			goto out_err;
+	}
+
 	err = -EIO;
 	gdp = ext4_get_group_desc(sb, group, &gdp_bh);
 	if (!gdp)
 		goto out_err;
 
+	if (handle) {
+		BUFFER_TRACE(gdp_bh, "get_write_access");
+		err = ext4_journal_get_write_access(handle, sb, gdp_bh,
+						    EXT4_JTR_NONE);
+		if (err)
+			goto out_err;
+	}
+
 	ext4_lock_group(sb, group);
 	if (ext4_has_group_desc_csum(sb) &&
 	    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
@@ -3982,12 +4004,14 @@ ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
 			ext4_free_clusters_after_init(sb, group, gdp));
 	}
 
-	already = 0;
-	for (i = 0; i < len; i++)
-		if (mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
-				mc->state)
-			already++;
-	changed = len - already;
+	if (flags & EXT4_MB_BITMAP_MARKED_CHECK) {
+		already = 0;
+		for (i = 0; i < len; i++)
+			if (mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
+					mc->state)
+				already++;
+		changed = len - already;
+	}
 
 	if (mc->state) {
 		mb_set_bits(bitmap_bh->b_data, blkoff, len);
@@ -4002,6 +4026,7 @@ ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
 	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
 	ext4_group_desc_csum_set(sb, group, gdp);
 	ext4_unlock_group(sb, group);
+	mc->changed = changed;
 
 	if (sbi->s_log_groups_per_flex) {
 		ext4_group_t flex_group = ext4_flex_group(sbi, group);
@@ -4014,15 +4039,17 @@ ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
 			atomic64_add(changed, &fg->free_clusters);
 	}
 
-	err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
+	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
 	if (err)
 		goto out_err;
-	err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
+	err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh);
 	if (err)
 		goto out_err;
 
-	sync_dirty_buffer(bitmap_bh);
-	sync_dirty_buffer(gdp_bh);
+	if (flags & EXT4_MB_SYNC_UPDATE) {
+		sync_dirty_buffer(bitmap_bh);
+		sync_dirty_buffer(gdp_bh);
+	}
 
 out_err:
 	brelse(bitmap_bh);
@@ -4186,7 +4213,9 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
 			break;
 		}
 
-		err = ext4_mb_mark_group_bb(&mc, group, blkoff, clen);
+		err = ext4_mb_mark_group_bb(&mc, group, blkoff, clen,
+					    EXT4_MB_BITMAP_MARKED_CHECK |
+					    EXT4_MB_SYNC_UPDATE);
 		if (err)
 			break;
 
@@ -6321,7 +6350,9 @@ static void ext4_free_blocks_simple(struct inode *inode, ext4_fsblk_t block,
 	ext4_grpblk_t blkoff;
 
 	ext4_get_group_no_and_offset(sb, block, &group, &blkoff);
-	ext4_mb_mark_group_bb(&mc, group, blkoff, count);
+	ext4_mb_mark_group_bb(&mc, group, blkoff, count,
+			      EXT4_MB_BITMAP_MARKED_CHECK |
+			      EXT4_MB_SYNC_UPDATE);
 }
 
 /**
-- 
2.30.0

[PATCH v5 4/8] ext4: call ext4_mb_mark_group_bb in ext4_mb_mark_diskspace_used

[Kemeng Shi]

call ext4_mb_mark_group_bb in ext4_mb_mark_diskspace_used to:
1. remove repeat code to normally update bitmap and group descriptor
on disk.
2. call ext4_mb_mark_group_bb instead of only setting bits in block bitmap
to fix the bitmap. Function ext4_mb_mark_group_bb will also update
checksum of bitmap and other counter along with the bit change to keep
the cosistent with bit change or block bitmap will be marked corrupted as
checksum of bitmap is in inconsistent state.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b1d8e140a3fa..34fd12aeaf8d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4064,13 +4064,17 @@ static noinline_for_stack int
 ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 				handle_t *handle, unsigned int reserv_clstrs)
 {
-	struct buffer_head *bitmap_bh = NULL;
+	struct ext4_mark_context mc = {
+		.handle = handle,
+		.sb = ac->ac_sb,
+		.state = 1,
+	};
 	struct ext4_group_desc *gdp;
-	struct buffer_head *gdp_bh;
 	struct ext4_sb_info *sbi;
 	struct super_block *sb;
 	ext4_fsblk_t block;
 	int err, len;
+	int flags = 0;
 
 	BUG_ON(ac->ac_status != AC_STATUS_FOUND);
 	BUG_ON(ac->ac_b_ex.fe_len <= 0);
@@ -4078,32 +4082,13 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 	sb = ac->ac_sb;
 	sbi = EXT4_SB(sb);
 
-	bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group);
-	if (IS_ERR(bitmap_bh)) {
-		return PTR_ERR(bitmap_bh);
-	}
-
-	BUFFER_TRACE(bitmap_bh, "getting write access");
-	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
-					    EXT4_JTR_NONE);
-	if (err)
-		goto out_err;
-
-	err = -EIO;
-	gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, &gdp_bh);
+	gdp = ext4_get_group_desc(sb, ac->ac_b_ex.fe_group, NULL);
 	if (!gdp)
-		goto out_err;
-
+		return -EIO;
 	ext4_debug("using block group %u(%d)\n", ac->ac_b_ex.fe_group,
 			ext4_free_group_clusters(sb, gdp));
 
-	BUFFER_TRACE(gdp_bh, "get_write_access");
-	err = ext4_journal_get_write_access(handle, sb, gdp_bh, EXT4_JTR_NONE);
-	if (err)
-		goto out_err;
-
 	block = ext4_grp_offs_to_block(sb, &ac->ac_b_ex);
-
 	len = EXT4_C2B(sbi, ac->ac_b_ex.fe_len);
 	if (!ext4_inode_block_valid(ac->ac_inode, block, len)) {
 		ext4_error(sb, "Allocating blocks %llu-%llu which overlap "
@@ -4112,41 +4097,28 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 		 * Fix the bitmap and return EFSCORRUPTED
 		 * We leak some of the blocks here.
 		 */
-		ext4_lock_group(sb, ac->ac_b_ex.fe_group);
-		mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start,
-			      ac->ac_b_ex.fe_len);
-		ext4_unlock_group(sb, ac->ac_b_ex.fe_group);
-		err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
+		err = ext4_mb_mark_group_bb(&mc, ac->ac_b_ex.fe_group,
+					    ac->ac_b_ex.fe_start,
+					    ac->ac_b_ex.fe_len,
+					    0);
 		if (!err)
 			err = -EFSCORRUPTED;
-		goto out_err;
+		return err;
 	}
 
-	ext4_lock_group(sb, ac->ac_b_ex.fe_group);
 #ifdef AGGRESSIVE_CHECK
-	{
-		int i;
-		for (i = 0; i < ac->ac_b_ex.fe_len; i++) {
-			BUG_ON(mb_test_bit(ac->ac_b_ex.fe_start + i,
-						bitmap_bh->b_data));
-		}
-	}
+	flags |= EXT4_MB_BITMAP_MARKED_CHECK;
 #endif
-	mb_set_bits(bitmap_bh->b_data, ac->ac_b_ex.fe_start,
-		      ac->ac_b_ex.fe_len);
-	if (ext4_has_group_desc_csum(sb) &&
-	    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
-		gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
-		ext4_free_group_clusters_set(sb, gdp,
-					     ext4_free_clusters_after_init(sb,
-						ac->ac_b_ex.fe_group, gdp));
-	}
-	len = ext4_free_group_clusters(sb, gdp) - ac->ac_b_ex.fe_len;
-	ext4_free_group_clusters_set(sb, gdp, len);
-	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
-	ext4_group_desc_csum_set(sb, ac->ac_b_ex.fe_group, gdp);
+	err = ext4_mb_mark_group_bb(&mc, ac->ac_b_ex.fe_group,
+				    ac->ac_b_ex.fe_start, ac->ac_b_ex.fe_len,
+				    flags);
+
+	if (err && mc.changed == 0)
+		return err;
 
-	ext4_unlock_group(sb, ac->ac_b_ex.fe_group);
+#ifdef AGGRESSIVE_CHECK
+	BUG_ON(mc.changed != ac->ac_b_ex.fe_len);
+#endif
 	percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len);
 	/*
 	 * Now reduce the dirty block count also. Should not go negative
@@ -4156,21 +4128,6 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 		percpu_counter_sub(&sbi->s_dirtyclusters_counter,
 				   reserv_clstrs);
 
-	if (sbi->s_log_groups_per_flex) {
-		ext4_group_t flex_group = ext4_flex_group(sbi,
-							  ac->ac_b_ex.fe_group);
-		atomic64_sub(ac->ac_b_ex.fe_len,
-			     &sbi_array_rcu_deref(sbi, s_flex_groups,
-						  flex_group)->free_clusters);
-	}
-
-	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
-	if (err)
-		goto out_err;
-	err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh);
-
-out_err:
-	brelse(bitmap_bh);
 	return err;
 }
 
-- 
2.30.0

[PATCH v5 5/8] ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb

[Kemeng Shi]

call ext4_mb_mark_group_bb in ext4_mb_clear_bb to remove repeat code
to update block bitmap and group descriptor on disk.

Note: ext4_mb_clear_bb will update buddy and bitmap in two critical
sections instead of update in the same critical section.

Original lock behavior introduced in commit 7a2fcbf7f8573 ("ext4: don't
use blocks freed but not yet committed in buddy cache init") to avoid
race betwwen ext4_mb_free_blocks and ext4_mb_init_cache:
ext4_mb_load_buddy_gfp
ext4_lock_group
mb_clear_bits(bitmap_bh, ...)
mb_free_blocks/ext4_mb_free_metadata
ext4_unlock_group
ext4_mb_unload_buddy

New lock behavior in this patch:
ext4_mb_load_buddy_gfp
ext4_lock_group
mb_clear_bits(bitmap_bh, ...)
ext4_unlock_group

/* no ext4_mb_init_cache for the same group will be called as
ext4_mb_load_buddy_gfp will ensure buddy page is update-to-date */

ext4_lock_group
mb_free_blocks/ext4_mb_free_metadata
ext4_unlock_group
ext4_mb_unload_buddy

As buddy page for group is always update-to-date between
ext4_mb_load_buddy_gfp and ext4_mb_unload_buddy. Then no
ext4_mb_init_cache will be called for the same group concurrentlly when
we update bitmap and buddy page betwwen buddy load and unload.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 34fd12aeaf8d..57cc304b724e 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6325,19 +6325,21 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 			       ext4_fsblk_t block, unsigned long count,
 			       int flags)
 {
-	struct buffer_head *bitmap_bh = NULL;
+	struct ext4_mark_context mc = {
+		.handle = handle,
+		.sb = inode->i_sb,
+		.state = 0,
+	};
 	struct super_block *sb = inode->i_sb;
-	struct ext4_group_desc *gdp;
 	struct ext4_group_info *grp;
 	unsigned int overflow;
 	ext4_grpblk_t bit;
-	struct buffer_head *gd_bh;
 	ext4_group_t block_group;
 	struct ext4_sb_info *sbi;
 	struct ext4_buddy e4b;
 	unsigned int count_clusters;
 	int err = 0;
-	int ret;
+	int mark_flags = 0;
 
 	sbi = EXT4_SB(sb);
 
@@ -6369,18 +6371,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 		/* The range changed so it's no longer validated */
 		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
 	}
-	count_clusters = EXT4_NUM_B2C(sbi, count);
-	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
-	if (IS_ERR(bitmap_bh)) {
-		err = PTR_ERR(bitmap_bh);
-		bitmap_bh = NULL;
-		goto error_return;
-	}
-	gdp = ext4_get_group_desc(sb, block_group, &gd_bh);
-	if (!gdp) {
-		err = -EIO;
-		goto error_return;
-	}
 
 	if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
 	    !ext4_inode_block_valid(inode, block, count)) {
@@ -6390,28 +6380,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 		goto error_return;
 	}
 
-	BUFFER_TRACE(bitmap_bh, "getting write access");
-	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
-					    EXT4_JTR_NONE);
-	if (err)
-		goto error_return;
-
-	/*
-	 * We are about to modify some metadata.  Call the journal APIs
-	 * to unshare ->b_data if a currently-committing transaction is
-	 * using it
-	 */
-	BUFFER_TRACE(gd_bh, "get_write_access");
-	err = ext4_journal_get_write_access(handle, sb, gd_bh, EXT4_JTR_NONE);
-	if (err)
-		goto error_return;
-#ifdef AGGRESSIVE_CHECK
-	{
-		int i;
-		for (i = 0; i < count_clusters; i++)
-			BUG_ON(!mb_test_bit(bit + i, bitmap_bh->b_data));
-	}
-#endif
+	count_clusters = EXT4_NUM_B2C(sbi, count);
 	trace_ext4_mballoc_free(sb, inode, block_group, bit, count_clusters);
 
 	/* __GFP_NOFAIL: retry infinitely, ignore TIF_MEMDIE and memcg limit. */
@@ -6420,6 +6389,22 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 	if (err)
 		goto error_return;
 
+#ifdef AGGRESSIVE_CHECK
+	mark_flags |= EXT4_MB_BITMAP_MARKED_CHECK;
+#endif
+	err = ext4_mb_mark_group_bb(&mc, block_group, bit, count_clusters,
+				    mark_flags);
+
+
+	if (err && mc.changed == 0) {
+		ext4_mb_unload_buddy(&e4b);
+		goto error_return;
+	}
+
+#ifdef AGGRESSIVE_CHECK
+	BUG_ON(mc.changed != count_clusters);
+#endif
+
 	/*
 	 * We need to make sure we don't reuse the freed block until after the
 	 * transaction is committed. We make an exception if the inode is to be
@@ -6442,13 +6427,8 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 		new_entry->efd_tid = handle->h_transaction->t_tid;
 
 		ext4_lock_group(sb, block_group);
-		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
 		ext4_mb_free_metadata(handle, &e4b, new_entry);
 	} else {
-		/* need to update group_info->bb_free and bitmap
-		 * with group lock held. generate_buddy look at
-		 * them with group lock_held
-		 */
 		if (test_opt(sb, DISCARD)) {
 			err = ext4_issue_discard(sb, block_group, bit,
 						 count_clusters, NULL);
@@ -6461,23 +6441,11 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 			EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info);
 
 		ext4_lock_group(sb, block_group);
-		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
 		mb_free_blocks(inode, &e4b, bit, count_clusters);
 	}
 
-	ret = ext4_free_group_clusters(sb, gdp) + count_clusters;
-	ext4_free_group_clusters_set(sb, gdp, ret);
-	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
-	ext4_group_desc_csum_set(sb, block_group, gdp);
 	ext4_unlock_group(sb, block_group);
 
-	if (sbi->s_log_groups_per_flex) {
-		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
-		atomic64_add(count_clusters,
-			     &sbi_array_rcu_deref(sbi, s_flex_groups,
-						  flex_group)->free_clusters);
-	}
-
 	/*
 	 * on a bigalloc file system, defer the s_freeclusters_counter
 	 * update to the caller (ext4_remove_space and friends) so they
@@ -6492,26 +6460,14 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
 
 	ext4_mb_unload_buddy(&e4b);
 
-	/* We dirtied the bitmap block */
-	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
-	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
-
-	/* And the group descriptor block */
-	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
-	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
-	if (!err)
-		err = ret;
-
 	if (overflow && !err) {
 		block += count;
 		count = overflow;
-		put_bh(bitmap_bh);
 		/* The range changed so it's no longer validated */
 		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
 		goto do_more;
 	}
 error_return:
-	brelse(bitmap_bh);
 	ext4_std_error(sb, err);
 	return;
 }
-- 
2.30.0

[PATCH v5 6/8] ext4: call ext4_mb_mark_group_bb in ext4_group_add_blocks

[Kemeng Shi]

call ext4_mb_mark_group_bb in ext4_group_add_blocks to remove repeat code
to update block bitmap and group descriptor on disk.

Note: ext4_group_add_blocks will update buddy and bitmap in two critical
sections instead of update in the same critical.

Originally:
ext4_mb_load_buddy_gfp
ext4_lock_group
mb_clear_bits(bitmap_bh, ...)
mb_free_blocks/ext4_mb_free_metadata
ext4_unlock_group
ext4_mb_unload_buddy

Now:
ext4_mb_load_buddy_gfp
ext4_lock_group
mb_clear_bits(bitmap_bh, ...)
ext4_unlock_group

/* no ext4_mb_init_cache for the same group will be called as
ext4_mb_load_buddy_gfp will ensure buddy page is update-to-date */

ext4_lock_group
mb_free_blocks/ext4_mb_free_metadata
ext4_unlock_group
ext4_mb_unload_buddy

As buddy page for group is always update-to-date between
ext4_mb_load_buddy_gfp and ext4_mb_unload_buddy. Then no
ext4_mb_init_cache will be called for the same group concurrentlly when
we update bitmap and buddy page betwwen buddy load and unload.

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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 57cc304b724e..54254f08ea0c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6587,23 +6587,23 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 			 ext4_fsblk_t block, unsigned long count)
 {
-	struct buffer_head *bitmap_bh = NULL;
-	struct buffer_head *gd_bh;
+	struct ext4_mark_context mc = {
+		.handle = handle,
+		.sb = sb,
+		.state = 0,
+	};
 	ext4_group_t block_group;
 	ext4_grpblk_t bit;
-	unsigned int i;
-	struct ext4_group_desc *desc;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_buddy e4b;
-	int err = 0, ret, free_clusters_count;
-	ext4_grpblk_t clusters_freed;
+	int err = 0;
 	ext4_fsblk_t first_cluster = EXT4_B2C(sbi, block);
 	ext4_fsblk_t last_cluster = EXT4_B2C(sbi, block + count - 1);
 	unsigned long cluster_count = last_cluster - first_cluster + 1;
 
 	ext4_debug("Adding block(s) %llu-%llu\n", block, block + count - 1);
 
-	if (count == 0)
+	if (cluster_count == 0)
 		return 0;
 
 	ext4_get_group_no_and_offset(sb, block, &block_group, &bit);
@@ -6618,19 +6618,6 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 		goto error_return;
 	}
 
-	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
-	if (IS_ERR(bitmap_bh)) {
-		err = PTR_ERR(bitmap_bh);
-		bitmap_bh = NULL;
-		goto error_return;
-	}
-
-	desc = ext4_get_group_desc(sb, block_group, &gd_bh);
-	if (!desc) {
-		err = -EIO;
-		goto error_return;
-	}
-
 	if (!ext4_sb_block_valid(sb, NULL, block, count)) {
 		ext4_error(sb, "Adding blocks in system zones - "
 			   "Block = %llu, count = %lu",
@@ -6639,75 +6626,30 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 		goto error_return;
 	}
 
-	BUFFER_TRACE(bitmap_bh, "getting write access");
-	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
-					    EXT4_JTR_NONE);
-	if (err)
-		goto error_return;
-
-	/*
-	 * We are about to modify some metadata.  Call the journal APIs
-	 * to unshare ->b_data if a currently-committing transaction is
-	 * using it
-	 */
-	BUFFER_TRACE(gd_bh, "get_write_access");
-	err = ext4_journal_get_write_access(handle, sb, gd_bh, EXT4_JTR_NONE);
+	err = ext4_mb_load_buddy(sb, block_group, &e4b);
 	if (err)
 		goto error_return;
 
-	for (i = 0, clusters_freed = 0; i < cluster_count; i++) {
-		BUFFER_TRACE(bitmap_bh, "clear bit");
-		if (!mb_test_bit(bit + i, bitmap_bh->b_data)) {
-			ext4_error(sb, "bit already cleared for block %llu",
-				   (ext4_fsblk_t)(block + i));
-			BUFFER_TRACE(bitmap_bh, "bit already cleared");
-		} else {
-			clusters_freed++;
-		}
-	}
+	err = ext4_mb_mark_group_bb(&mc, block_group, bit, cluster_count,
+				    EXT4_MB_BITMAP_MARKED_CHECK);
 
-	err = ext4_mb_load_buddy(sb, block_group, &e4b);
-	if (err)
+	if (err && mc.changed == 0) {
+		ext4_mb_unload_buddy(&e4b);
 		goto error_return;
+	}
 
-	/*
-	 * need to update group_info->bb_free and bitmap
-	 * with group lock held. generate_buddy look at
-	 * them with group lock_held
-	 */
+	if (mc.changed != cluster_count)
+		ext4_error(sb, "bit already cleared in group %u",
+			   block_group);
 	ext4_lock_group(sb, block_group);
-	mb_clear_bits(bitmap_bh->b_data, bit, cluster_count);
 	mb_free_blocks(NULL, &e4b, bit, cluster_count);
-	free_clusters_count = clusters_freed +
-		ext4_free_group_clusters(sb, desc);
-	ext4_free_group_clusters_set(sb, desc, free_clusters_count);
-	ext4_block_bitmap_csum_set(sb, desc, bitmap_bh);
-	ext4_group_desc_csum_set(sb, block_group, desc);
 	ext4_unlock_group(sb, block_group);
 	percpu_counter_add(&sbi->s_freeclusters_counter,
-			   clusters_freed);
-
-	if (sbi->s_log_groups_per_flex) {
-		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
-		atomic64_add(clusters_freed,
-			     &sbi_array_rcu_deref(sbi, s_flex_groups,
-						  flex_group)->free_clusters);
-	}
+			   mc.changed);
 
 	ext4_mb_unload_buddy(&e4b);
 
-	/* We dirtied the bitmap block */
-	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
-	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
-
-	/* And the group descriptor block */
-	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
-	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
-	if (!err)
-		err = ret;
-
 error_return:
-	brelse(bitmap_bh);
 	ext4_std_error(sb, err);
 	return err;
 }
-- 
2.30.0

[PATCH v5 7/8] ext4: add some kunit stub for mballoc kunit test

[Kemeng Shi]

Multiblocks allocation will read and write block bitmap and group
descriptor which reside on disk. Add kunit stub to function
ext4_get_group_desc, ext4_read_block_bitmap_nowait, ext4_wait_block_bitmap
and ext4_mb_mark_group_bb to avoid real IO to disk.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/balloc.c  | 16 ++++++++++++++++
 fs/ext4/mballoc.c |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 1f72f977c6db..d39655fb2f53 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -22,6 +22,7 @@
 #include "mballoc.h"
 
 #include <trace/events/ext4.h>
+#include <kunit/static_stub.h>
 
 static unsigned ext4_num_base_meta_clusters(struct super_block *sb,
 					    ext4_group_t block_group);
@@ -274,6 +275,11 @@ struct ext4_group_desc * ext4_get_group_desc(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct buffer_head *bh_p;
 
+#ifdef CONFIG_EXT4_KUNIT_TESTS
+	KUNIT_STATIC_STUB_REDIRECT(ext4_get_group_desc,
+				   sb, block_group, bh);
+#endif
+
 	if (block_group >= ngroups) {
 		ext4_error(sb, "block_group >= groups_count - block_group = %u,"
 			   " groups_count = %u", block_group, ngroups);
@@ -468,6 +474,11 @@ ext4_read_block_bitmap_nowait(struct super_block *sb, ext4_group_t block_group,
 	ext4_fsblk_t bitmap_blk;
 	int err;
 
+#ifdef CONFIG_EXT4_KUNIT_TESTS
+	KUNIT_STATIC_STUB_REDIRECT(ext4_read_block_bitmap_nowait,
+				   sb, block_group, ignore_locked);
+#endif
+
 	desc = ext4_get_group_desc(sb, block_group, NULL);
 	if (!desc)
 		return ERR_PTR(-EFSCORRUPTED);
@@ -563,6 +574,11 @@ int ext4_wait_block_bitmap(struct super_block *sb, ext4_group_t block_group,
 {
 	struct ext4_group_desc *desc;
 
+#ifdef CONFIG_EXT4_KUNIT_TESTS
+	KUNIT_STATIC_STUB_REDIRECT(ext4_wait_block_bitmap,
+				   sb, block_group, bh);
+#endif
+
 	if (!buffer_new(bh))
 		return 0;
 	desc = ext4_get_group_desc(sb, block_group, NULL);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 54254f08ea0c..dd2fc0546c0b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -17,6 +17,7 @@
 #include <linux/nospec.h>
 #include <linux/backing-dev.h>
 #include <trace/events/ext4.h>
+#include <kunit/static_stub.h>
 
 /*
  * MUSTDO:
@@ -3970,6 +3971,11 @@ ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
 	int err;
 	unsigned int i, already, changed = len;
 
+#ifdef CONFIG_EXT4_KUNIT_TESTS
+	KUNIT_STATIC_STUB_REDIRECT(ext4_mb_mark_group_bb,
+				   mc, group, blkoff, len, flags);
+#endif
+
 	mc->changed = 0;
 	bitmap_bh = ext4_read_block_bitmap(sb, group);
 	if (IS_ERR(bitmap_bh))
-- 
2.30.0

[PATCH v5 8/8] ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

[Kemeng Shi]

Here are prepared work:
1. Include mballoc-test.c to mballoc.c to be able test static function
in mballoc.c.
2. Implement static stub to avoid read IO to disk.
3. Construct fake super_block. Only partial members are set, more members
will be set when more functions are tested.
Then unit test for ext4_mb_new_blocks_simple is added.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/mballoc-test.c | 323 +++++++++++++++++++++++++++++++++++++++++
 fs/ext4/mballoc.c      |   4 +
 2 files changed, 327 insertions(+)
 create mode 100644 fs/ext4/mballoc-test.c

diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
new file mode 100644
index 000000000000..184e6cb2070f
--- /dev/null
+++ b/fs/ext4/mballoc-test.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test of ext4 multiblocks allocation.
+ */
+
+#include <kunit/test.h>
+#include <kunit/static_stub.h>
+
+#include "ext4.h"
+
+struct mb_grp_ctx {
+	struct buffer_head bitmap_bh;
+	struct ext4_group_desc desc;
+	/* one group descriptor for each group descriptor for simplicity */
+	struct buffer_head gd_bh;
+};
+
+struct mb_ctx {
+	struct mb_grp_ctx *grp_ctx;
+};
+
+struct fake_super_block {
+	struct super_block sb;
+	struct mb_ctx mb_ctx;
+};
+
+#define MB_CTX(_sb) (&(container_of((_sb), struct fake_super_block, sb)->mb_ctx))
+#define MB_GRP_CTX(_sb, _group) (&MB_CTX(_sb)->grp_ctx[_group])
+
+static struct super_block *alloc_fake_super_block(void)
+{
+	struct ext4_super_block *es = kzalloc(sizeof(*es), GFP_KERNEL);
+	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
+	struct fake_super_block *fsb = kzalloc(sizeof(*fsb), GFP_KERNEL);
+
+	if (fsb == NULL || sbi == NULL || es == NULL)
+		goto out;
+
+	sbi->s_es = es;
+	fsb->sb.s_fs_info = sbi;
+	return &fsb->sb;
+
+out:
+	kfree(fsb);
+	kfree(sbi);
+	kfree(es);
+	return NULL;
+}
+
+static void free_fake_super_block(struct super_block *sb)
+{
+	struct fake_super_block *fsb = container_of(sb, struct fake_super_block, sb);
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	kfree(sbi->s_es);
+	kfree(sbi);
+	kfree(fsb);
+}
+
+struct ext4_block_layout {
+	unsigned char blocksize_bits;
+	unsigned int cluster_bits;
+	unsigned long blocks_per_group;
+	ext4_group_t group_count;
+	unsigned long desc_size;
+};
+
+static void init_sb_layout(struct super_block *sb,
+			  struct ext4_block_layout *layout)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+
+	sb->s_blocksize = 1UL << layout->blocksize_bits;
+	sb->s_blocksize_bits = layout->blocksize_bits;
+
+	sbi->s_groups_count = layout->group_count;
+	sbi->s_blocks_per_group = layout->blocks_per_group;
+	sbi->s_cluster_bits = layout->cluster_bits;
+	sbi->s_cluster_ratio = 1U << layout->cluster_bits;
+	sbi->s_clusters_per_group = layout->blocks_per_group >>
+				    layout->cluster_bits;
+	sbi->s_desc_size = layout->desc_size;
+
+	es->s_first_data_block = cpu_to_le32(0);
+	es->s_blocks_count_lo = cpu_to_le32(layout->blocks_per_group *
+					    layout->group_count);
+}
+
+static int mb_grp_ctx_init(struct super_block *sb,
+			   struct mb_grp_ctx *grp_ctx)
+{
+	grp_ctx->bitmap_bh.b_data = kzalloc(EXT4_BLOCK_SIZE(sb), GFP_KERNEL);
+	if (grp_ctx->bitmap_bh.b_data == NULL)
+		return -ENOMEM;
+
+	get_bh(&grp_ctx->bitmap_bh);
+	get_bh(&grp_ctx->gd_bh);
+	return 0;
+}
+
+static void mb_grp_ctx_release(struct mb_grp_ctx *grp_ctx)
+{
+	kfree(grp_ctx->bitmap_bh.b_data);
+	grp_ctx->bitmap_bh.b_data = NULL;
+}
+
+static void mb_ctx_mark_used(struct super_block *sb, ext4_group_t group,
+			     unsigned int start, unsigned int len)
+{
+	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, group);
+
+	mb_set_bits(grp_ctx->bitmap_bh.b_data, start, len);
+}
+
+/* called after init_sb_layout */
+static int mb_ctx_init(struct super_block *sb)
+{
+	struct mb_ctx *ctx = MB_CTX(sb);
+	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
+
+	ctx->grp_ctx = kcalloc(ngroups, sizeof(struct mb_grp_ctx),
+			       GFP_KERNEL);
+	if (ctx->grp_ctx == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < ngroups; i++)
+		if (mb_grp_ctx_init(sb, &ctx->grp_ctx[i]))
+			goto out;
+
+	/*
+	 * first data block(first cluster in first group) is used by
+	 * metadata, mark it used to avoid to alloc data block at first
+	 * block which will fail ext4_sb_block_valid check.
+	 */
+	mb_set_bits(ctx->grp_ctx[0].bitmap_bh.b_data, 0, 1);
+
+	return 0;
+out:
+	while (i-- > 0)
+		mb_grp_ctx_release(&ctx->grp_ctx[i]);
+	kfree(ctx->grp_ctx);
+	return -ENOMEM;
+}
+
+static void mb_ctx_release(struct super_block *sb)
+{
+	struct mb_ctx *ctx = MB_CTX(sb);
+	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
+
+	for (i = 0; i < ngroups; i++)
+		mb_grp_ctx_release(&ctx->grp_ctx[i]);
+	kfree(ctx->grp_ctx);
+}
+
+static struct buffer_head *
+ext4_read_block_bitmap_nowait_stub(struct super_block *sb, ext4_group_t block_group,
+				   bool ignore_locked)
+{
+	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
+
+	get_bh(&grp_ctx->bitmap_bh);
+	return &grp_ctx->bitmap_bh;
+}
+
+static int ext4_wait_block_bitmap_stub(struct super_block *sb,
+				ext4_group_t block_group,
+				struct buffer_head *bh)
+{
+	return 0;
+}
+
+static struct ext4_group_desc *
+ext4_get_group_desc_stub(struct super_block *sb, ext4_group_t block_group,
+			 struct buffer_head **bh)
+{
+	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
+
+	if (bh != NULL)
+		*bh = &grp_ctx->gd_bh;
+
+	return &grp_ctx->desc;
+}
+
+static int ext4_mb_mark_group_bb_stub(struct ext4_mark_context *mc,
+			       ext4_group_t group, ext4_grpblk_t blkoff,
+			       ext4_grpblk_t len, int flags)
+{
+	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(mc->sb, group);
+	struct buffer_head *bitmap_bh = &grp_ctx->bitmap_bh;
+
+	if (mc->state)
+		mb_set_bits(bitmap_bh->b_data, blkoff, len);
+	else
+		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
+
+	return 0;
+}
+
+#define TEST_BLOCKSIZE_BITS 10
+#define TEST_CLUSTER_BITS 3
+#define TEST_BLOCKS_PER_GROUP 8192
+#define TEST_GROUP_COUNT 4
+#define TEST_DESC_SIZE 64
+#define TEST_GOAL_GROUP 1
+static int mballoc_test_init(struct kunit *test)
+{
+	struct ext4_block_layout layout = {
+		.blocksize_bits = TEST_BLOCKSIZE_BITS,
+		.cluster_bits = TEST_CLUSTER_BITS,
+		.blocks_per_group = TEST_BLOCKS_PER_GROUP,
+		.group_count = TEST_GROUP_COUNT,
+		.desc_size = TEST_DESC_SIZE,
+	};
+	struct super_block *sb;
+	int ret;
+
+	sb = alloc_fake_super_block();
+	if (sb == NULL)
+		return -ENOMEM;
+
+	init_sb_layout(sb, &layout);
+
+	ret = mb_ctx_init(sb);
+	if (ret != 0) {
+		free_fake_super_block(sb);
+		return ret;
+	}
+
+	test->priv = sb;
+	kunit_activate_static_stub(test,
+				   ext4_read_block_bitmap_nowait,
+				   ext4_read_block_bitmap_nowait_stub);
+	kunit_activate_static_stub(test,
+				   ext4_wait_block_bitmap,
+				   ext4_wait_block_bitmap_stub);
+	kunit_activate_static_stub(test,
+				   ext4_get_group_desc,
+				   ext4_get_group_desc_stub);
+	kunit_activate_static_stub(test,
+				   ext4_mb_mark_group_bb,
+				   ext4_mb_mark_group_bb_stub);
+	return 0;
+}
+
+static void mballoc_test_exit(struct kunit *test)
+{
+	struct super_block *sb = (struct super_block *)test->priv;
+
+	mb_ctx_release(sb);
+	free_fake_super_block(sb);
+}
+
+static void test_new_blocks_simple(struct kunit *test)
+{
+	struct super_block *sb = (struct super_block *)test->priv;
+	struct inode inode = { .i_sb = sb, };
+	struct ext4_allocation_request ar;
+	ext4_group_t i, goal_group = TEST_GOAL_GROUP;
+	int err = 0;
+	ext4_fsblk_t found;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	ar.inode = &inode;
+
+	/* get block at goal */
+	ar.goal = ext4_group_first_block_no(sb, goal_group);
+	found = ext4_mb_new_blocks_simple(&ar, &err);
+	KUNIT_ASSERT_EQ_MSG(test, ar.goal, found,
+		"failed to alloc block at goal, expected %llu found %llu",
+		ar.goal, found);
+
+	/* get block after goal in goal group */
+	ar.goal = ext4_group_first_block_no(sb, goal_group);
+	found = ext4_mb_new_blocks_simple(&ar, &err);
+	KUNIT_ASSERT_EQ_MSG(test, ar.goal + EXT4_C2B(sbi, 1), found,
+		"failed to alloc block after goal in goal group, expected %llu found %llu",
+		ar.goal + 1, found);
+
+	/* get block after goal group */
+	mb_ctx_mark_used(sb, goal_group, 0, EXT4_CLUSTERS_PER_GROUP(sb));
+	ar.goal = ext4_group_first_block_no(sb, goal_group);
+	found = ext4_mb_new_blocks_simple(&ar, &err);
+	KUNIT_ASSERT_EQ_MSG(test,
+		ext4_group_first_block_no(sb, goal_group + 1), found,
+		"failed to alloc block after goal group, expected %llu found %llu",
+		ext4_group_first_block_no(sb, goal_group + 1), found);
+
+	/* get block before goal group */
+	for (i = goal_group; i < ext4_get_groups_count(sb); i++)
+		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
+	ar.goal = ext4_group_first_block_no(sb, goal_group);
+	found = ext4_mb_new_blocks_simple(&ar, &err);
+	KUNIT_ASSERT_EQ_MSG(test,
+		ext4_group_first_block_no(sb, 0) + EXT4_C2B(sbi, 1), found,
+		"failed to alloc block before goal group, expected %llu found %llu",
+		ext4_group_first_block_no(sb, 0 + EXT4_C2B(sbi, 1)), found);
+
+	/* no block available, fail to allocate block */
+	for (i = 0; i < ext4_get_groups_count(sb); i++)
+		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
+	ar.goal = ext4_group_first_block_no(sb, goal_group);
+	found = ext4_mb_new_blocks_simple(&ar, &err);
+	KUNIT_ASSERT_NE_MSG(test, err, 0,
+		"unexpectedly get block when no block is available");
+}
+
+
+static struct kunit_case ext4_mballoc_test_cases[] = {
+	KUNIT_CASE(test_new_blocks_simple),
+	{}
+};
+
+static struct kunit_suite ext4_mballoc_test_suite = {
+	.name = "ext4_mballoc_test",
+	.init = mballoc_test_init,
+	.exit = mballoc_test_exit,
+	.test_cases = ext4_mballoc_test_cases,
+};
+
+kunit_test_suites(&ext4_mballoc_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index dd2fc0546c0b..b6b963412cdc 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6954,3 +6954,7 @@ ext4_mballoc_query_range(
 
 	return error;
 }
+
+#ifdef CONFIG_EXT4_KUNIT_TESTS
+#include "mballoc-test.c"
+#endif
-- 
2.30.0

Re: [PATCH v5 1/8] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb

[Ritesh Harjani]

Kemeng Shi <shikemeng@huaweicloud.com> writes:

> There are several reasons to add a general function to update block
> bitmap and group descriptor on disk:
> 1. pair behavior of alloc/free bits. For example,
> ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups
> in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this.
> 2. remove repeat code to read from disk, update and write back to disk.
> 3. reduce future unit test mocks to catch real IO to update structure
> on disk.

Thanks for the cleanup and sorry that I am starting to review this
series only now. However I do have some review comments to understand a
bit more on the patch series. 

>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  fs/ext4/mballoc.c | 157 +++++++++++++++++++++++++---------------------
>  1 file changed, 87 insertions(+), 70 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index a2475b8c9fb5..58864a9116c0 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3948,6 +3948,86 @@ void ext4_exit_mballoc(void)
>  	ext4_groupinfo_destroy_slabs();
>  }
>  
> +struct ext4_mark_context {
> +	struct super_block *sb;
> +	int state;
> +};

It's not totally clear the intention behind this structure from above
since it lacking any comments.

Can you please help me understand why do we need this.
I still don't know whether we require this structure and what is it's
purpose. Is it only for reducing the number of variable passing?

Let me do more reading... 

...On more reading, I was previous considering to rename it to something
like ext4_mb_mark_context, but then I realized the naming of this is
something similar to ext4_allocation_context. So we may keep the naming
as is.

So since this structure, presumably, is used for marking blk bits for
mballoc. Why don't we pass useful information which is relevant for
this operation like - 

    ext4_mark_context {
        ext4_group_t mc_group;          /* block group */
        ext4_grpblk_t mc_clblk;	    /* block in cluster units */
        ext4_grpblk_t mc_cllen;	    /* len in cluster units */
        ext4_grpblk_t mc_clupdates;     /* number of clusters marked/unmarked */
        unsigned int mc_flags;          /* flags ... */
        bool mc_state;                  /* to set or unset state */
    };

Maybe, super_block and handle we can pass as an argument as those doesn't
define the ext4_mark_context for mballoc.

Since this structure is prepared not at the begining of any function, we
may need a prepare function for it. e.g. 

   static void ext4_mb_prepare_mark_context(&mc, ...)
   static int ext4_mb_mark_context(sb, handle, &mc);  (instead of ext4_mb_mark_group_bb())

Does this sounds better to you? Thoughts?

Otherwise I think having a common function for mb_mark_context looks
like a nice cleanup.

-ritesh

> +
> +static int
> +ext4_mb_mark_group_bb(struct ext4_mark_context *mc, ext4_group_t group,
> +		      ext4_grpblk_t blkoff, ext4_grpblk_t len)
> +{
> +	struct super_block *sb = mc->sb;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct buffer_head *bitmap_bh = NULL;
> +	struct ext4_group_desc *gdp;
> +	struct buffer_head *gdp_bh;
> +	int err;
> +	unsigned int i, already, changed;
> +
> +	bitmap_bh = ext4_read_block_bitmap(sb, group);
> +	if (IS_ERR(bitmap_bh))
> +		return PTR_ERR(bitmap_bh);
> +
> +	err = -EIO;
> +	gdp = ext4_get_group_desc(sb, group, &gdp_bh);
> +	if (!gdp)
> +		goto out_err;
> +
> +	ext4_lock_group(sb, group);
> +	if (ext4_has_group_desc_csum(sb) &&
> +	    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
> +		gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
> +		ext4_free_group_clusters_set(sb, gdp,
> +			ext4_free_clusters_after_init(sb, group, gdp));
> +	}
> +
> +	already = 0;
> +	for (i = 0; i < len; i++)
> +		if (mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
> +				mc->state)
> +			already++;
> +	changed = len - already;
> +
> +	if (mc->state) {
> +		mb_set_bits(bitmap_bh->b_data, blkoff, len);
> +		ext4_free_group_clusters_set(sb, gdp,
> +			ext4_free_group_clusters(sb, gdp) - changed);
> +	} else {
> +		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
> +		ext4_free_group_clusters_set(sb, gdp,
> +			ext4_free_group_clusters(sb, gdp) + changed);
> +	}
> +
> +	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
> +	ext4_group_desc_csum_set(sb, group, gdp);
> +	ext4_unlock_group(sb, group);
> +
> +	if (sbi->s_log_groups_per_flex) {
> +		ext4_group_t flex_group = ext4_flex_group(sbi, group);
> +		struct flex_groups *fg = sbi_array_rcu_deref(sbi,
> +					   s_flex_groups, flex_group);
> +
> +		if (mc->state)
> +			atomic64_sub(changed, &fg->free_clusters);
> +		else
> +			atomic64_add(changed, &fg->free_clusters);
> +	}
> +
> +	err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
> +	if (err)
> +		goto out_err;
> +	err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
> +	if (err)
> +		goto out_err;
> +
> +	sync_dirty_buffer(bitmap_bh);
> +	sync_dirty_buffer(gdp_bh);
> +
> +out_err:
> +	brelse(bitmap_bh);
> +	return err;
> +}
>  
>  /*
>   * Check quota and mark chosen space (ac->ac_b_ex) non-free in bitmaps
> @@ -4074,15 +4154,15 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
>  void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
>  			int len, int state)
>  {
> -	struct buffer_head *bitmap_bh = NULL;
> -	struct ext4_group_desc *gdp;
> -	struct buffer_head *gdp_bh;
> +	struct ext4_mark_context mc = {
> +		.sb = sb,
> +		.state = state,
> +	};
>  	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  	ext4_group_t group;
>  	ext4_grpblk_t blkoff;
> -	int i, err;
> -	int already;
> -	unsigned int clen, clen_changed, thisgrp_len;
> +	int err;
> +	unsigned int clen, thisgrp_len;
>  
>  	while (len > 0) {
>  		ext4_get_group_no_and_offset(sb, block, &group, &blkoff);
> @@ -4103,80 +4183,17 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block,
>  			ext4_error(sb, "Marking blocks in system zone - "
>  				   "Block = %llu, len = %u",
>  				   block, thisgrp_len);
> -			bitmap_bh = NULL;
>  			break;
>  		}
>  
> -		bitmap_bh = ext4_read_block_bitmap(sb, group);
> -		if (IS_ERR(bitmap_bh)) {
> -			err = PTR_ERR(bitmap_bh);
> -			bitmap_bh = NULL;
> -			break;
> -		}
> -
> -		err = -EIO;
> -		gdp = ext4_get_group_desc(sb, group, &gdp_bh);
> -		if (!gdp)
> -			break;
> -
> -		ext4_lock_group(sb, group);
> -		already = 0;
> -		for (i = 0; i < clen; i++)
> -			if (!mb_test_bit(blkoff + i, bitmap_bh->b_data) ==
> -					 !state)
> -				already++;
> -
> -		clen_changed = clen - already;
> -		if (state)
> -			mb_set_bits(bitmap_bh->b_data, blkoff, clen);
> -		else
> -			mb_clear_bits(bitmap_bh->b_data, blkoff, clen);
> -		if (ext4_has_group_desc_csum(sb) &&
> -		    (gdp->bg_flags & cpu_to_le16(EXT4_BG_BLOCK_UNINIT))) {
> -			gdp->bg_flags &= cpu_to_le16(~EXT4_BG_BLOCK_UNINIT);
> -			ext4_free_group_clusters_set(sb, gdp,
> -			     ext4_free_clusters_after_init(sb, group, gdp));
> -		}
> -		if (state)
> -			clen = ext4_free_group_clusters(sb, gdp) - clen_changed;
> -		else
> -			clen = ext4_free_group_clusters(sb, gdp) + clen_changed;
> -
> -		ext4_free_group_clusters_set(sb, gdp, clen);
> -		ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
> -		ext4_group_desc_csum_set(sb, group, gdp);
> -
> -		ext4_unlock_group(sb, group);
> -
> -		if (sbi->s_log_groups_per_flex) {
> -			ext4_group_t flex_group = ext4_flex_group(sbi, group);
> -			struct flex_groups *fg = sbi_array_rcu_deref(sbi,
> -						   s_flex_groups, flex_group);
> -
> -			if (state)
> -				atomic64_sub(clen_changed, &fg->free_clusters);
> -			else
> -				atomic64_add(clen_changed, &fg->free_clusters);
> -
> -		}
> -
> -		err = ext4_handle_dirty_metadata(NULL, NULL, bitmap_bh);
> -		if (err)
> -			break;
> -		sync_dirty_buffer(bitmap_bh);
> -		err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh);
> -		sync_dirty_buffer(gdp_bh);
> +		err = ext4_mb_mark_group_bb(&mc, group, blkoff, clen);
>  		if (err)
>  			break;
>  
>  		block += thisgrp_len;
>  		len -= thisgrp_len;
> -		brelse(bitmap_bh);
>  		BUG_ON(len < 0);
>  	}
> -
> -	if (err)
> -		brelse(bitmap_bh);
>  }
>  
>  /*
> -- 
> 2.30.0

Re: [PATCH v5 5/8] ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb

[Ritesh Harjani]

Kemeng Shi <shikemeng@huaweicloud.com> writes:

> call ext4_mb_mark_group_bb in ext4_mb_clear_bb to remove repeat code
> to update block bitmap and group descriptor on disk.
>
> Note: ext4_mb_clear_bb will update buddy and bitmap in two critical
> sections instead of update in the same critical section.
>
> Original lock behavior introduced in commit 7a2fcbf7f8573 ("ext4: don't
> use blocks freed but not yet committed in buddy cache init") to avoid
> race betwwen ext4_mb_free_blocks and ext4_mb_init_cache:
> ext4_mb_load_buddy_gfp
> ext4_lock_group
> mb_clear_bits(bitmap_bh, ...)
> mb_free_blocks/ext4_mb_free_metadata
> ext4_unlock_group
> ext4_mb_unload_buddy
>
> New lock behavior in this patch:
> ext4_mb_load_buddy_gfp
> ext4_lock_group
> mb_clear_bits(bitmap_bh, ...)
> ext4_unlock_group
>
> /* no ext4_mb_init_cache for the same group will be called as
> ext4_mb_load_buddy_gfp will ensure buddy page is update-to-date */
>
> ext4_lock_group
> mb_free_blocks/ext4_mb_free_metadata
> ext4_unlock_group
> ext4_mb_unload_buddy
>
> As buddy page for group is always update-to-date between
> ext4_mb_load_buddy_gfp and ext4_mb_unload_buddy. Then no
> ext4_mb_init_cache will be called for the same group concurrentlly when
> we update bitmap and buddy page betwwen buddy load and unload.

More information will definitely help.

In ext4_mb_init_cache(), within a lock_group(), we first initialize
a incore bitmap from on disk block bitmap, pa and from
ext4_mb_generate_from_freelist() (this function basically requires
ext4_mb_free_metadata() to be called)
Then we go and initialize incore buddy within a page which utilize bitmap
block data (from previous step) for generating buddy info.
So this clearly means we need incore bitmap and mb_free_metadata() to be updated
together within the same group lock.

Now you said that ext4_mb_init_cache() can't be called together between
ext4_mb_load_buddy_gfp() and unload_buddy() because buddy page is uptodate?? 
Can you give more details please? 

What about if the resize gets called on the last group which is within the
same page on which we are operating. Also consider blocksize < pagesize.
That means we can have even more blocks within the same page.
So ext4_mb_init_cache() can still get called right while between load_buddy and unload_buddy?

Maybe I need to take a closer look at it.

-ritesh


>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc.c | 90 ++++++++++++-----------------------------------
>  1 file changed, 23 insertions(+), 67 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 34fd12aeaf8d..57cc304b724e 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6325,19 +6325,21 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  			       ext4_fsblk_t block, unsigned long count,
>  			       int flags)
>  {
> -	struct buffer_head *bitmap_bh = NULL;
> +	struct ext4_mark_context mc = {
> +		.handle = handle,
> +		.sb = inode->i_sb,
> +		.state = 0,
> +	};
>  	struct super_block *sb = inode->i_sb;
> -	struct ext4_group_desc *gdp;
>  	struct ext4_group_info *grp;
>  	unsigned int overflow;
>  	ext4_grpblk_t bit;
> -	struct buffer_head *gd_bh;
>  	ext4_group_t block_group;
>  	struct ext4_sb_info *sbi;
>  	struct ext4_buddy e4b;
>  	unsigned int count_clusters;
>  	int err = 0;
> -	int ret;
> +	int mark_flags = 0;
>  
>  	sbi = EXT4_SB(sb);
>  
> @@ -6369,18 +6371,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  		/* The range changed so it's no longer validated */
>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>  	}
> -	count_clusters = EXT4_NUM_B2C(sbi, count);
> -	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
> -	if (IS_ERR(bitmap_bh)) {
> -		err = PTR_ERR(bitmap_bh);
> -		bitmap_bh = NULL;
> -		goto error_return;
> -	}
> -	gdp = ext4_get_group_desc(sb, block_group, &gd_bh);
> -	if (!gdp) {
> -		err = -EIO;
> -		goto error_return;
> -	}
>  
>  	if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
>  	    !ext4_inode_block_valid(inode, block, count)) {
> @@ -6390,28 +6380,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  		goto error_return;
>  	}
>  
> -	BUFFER_TRACE(bitmap_bh, "getting write access");
> -	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
> -					    EXT4_JTR_NONE);
> -	if (err)
> -		goto error_return;
> -
> -	/*
> -	 * We are about to modify some metadata.  Call the journal APIs
> -	 * to unshare ->b_data if a currently-committing transaction is
> -	 * using it
> -	 */
> -	BUFFER_TRACE(gd_bh, "get_write_access");
> -	err = ext4_journal_get_write_access(handle, sb, gd_bh, EXT4_JTR_NONE);
> -	if (err)
> -		goto error_return;
> -#ifdef AGGRESSIVE_CHECK
> -	{
> -		int i;
> -		for (i = 0; i < count_clusters; i++)
> -			BUG_ON(!mb_test_bit(bit + i, bitmap_bh->b_data));
> -	}
> -#endif
> +	count_clusters = EXT4_NUM_B2C(sbi, count);
>  	trace_ext4_mballoc_free(sb, inode, block_group, bit, count_clusters);
>  
>  	/* __GFP_NOFAIL: retry infinitely, ignore TIF_MEMDIE and memcg limit. */
> @@ -6420,6 +6389,22 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  	if (err)
>  		goto error_return;
>  
> +#ifdef AGGRESSIVE_CHECK
> +	mark_flags |= EXT4_MB_BITMAP_MARKED_CHECK;
> +#endif
> +	err = ext4_mb_mark_group_bb(&mc, block_group, bit, count_clusters,
> +				    mark_flags);
> +
> +
> +	if (err && mc.changed == 0) {
> +		ext4_mb_unload_buddy(&e4b);
> +		goto error_return;
> +	}
> +
> +#ifdef AGGRESSIVE_CHECK
> +	BUG_ON(mc.changed != count_clusters);
> +#endif
> +
>  	/*
>  	 * We need to make sure we don't reuse the freed block until after the
>  	 * transaction is committed. We make an exception if the inode is to be
> @@ -6442,13 +6427,8 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  		new_entry->efd_tid = handle->h_transaction->t_tid;
>  
>  		ext4_lock_group(sb, block_group);
> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>  		ext4_mb_free_metadata(handle, &e4b, new_entry);
>  	} else {
> -		/* need to update group_info->bb_free and bitmap
> -		 * with group lock held. generate_buddy look at
> -		 * them with group lock_held
> -		 */
>  		if (test_opt(sb, DISCARD)) {
>  			err = ext4_issue_discard(sb, block_group, bit,
>  						 count_clusters, NULL);
> @@ -6461,23 +6441,11 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  			EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info);
>  
>  		ext4_lock_group(sb, block_group);
> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>  		mb_free_blocks(inode, &e4b, bit, count_clusters);
>  	}
>  
> -	ret = ext4_free_group_clusters(sb, gdp) + count_clusters;
> -	ext4_free_group_clusters_set(sb, gdp, ret);
> -	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
> -	ext4_group_desc_csum_set(sb, block_group, gdp);
>  	ext4_unlock_group(sb, block_group);
>  
> -	if (sbi->s_log_groups_per_flex) {
> -		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
> -		atomic64_add(count_clusters,
> -			     &sbi_array_rcu_deref(sbi, s_flex_groups,
> -						  flex_group)->free_clusters);
> -	}
> -
>  	/*
>  	 * on a bigalloc file system, defer the s_freeclusters_counter
>  	 * update to the caller (ext4_remove_space and friends) so they
> @@ -6492,26 +6460,14 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>  
>  	ext4_mb_unload_buddy(&e4b);
>  
> -	/* We dirtied the bitmap block */
> -	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
> -	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
> -
> -	/* And the group descriptor block */
> -	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
> -	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
> -	if (!err)
> -		err = ret;
> -
>  	if (overflow && !err) {
>  		block += count;
>  		count = overflow;
> -		put_bh(bitmap_bh);
>  		/* The range changed so it's no longer validated */
>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>  		goto do_more;
>  	}
>  error_return:
> -	brelse(bitmap_bh);
>  	ext4_std_error(sb, err);
>  	return;
>  }
> -- 
> 2.30.0

Re: [PATCH v5 5/8] ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Kemeng Shi <shikemeng@huaweicloud.com> writes:
>
>> call ext4_mb_mark_group_bb in ext4_mb_clear_bb to remove repeat code
>> to update block bitmap and group descriptor on disk.
>>
>> Note: ext4_mb_clear_bb will update buddy and bitmap in two critical
>> sections instead of update in the same critical section.
>>
>> Original lock behavior introduced in commit 7a2fcbf7f8573 ("ext4: don't
>> use blocks freed but not yet committed in buddy cache init") to avoid
>> race betwwen ext4_mb_free_blocks and ext4_mb_init_cache:
>> ext4_mb_load_buddy_gfp
>> ext4_lock_group
>> mb_clear_bits(bitmap_bh, ...)
>> mb_free_blocks/ext4_mb_free_metadata
>> ext4_unlock_group
>> ext4_mb_unload_buddy
>>
>> New lock behavior in this patch:
>> ext4_mb_load_buddy_gfp
>> ext4_lock_group
>> mb_clear_bits(bitmap_bh, ...)
>> ext4_unlock_group
>>
>> /* no ext4_mb_init_cache for the same group will be called as
>> ext4_mb_load_buddy_gfp will ensure buddy page is update-to-date */
>>
>> ext4_lock_group
>> mb_free_blocks/ext4_mb_free_metadata
>> ext4_unlock_group
>> ext4_mb_unload_buddy
>>
>> As buddy page for group is always update-to-date between
>> ext4_mb_load_buddy_gfp and ext4_mb_unload_buddy. Then no
>> ext4_mb_init_cache will be called for the same group concurrentlly when
>> we update bitmap and buddy page betwwen buddy load and unload.
>
> More information will definitely help.
>
> In ext4_mb_init_cache(), within a lock_group(), we first initialize
> a incore bitmap from on disk block bitmap, pa and from
> ext4_mb_generate_from_freelist() (this function basically requires
> ext4_mb_free_metadata() to be called)
> Then we go and initialize incore buddy within a page which utilize bitmap
> block data (from previous step) for generating buddy info.
> So this clearly means we need incore bitmap and mb_free_metadata() to be updated
> together within the same group lock.

Here I meant on-disk bitmap in bitmap_bh. Anyhow I don't think this is really
required though.


>
> Now you said that ext4_mb_init_cache() can't be called together between
> ext4_mb_load_buddy_gfp() and unload_buddy() because buddy page is uptodate?? 
> Can you give more details please? 

Ok. So even if the page is uptodate, ext4_mb_init_cache() can still get
called when you have a resize and the extended group belongs to the same
page (for bs < ps). But we don't touch the bitmap and buddy info for the
groups which are already initialized.
And as you said we can't be working on bitmap or buddy info unless we
have called ext4_mb_load_buddy_gfp() which takes care of the bitmap and
buddy initialization for this group and it will clear the
EXT4_GROUP_INFO_NEED_INIT_BIT in grp->bb_state so that
ext4_mb_init_cache() called due to resize doesn't re-initialize it.


But one concern that I still have is - till now we update the bitmap and
buddy info atomically within the same group lock. This patch is changing
this behavior. So you might wanna explain that this race will never happen
and why?


ext4_mb_clear_bb()              xxxxxxxx()

    ext4_mb_load_buddy_gfp()    ext4_mb_load_buddy_gfp() // this can happen in parallel for initialized groups
    ext4_lock_group()           ext4_lock_group()   // will wait
    update block bitmap
    ext4_unlock_group()
                                ext4_lock_group() // succeed. 
                                looks into bitmap and buddy info and found discripancy.
                                mark group corrupt or something?
                                ext4_unlock_group()    

    ext4_lock_group()
    update buddy info
    ext4_unlock_group()
    ext4_mb_unlock_buddy()      ext4_mb_unlock_buddy()


...On more reading, I don't find a dependency between the two. 
I see mb_free_blocks (poor naming I know...) which is responsible for
freeing buddy info does not have any return value. So it won't return an
error ever. Other thing to note is, ext4_mb_release_inode_pa() does
check for bits set in bitmap and based on that call mb_free_blocks(),
but I think we don't have a problem there since we take a group lock and
we first update the bitmap. 

So I haven't found any dependency due to which we need to update bitmap
and buddy info atomically. Hence IMO, we can separate it out from a common
lock.
Feel free to add/update more details if you thnk anything is missed.
I didn't put why journal commit cannot run between the two, because I
think we are still in the middle of a txn.
(i.e. we still haven't call ext4_journal_stop())

I am putting above information here.. so that if someone else reviews
the code, then can find this discussion for reference.

However please note that the subject of the commit is not very
informative. I think this patch should have been split into two - 

1. ext4: Separate block bitmap and buddy bitmap freeing in ext4_mb_clear_bb() 
... In this commit message you should explain why this can be seperated.
This way a reviewer would know that this requires a closer review to
make sure that locking is handled correctly and/or there is no race.

2. ext4: Make use of ext4_mb_mark_group_bb() in ext4_mb_clear_bb()
This commit message then just becomes make use of the new function that
you created.

-ritesh

>
> What about if the resize gets called on the last group which is within the
> same page on which we are operating. Also consider blocksize < pagesize.
> That means we can have even more blocks within the same page.
> So ext4_mb_init_cache() can still get called right while between load_buddy and unload_buddy?
>
> Maybe I need to take a closer look at it.
>
> -ritesh
>
>
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/mballoc.c | 90 ++++++++++++-----------------------------------
>>  1 file changed, 23 insertions(+), 67 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 34fd12aeaf8d..57cc304b724e 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -6325,19 +6325,21 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  			       ext4_fsblk_t block, unsigned long count,
>>  			       int flags)
>>  {
>> -	struct buffer_head *bitmap_bh = NULL;
>> +	struct ext4_mark_context mc = {
>> +		.handle = handle,
>> +		.sb = inode->i_sb,
>> +		.state = 0,
>> +	};
>>  	struct super_block *sb = inode->i_sb;
>> -	struct ext4_group_desc *gdp;
>>  	struct ext4_group_info *grp;
>>  	unsigned int overflow;
>>  	ext4_grpblk_t bit;
>> -	struct buffer_head *gd_bh;
>>  	ext4_group_t block_group;
>>  	struct ext4_sb_info *sbi;
>>  	struct ext4_buddy e4b;
>>  	unsigned int count_clusters;
>>  	int err = 0;
>> -	int ret;
>> +	int mark_flags = 0;
>>  
>>  	sbi = EXT4_SB(sb);
>>  
>> @@ -6369,18 +6371,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  		/* The range changed so it's no longer validated */
>>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>>  	}
>> -	count_clusters = EXT4_NUM_B2C(sbi, count);
>> -	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
>> -	if (IS_ERR(bitmap_bh)) {
>> -		err = PTR_ERR(bitmap_bh);
>> -		bitmap_bh = NULL;
>> -		goto error_return;
>> -	}
>> -	gdp = ext4_get_group_desc(sb, block_group, &gd_bh);
>> -	if (!gdp) {
>> -		err = -EIO;
>> -		goto error_return;
>> -	}
>>  
>>  	if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
>>  	    !ext4_inode_block_valid(inode, block, count)) {
>> @@ -6390,28 +6380,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  		goto error_return;
>>  	}
>>  
>> -	BUFFER_TRACE(bitmap_bh, "getting write access");
>> -	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
>> -					    EXT4_JTR_NONE);
>> -	if (err)
>> -		goto error_return;
>> -
>> -	/*
>> -	 * We are about to modify some metadata.  Call the journal APIs
>> -	 * to unshare ->b_data if a currently-committing transaction is
>> -	 * using it
>> -	 */
>> -	BUFFER_TRACE(gd_bh, "get_write_access");
>> -	err = ext4_journal_get_write_access(handle, sb, gd_bh, EXT4_JTR_NONE);
>> -	if (err)
>> -		goto error_return;
>> -#ifdef AGGRESSIVE_CHECK
>> -	{
>> -		int i;
>> -		for (i = 0; i < count_clusters; i++)
>> -			BUG_ON(!mb_test_bit(bit + i, bitmap_bh->b_data));
>> -	}
>> -#endif
>> +	count_clusters = EXT4_NUM_B2C(sbi, count);
>>  	trace_ext4_mballoc_free(sb, inode, block_group, bit, count_clusters);
>>  
>>  	/* __GFP_NOFAIL: retry infinitely, ignore TIF_MEMDIE and memcg limit. */
>> @@ -6420,6 +6389,22 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  	if (err)
>>  		goto error_return;
>>  
>> +#ifdef AGGRESSIVE_CHECK
>> +	mark_flags |= EXT4_MB_BITMAP_MARKED_CHECK;
>> +#endif
>> +	err = ext4_mb_mark_group_bb(&mc, block_group, bit, count_clusters,
>> +				    mark_flags);
>> +
>> +
>> +	if (err && mc.changed == 0) {
>> +		ext4_mb_unload_buddy(&e4b);
>> +		goto error_return;
>> +	}
>> +
>> +#ifdef AGGRESSIVE_CHECK
>> +	BUG_ON(mc.changed != count_clusters);
>> +#endif
>> +
>>  	/*
>>  	 * We need to make sure we don't reuse the freed block until after the
>>  	 * transaction is committed. We make an exception if the inode is to be
>> @@ -6442,13 +6427,8 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  		new_entry->efd_tid = handle->h_transaction->t_tid;
>>  
>>  		ext4_lock_group(sb, block_group);
>> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>>  		ext4_mb_free_metadata(handle, &e4b, new_entry);
>>  	} else {
>> -		/* need to update group_info->bb_free and bitmap
>> -		 * with group lock held. generate_buddy look at
>> -		 * them with group lock_held
>> -		 */
>>  		if (test_opt(sb, DISCARD)) {
>>  			err = ext4_issue_discard(sb, block_group, bit,
>>  						 count_clusters, NULL);
>> @@ -6461,23 +6441,11 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  			EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info);
>>  
>>  		ext4_lock_group(sb, block_group);
>> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>>  		mb_free_blocks(inode, &e4b, bit, count_clusters);
>>  	}
>>  
>> -	ret = ext4_free_group_clusters(sb, gdp) + count_clusters;
>> -	ext4_free_group_clusters_set(sb, gdp, ret);
>> -	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
>> -	ext4_group_desc_csum_set(sb, block_group, gdp);
>>  	ext4_unlock_group(sb, block_group);
>>  
>> -	if (sbi->s_log_groups_per_flex) {
>> -		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
>> -		atomic64_add(count_clusters,
>> -			     &sbi_array_rcu_deref(sbi, s_flex_groups,
>> -						  flex_group)->free_clusters);
>> -	}
>> -
>>  	/*
>>  	 * on a bigalloc file system, defer the s_freeclusters_counter
>>  	 * update to the caller (ext4_remove_space and friends) so they
>> @@ -6492,26 +6460,14 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>  
>>  	ext4_mb_unload_buddy(&e4b);
>>  
>> -	/* We dirtied the bitmap block */
>> -	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
>> -	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
>> -
>> -	/* And the group descriptor block */
>> -	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
>> -	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
>> -	if (!err)
>> -		err = ret;
>> -
>>  	if (overflow && !err) {
>>  		block += count;
>>  		count = overflow;
>> -		put_bh(bitmap_bh);
>>  		/* The range changed so it's no longer validated */
>>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>>  		goto do_more;
>>  	}
>>  error_return:
>> -	brelse(bitmap_bh);
>>  	ext4_std_error(sb, err);
>>  	return;
>>  }
>> -- 
>> 2.30.0

Re: [PATCH v5 7/8] ext4: add some kunit stub for mballoc kunit test

[Jason Yan]

On 2023/6/29 22:40, Kemeng Shi wrote:
> Multiblocks allocation will read and write block bitmap and group
> descriptor which reside on disk. Add kunit stub to function
> ext4_get_group_desc, ext4_read_block_bitmap_nowait, ext4_wait_block_bitmap
> and ext4_mb_mark_group_bb to avoid real IO to disk.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   fs/ext4/balloc.c  | 16 ++++++++++++++++
>   fs/ext4/mballoc.c |  6 ++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 1f72f977c6db..d39655fb2f53 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -22,6 +22,7 @@
>   #include "mballoc.h"
>   
>   #include <trace/events/ext4.h>
> +#include <kunit/static_stub.h>
>   
>   static unsigned ext4_num_base_meta_clusters(struct super_block *sb,
>   					    ext4_group_t block_group);
> @@ -274,6 +275,11 @@ struct ext4_group_desc * ext4_get_group_desc(struct super_block *sb,
>   	struct ext4_sb_info *sbi = EXT4_SB(sb);
>   	struct buffer_head *bh_p;
>   
> +#ifdef CONFIG_EXT4_KUNIT_TESTS
> +	KUNIT_STATIC_STUB_REDIRECT(ext4_get_group_desc,
> +				   sb, block_group, bh);
> +#endif

Hi Kemeng,

I'm not a fan of adding "#ifdef" blocks in functions everywhere. The 
right thing to do is to put these "#ifdef" blocks in header files and to 
make the macro or functions empty if you want to make these codes 
conditionally-compiled out.

The standard usage in kernel looks like:

In the header file: "a.h"

#ifdef CONFIG_XXXX
void foo(struct sas_task *task);
#else
static inline void foo(struct sas_task *task)
{
}
#endif

or

#ifdef CONFIG_XXXX
#define foo()  do_something()
#else
#define foo()
#endif

And in the c file: "a.c"

#include "a.h"

void bar(void)
{
	foo();
	something_else();
}

Please refer to:
https://docs.kernel.org/process/4.Coding.html?#ifdef-and-preprocessor-use-in-general

Thanks,
Jason

Re: [PATCH v5 1/8] ext4: factor out codes to update block bitmap and group descriptor on disk from ext4_mb_mark_bb

[Kemeng Shi]

on 7/22/2023 2:24 PM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@huaweicloud.com> writes:
> 
>> There are several reasons to add a general function to update block
>> bitmap and group descriptor on disk:
>> 1. pair behavior of alloc/free bits. For example,
>> ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups
>> in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this.
>> 2. remove repeat code to read from disk, update and write back to disk.
>> 3. reduce future unit test mocks to catch real IO to update structure
>> on disk.
> 
> Thanks for the cleanup and sorry that I am starting to review this
> series only now. However I do have some review comments to understand a
> bit more on the patch series. 
> 
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
>> ---
>>  fs/ext4/mballoc.c | 157 +++++++++++++++++++++++++---------------------
>>  1 file changed, 87 insertions(+), 70 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index a2475b8c9fb5..58864a9116c0 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -3948,6 +3948,86 @@ void ext4_exit_mballoc(void)
>>  	ext4_groupinfo_destroy_slabs();
>>  }
>>  
>> +struct ext4_mark_context {
>> +	struct super_block *sb;
>> +	int state;
>> +};
> 
> It's not totally clear the intention behind this structure from above
> since it lacking any comments.
> 
> Can you please help me understand why do we need this.
> I still don't know whether we require this structure and what is it's
> purpose. Is it only for reducing the number of variable passing?
Exactly. It's only for reducing the number of variable passing.
> Let me do more reading... 
> 
> ...On more reading, I was previous considering to rename it to something
> like ext4_mb_mark_context, but then I realized the naming of this is
> something similar to ext4_allocation_context. So we may keep the naming
> as is.
Exactly again. The ext4_mark_context is based on ext4_allocation_context.
> So since this structure, presumably, is used for marking blk bits for
> mballoc. Why don't we pass useful information which is relevant for
> this operation like - 
> 
>     ext4_mark_context {
>         ext4_group_t mc_group;          /* block group */
>         ext4_grpblk_t mc_clblk;	    /* block in cluster units */
>         ext4_grpblk_t mc_cllen;	    /* len in cluster units */
>         ext4_grpblk_t mc_clupdates;     /* number of clusters marked/unmarked */
>         unsigned int mc_flags;          /* flags ... */
>         bool mc_state;                  /* to set or unset state */
>     };
> 
> Maybe, super_block and handle we can pass as an argument as those doesn't
> define the ext4_mark_context for mballoc.
Actually, I try to put stable arguments need by bit mark into
ext4_mark_context then ext4_mark_context could be initialized once and used
multiple times. For example, if there is function to mark multiple bit
fragments, it will use ext4_allocation_context as:
 struct ext4_mark_context mc = {
   /* initialization */
 }
 /* mark fragment1 */
 ext4_mb_mark_group_bb(&mc, group1, blkoff1, len1);
 /* mark fragment2 */
 ext4_mb_mark_group_bb(&mc, group2, blkoff2, len2);
And I thinks these stable arguments match "context" meaning which bit
mark needed to work around :).

Put bit mark relevant information into ext4_mark_context is absolutely
a great choice. I will arrange ext4_mark_context in this way if you
still prefer this.

> Since this structure is prepared not at the begining of any function, we
> may need a prepare function for it. e.g. 
> 
>    static void ext4_mb_prepare_mark_context(&mc, ...)
>    static int ext4_mb_mark_context(sb, handle, &mc);  (instead of ext4_mb_mark_group_bb())
> 
> Does this sounds better to you? Thoughts?
>
Yes, prepare function is a great idea. I will add this in next version.
> Otherwise I think having a common function for mb_mark_context looks
> like a nice cleanup.
> 
Thanks! this means a lot to me!


-- 
Best wishes
Kemeng Shi

Re: [PATCH v5 5/8] ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb

[Kemeng Shi]

on 7/23/2023 1:37 PM, Ritesh Harjani wrote:
> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
> 
>> Kemeng Shi <shikemeng@huaweicloud.com> writes:
>>
>>> call ext4_mb_mark_group_bb in ext4_mb_clear_bb to remove repeat code
>>> to update block bitmap and group descriptor on disk.
>>>
>>> Note: ext4_mb_clear_bb will update buddy and bitmap in two critical
>>> sections instead of update in the same critical section.
>>>
>>> Original lock behavior introduced in commit 7a2fcbf7f8573 ("ext4: don't
>>> use blocks freed but not yet committed in buddy cache init") to avoid
>>> race betwwen ext4_mb_free_blocks and ext4_mb_init_cache:
>>> ext4_mb_load_buddy_gfp
>>> ext4_lock_group
>>> mb_clear_bits(bitmap_bh, ...)
>>> mb_free_blocks/ext4_mb_free_metadata
>>> ext4_unlock_group
>>> ext4_mb_unload_buddy
>>>
>>> New lock behavior in this patch:
>>> ext4_mb_load_buddy_gfp
>>> ext4_lock_group
>>> mb_clear_bits(bitmap_bh, ...)
>>> ext4_unlock_group
>>>
>>> /* no ext4_mb_init_cache for the same group will be called as
>>> ext4_mb_load_buddy_gfp will ensure buddy page is update-to-date */
>>>
>>> ext4_lock_group
>>> mb_free_blocks/ext4_mb_free_metadata
>>> ext4_unlock_group
>>> ext4_mb_unload_buddy
>>>
>>> As buddy page for group is always update-to-date between
>>> ext4_mb_load_buddy_gfp and ext4_mb_unload_buddy. Then no
>>> ext4_mb_init_cache will be called for the same group concurrentlly when
>>> we update bitmap and buddy page betwwen buddy load and unload.
>>
>> More information will definitely help.
>>
>> In ext4_mb_init_cache(), within a lock_group(), we first initialize
>> a incore bitmap from on disk block bitmap, pa and from
>> ext4_mb_generate_from_freelist() (this function basically requires
>> ext4_mb_free_metadata() to be called)
>> Then we go and initialize incore buddy within a page which utilize bitmap
>> block data (from previous step) for generating buddy info.
>> So this clearly means we need incore bitmap and mb_free_metadata() to be updated
>> together within the same group lock.
> 
> Here I meant on-disk bitmap in bitmap_bh. Anyhow I don't think this is really
> required though.
> 
> 
>>
>> Now you said that ext4_mb_init_cache() can't be called together between
>> ext4_mb_load_buddy_gfp() and unload_buddy() because buddy page is uptodate?? 
>> Can you give more details please? 
> 
> Ok. So even if the page is uptodate, ext4_mb_init_cache() can still get
> called when you have a resize and the extended group belongs to the same
> page (for bs < ps). But we don't touch the bitmap and buddy info for the
> groups which are already initialized.
> And as you said we can't be working on bitmap or buddy info unless we
> have called ext4_mb_load_buddy_gfp() which takes care of the bitmap and
> buddy initialization for this group and it will clear the
> EXT4_GROUP_INFO_NEED_INIT_BIT in grp->bb_state so that
> ext4_mb_init_cache() called due to resize doesn't re-initialize it.
> 
Yes, my point is that only first ext4_mb_load_buddy_gfp of a group will
do real buddy initialization which will generate buddy from on-disk bitmap
and data from ext4_mb_free_metadata. Any code after ext4_mb_load_buddy_gfp
has no race with buddy initialization. As ext4_mb_free_metadata is called
after ext4_mb_load_buddy_gfp, there is no race with buddy initialization.

> But one concern that I still have is - till now we update the bitmap and
> buddy info atomically within the same group lock. This patch is changing
> this behavior. So you might wanna explain that this race will never happen
> and why?
> 
> 
> ext4_mb_clear_bb()              xxxxxxxx()
> 
>     ext4_mb_load_buddy_gfp()    ext4_mb_load_buddy_gfp() // this can happen in parallel for initialized groups
>     ext4_lock_group()           ext4_lock_group()   // will wait
>     update block bitmap
>     ext4_unlock_group()
>                                 ext4_lock_group() // succeed. 
>                                 looks into bitmap and buddy info and found discripancy.
>                                 mark group corrupt or something?
>                                 ext4_unlock_group()    
> 
>     ext4_lock_group()
>     update buddy info
>     ext4_unlock_group()
>     ext4_mb_unlock_buddy()      ext4_mb_unlock_buddy()
> 
> 
> ...On more reading, I don't find a dependency between the two. 
> I see mb_free_blocks (poor naming I know...) which is responsible for
> freeing buddy info does not have any return value. So it won't return an
> error ever. Other thing to note is, ext4_mb_release_inode_pa() does
> check for bits set in bitmap and based on that call mb_free_blocks(),
> but I think we don't have a problem there since we take a group lock and
> we first update the bitmap. 
> 
> So I haven't found any dependency due to which we need to update bitmap
> and buddy info atomically. Hence IMO, we can separate it out from a common
> lock> Feel free to add/update more details if you thnk anything is missed.
After this patchset, here is all paths to cooperate update of on-disk bitmap
and buddy bitmap in seperate lock (search all functions calling
ext4_mb_load_buddy or ext4_mb_load_buddy_gfp to find potential code path to
update both on-disk bitmap and buddy bitmap):
code to free blocks:
ext4_group_add_blocks (lock is separated in this patchset)
  lock
  clear on-disk bitmap
  unlock

  lock
  clear buddy bitmap
  unlock

ext4_mb_clear_bb (lock is separated in this patchset)
  lock
  clear on-disk bitmap
  unlock

  lock
  clear buddy bitmap
  unlock

code to alloc blocks:
ext4_mb_new_blocks (lock was separated before)
  lock
  search and set buddy bitmap
  unlock

  ext4_mb_mark_diskspace_used
    lock
    set on-disk bitmap
    unlock

We always clear on-disk bitmap first, and then clear buddy bitmap during
free while we do allocation on reverse order, i.e. seach and set buddy
bitmap first, and then set on-disk bitmap.
With this order, we know that bits are cleared in both on-dism bitmap
and buddy bitmap when it's seen from allocation.

> I didn't put why journal commit cannot run between the two, because I
> think we are still in the middle of a txn.
> (i.e. we still haven't call ext4_journal_stop())
> 
Yes, this is also explained in [1].

> I am putting above information here.. so that if someone else reviews
> the code, then can find this discussion for reference.
> 
> However please note that the subject of the commit is not very
> informative. I think this patch should have been split into two - 
> 
> 1. ext4: Separate block bitmap and buddy bitmap freeing in ext4_mb_clear_bb() 
> ... In this commit message you should explain why this can be seperated.
> This way a reviewer would know that this requires a closer review to
> make sure that locking is handled correctly and/or there is no race.
> 
> 2. ext4: Make use of ext4_mb_mark_group_bb() in ext4_mb_clear_bb()
> This commit message then just becomes make use of the new function that
> you created.
> 
Sure, I will split this patch in next version and add details discussed to
commit.

[1] https://lore.kernel.org/linux-ext4/bb19c6f8-d31f-f686-17f9-3fd2bb1db3dd@huaweicloud.com/

-- 
Best wishes
Kemeng Shi

> -ritesh
> >>
>> What about if the resize gets called on the last group which is within the
>> same page on which we are operating. Also consider blocksize < pagesize.
>> That means we can have even more blocks within the same page.
>> So ext4_mb_init_cache() can still get called right while between load_buddy and unload_buddy?
>>
>> Maybe I need to take a closer look at it.
>>
>> -ritesh
>>
>>
>>>
>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ---
>>>  fs/ext4/mballoc.c | 90 ++++++++++++-----------------------------------
>>>  1 file changed, 23 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>>> index 34fd12aeaf8d..57cc304b724e 100644
>>> --- a/fs/ext4/mballoc.c
>>> +++ b/fs/ext4/mballoc.c
>>> @@ -6325,19 +6325,21 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  			       ext4_fsblk_t block, unsigned long count,
>>>  			       int flags)
>>>  {
>>> -	struct buffer_head *bitmap_bh = NULL;
>>> +	struct ext4_mark_context mc = {
>>> +		.handle = handle,
>>> +		.sb = inode->i_sb,
>>> +		.state = 0,
>>> +	};
>>>  	struct super_block *sb = inode->i_sb;
>>> -	struct ext4_group_desc *gdp;
>>>  	struct ext4_group_info *grp;
>>>  	unsigned int overflow;
>>>  	ext4_grpblk_t bit;
>>> -	struct buffer_head *gd_bh;
>>>  	ext4_group_t block_group;
>>>  	struct ext4_sb_info *sbi;
>>>  	struct ext4_buddy e4b;
>>>  	unsigned int count_clusters;
>>>  	int err = 0;
>>> -	int ret;
>>> +	int mark_flags = 0;
>>>  
>>>  	sbi = EXT4_SB(sb);
>>>  
>>> @@ -6369,18 +6371,6 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  		/* The range changed so it's no longer validated */
>>>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>>>  	}
>>> -	count_clusters = EXT4_NUM_B2C(sbi, count);
>>> -	bitmap_bh = ext4_read_block_bitmap(sb, block_group);
>>> -	if (IS_ERR(bitmap_bh)) {
>>> -		err = PTR_ERR(bitmap_bh);
>>> -		bitmap_bh = NULL;
>>> -		goto error_return;
>>> -	}
>>> -	gdp = ext4_get_group_desc(sb, block_group, &gd_bh);
>>> -	if (!gdp) {
>>> -		err = -EIO;
>>> -		goto error_return;
>>> -	}
>>>  
>>>  	if (!(flags & EXT4_FREE_BLOCKS_VALIDATED) &&
>>>  	    !ext4_inode_block_valid(inode, block, count)) {
>>> @@ -6390,28 +6380,7 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  		goto error_return;
>>>  	}
>>>  
>>> -	BUFFER_TRACE(bitmap_bh, "getting write access");
>>> -	err = ext4_journal_get_write_access(handle, sb, bitmap_bh,
>>> -					    EXT4_JTR_NONE);
>>> -	if (err)
>>> -		goto error_return;
>>> -
>>> -	/*
>>> -	 * We are about to modify some metadata.  Call the journal APIs
>>> -	 * to unshare ->b_data if a currently-committing transaction is
>>> -	 * using it
>>> -	 */
>>> -	BUFFER_TRACE(gd_bh, "get_write_access");
>>> -	err = ext4_journal_get_write_access(handle, sb, gd_bh, EXT4_JTR_NONE);
>>> -	if (err)
>>> -		goto error_return;
>>> -#ifdef AGGRESSIVE_CHECK
>>> -	{
>>> -		int i;
>>> -		for (i = 0; i < count_clusters; i++)
>>> -			BUG_ON(!mb_test_bit(bit + i, bitmap_bh->b_data));
>>> -	}
>>> -#endif
>>> +	count_clusters = EXT4_NUM_B2C(sbi, count);
>>>  	trace_ext4_mballoc_free(sb, inode, block_group, bit, count_clusters);
>>>  
>>>  	/* __GFP_NOFAIL: retry infinitely, ignore TIF_MEMDIE and memcg limit. */
>>> @@ -6420,6 +6389,22 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  	if (err)
>>>  		goto error_return;
>>>  
>>> +#ifdef AGGRESSIVE_CHECK
>>> +	mark_flags |= EXT4_MB_BITMAP_MARKED_CHECK;
>>> +#endif
>>> +	err = ext4_mb_mark_group_bb(&mc, block_group, bit, count_clusters,
>>> +				    mark_flags);
>>> +
>>> +
>>> +	if (err && mc.changed == 0) {
>>> +		ext4_mb_unload_buddy(&e4b);
>>> +		goto error_return;
>>> +	}
>>> +
>>> +#ifdef AGGRESSIVE_CHECK
>>> +	BUG_ON(mc.changed != count_clusters);
>>> +#endif
>>> +
>>>  	/*
>>>  	 * We need to make sure we don't reuse the freed block until after the
>>>  	 * transaction is committed. We make an exception if the inode is to be
>>> @@ -6442,13 +6427,8 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  		new_entry->efd_tid = handle->h_transaction->t_tid;
>>>  
>>>  		ext4_lock_group(sb, block_group);
>>> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>>>  		ext4_mb_free_metadata(handle, &e4b, new_entry);
>>>  	} else {
>>> -		/* need to update group_info->bb_free and bitmap
>>> -		 * with group lock held. generate_buddy look at
>>> -		 * them with group lock_held
>>> -		 */
>>>  		if (test_opt(sb, DISCARD)) {
>>>  			err = ext4_issue_discard(sb, block_group, bit,
>>>  						 count_clusters, NULL);
>>> @@ -6461,23 +6441,11 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  			EXT4_MB_GRP_CLEAR_TRIMMED(e4b.bd_info);
>>>  
>>>  		ext4_lock_group(sb, block_group);
>>> -		mb_clear_bits(bitmap_bh->b_data, bit, count_clusters);
>>>  		mb_free_blocks(inode, &e4b, bit, count_clusters);
>>>  	}
>>>  
>>> -	ret = ext4_free_group_clusters(sb, gdp) + count_clusters;
>>> -	ext4_free_group_clusters_set(sb, gdp, ret);
>>> -	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
>>> -	ext4_group_desc_csum_set(sb, block_group, gdp);
>>>  	ext4_unlock_group(sb, block_group);
>>>  
>>> -	if (sbi->s_log_groups_per_flex) {
>>> -		ext4_group_t flex_group = ext4_flex_group(sbi, block_group);
>>> -		atomic64_add(count_clusters,
>>> -			     &sbi_array_rcu_deref(sbi, s_flex_groups,
>>> -						  flex_group)->free_clusters);
>>> -	}
>>> -
>>>  	/*
>>>  	 * on a bigalloc file system, defer the s_freeclusters_counter
>>>  	 * update to the caller (ext4_remove_space and friends) so they
>>> @@ -6492,26 +6460,14 @@ static void ext4_mb_clear_bb(handle_t *handle, struct inode *inode,
>>>  
>>>  	ext4_mb_unload_buddy(&e4b);
>>>  
>>> -	/* We dirtied the bitmap block */
>>> -	BUFFER_TRACE(bitmap_bh, "dirtied bitmap block");
>>> -	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
>>> -
>>> -	/* And the group descriptor block */
>>> -	BUFFER_TRACE(gd_bh, "dirtied group descriptor block");
>>> -	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
>>> -	if (!err)
>>> -		err = ret;
>>> -
>>>  	if (overflow && !err) {
>>>  		block += count;
>>>  		count = overflow;
>>> -		put_bh(bitmap_bh);
>>>  		/* The range changed so it's no longer validated */
>>>  		flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
>>>  		goto do_more;
>>>  	}
>>>  error_return:
>>> -	brelse(bitmap_bh);
>>>  	ext4_std_error(sb, err);
>>>  	return;
>>>  }
>>> -- 
>>> 2.30.0
> 

Re: [PATCH v5 8/8] ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

[Ritesh Harjani]

Kemeng Shi <shikemeng@huaweicloud.com> writes:

> Here are prepared work:
> 1. Include mballoc-test.c to mballoc.c to be able test static function
> in mballoc.c.
> 2. Implement static stub to avoid read IO to disk.
> 3. Construct fake super_block. Only partial members are set, more members
> will be set when more functions are tested.
> Then unit test for ext4_mb_new_blocks_simple is added.
>
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/mballoc-test.c | 323 +++++++++++++++++++++++++++++++++++++++++
>  fs/ext4/mballoc.c      |   4 +
>  2 files changed, 327 insertions(+)
>  create mode 100644 fs/ext4/mballoc-test.c
>
> diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
> new file mode 100644
> index 000000000000..184e6cb2070f
> --- /dev/null
> +++ b/fs/ext4/mballoc-test.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test of ext4 multiblocks allocation.
> + */
> +
> +#include <kunit/test.h>
> +#include <kunit/static_stub.h>
> +
> +#include "ext4.h"
> +
> +struct mb_grp_ctx {
> +	struct buffer_head bitmap_bh;
> +	struct ext4_group_desc desc;
> +	/* one group descriptor for each group descriptor for simplicity */
> +	struct buffer_head gd_bh;
> +};

I suppose desc and gd_bh are just the place holders so that
ext4_mb_new_blocks_simple() doesn't fail right? Is there any other use
of this? Because I don't see we initializing these.

> +
> +struct mb_ctx {
> +	struct mb_grp_ctx *grp_ctx;
> +};
> +
> +struct fake_super_block {
> +	struct super_block sb;
> +	struct mb_ctx mb_ctx;
> +};
> +
> +#define MB_CTX(_sb) (&(container_of((_sb), struct fake_super_block, sb)->mb_ctx))
> +#define MB_GRP_CTX(_sb, _group) (&MB_CTX(_sb)->grp_ctx[_group])
> +
> +static struct super_block *alloc_fake_super_block(void)
> +{
> +	struct ext4_super_block *es = kzalloc(sizeof(*es), GFP_KERNEL);
> +	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> +	struct fake_super_block *fsb = kzalloc(sizeof(*fsb), GFP_KERNEL);
> +
> +	if (fsb == NULL || sbi == NULL || es == NULL)
> +		goto out;
> +
> +	sbi->s_es = es;
> +	fsb->sb.s_fs_info = sbi;
> +	return &fsb->sb;
> +
> +out:
> +	kfree(fsb);
> +	kfree(sbi);
> +	kfree(es);
> +	return NULL;
> +}
> +
> +static void free_fake_super_block(struct super_block *sb)
> +{
> +	struct fake_super_block *fsb = container_of(sb, struct fake_super_block, sb);
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	kfree(sbi->s_es);
> +	kfree(sbi);
> +	kfree(fsb);
> +}
> +
> +struct ext4_block_layout {
> +	unsigned char blocksize_bits;
> +	unsigned int cluster_bits;
> +	unsigned long blocks_per_group;
> +	ext4_group_t group_count;
> +	unsigned long desc_size;
> +};
> +
> +static void init_sb_layout(struct super_block *sb,
> +			  struct ext4_block_layout *layout)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_super_block *es = sbi->s_es;
> +
> +	sb->s_blocksize = 1UL << layout->blocksize_bits;
> +	sb->s_blocksize_bits = layout->blocksize_bits;
> +
> +	sbi->s_groups_count = layout->group_count;
> +	sbi->s_blocks_per_group = layout->blocks_per_group;
> +	sbi->s_cluster_bits = layout->cluster_bits;
> +	sbi->s_cluster_ratio = 1U << layout->cluster_bits;
> +	sbi->s_clusters_per_group = layout->blocks_per_group >>
> +				    layout->cluster_bits;
> +	sbi->s_desc_size = layout->desc_size;
> +
> +	es->s_first_data_block = cpu_to_le32(0);
> +	es->s_blocks_count_lo = cpu_to_le32(layout->blocks_per_group *
> +					    layout->group_count);
> +}
> +
> +static int mb_grp_ctx_init(struct super_block *sb,
> +			   struct mb_grp_ctx *grp_ctx)
> +{
> +	grp_ctx->bitmap_bh.b_data = kzalloc(EXT4_BLOCK_SIZE(sb), GFP_KERNEL);
> +	if (grp_ctx->bitmap_bh.b_data == NULL)
> +		return -ENOMEM;
> +
> +	get_bh(&grp_ctx->bitmap_bh);
> +	get_bh(&grp_ctx->gd_bh);
> +	return 0;
> +}
> +
> +static void mb_grp_ctx_release(struct mb_grp_ctx *grp_ctx)
> +{
> +	kfree(grp_ctx->bitmap_bh.b_data);
> +	grp_ctx->bitmap_bh.b_data = NULL;

No brelse() here?

> +}
> +
> +static void mb_ctx_mark_used(struct super_block *sb, ext4_group_t group,
> +			     unsigned int start, unsigned int len)
> +{
> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, group);
> +
> +	mb_set_bits(grp_ctx->bitmap_bh.b_data, start, len);
> +}
> +
> +/* called after init_sb_layout */
> +static int mb_ctx_init(struct super_block *sb)
> +{
> +	struct mb_ctx *ctx = MB_CTX(sb);
> +	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
> +
> +	ctx->grp_ctx = kcalloc(ngroups, sizeof(struct mb_grp_ctx),
> +			       GFP_KERNEL);
> +	if (ctx->grp_ctx == NULL)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ngroups; i++)
> +		if (mb_grp_ctx_init(sb, &ctx->grp_ctx[i]))
> +			goto out;
> +
> +	/*
> +	 * first data block(first cluster in first group) is used by
> +	 * metadata, mark it used to avoid to alloc data block at first
> +	 * block which will fail ext4_sb_block_valid check.
> +	 */
> +	mb_set_bits(ctx->grp_ctx[0].bitmap_bh.b_data, 0, 1);
> +
> +	return 0;
> +out:
> +	while (i-- > 0)
> +		mb_grp_ctx_release(&ctx->grp_ctx[i]);
> +	kfree(ctx->grp_ctx);
> +	return -ENOMEM;
> +}
> +
> +static void mb_ctx_release(struct super_block *sb)
> +{
> +	struct mb_ctx *ctx = MB_CTX(sb);
> +	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
> +
> +	for (i = 0; i < ngroups; i++)
> +		mb_grp_ctx_release(&ctx->grp_ctx[i]);
> +	kfree(ctx->grp_ctx);
> +}
> +
> +static struct buffer_head *
> +ext4_read_block_bitmap_nowait_stub(struct super_block *sb, ext4_group_t block_group,
> +				   bool ignore_locked)
> +{
> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
> +
> +	get_bh(&grp_ctx->bitmap_bh);

I don't know how will you call brelse() for this?
It should be ok anyways since it's not a real buffer_head. But may be it
will be good to add a comment about it.

> +	return &grp_ctx->bitmap_bh;
> +}
> +
> +static int ext4_wait_block_bitmap_stub(struct super_block *sb,
> +				ext4_group_t block_group,
> +				struct buffer_head *bh)
> +{
> +	return 0;
> +}
> +
> +static struct ext4_group_desc *
> +ext4_get_group_desc_stub(struct super_block *sb, ext4_group_t block_group,
> +			 struct buffer_head **bh)
> +{
> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
> +
> +	if (bh != NULL)
> +		*bh = &grp_ctx->gd_bh;
> +
> +	return &grp_ctx->desc;
> +}
> +
> +static int ext4_mb_mark_group_bb_stub(struct ext4_mark_context *mc,
> +			       ext4_group_t group, ext4_grpblk_t blkoff,
> +			       ext4_grpblk_t len, int flags)
> +{
> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(mc->sb, group);
> +	struct buffer_head *bitmap_bh = &grp_ctx->bitmap_bh;
> +
> +	if (mc->state)
> +		mb_set_bits(bitmap_bh->b_data, blkoff, len);
> +	else
> +		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
> +
> +	return 0;
> +}
> +
> +#define TEST_BLOCKSIZE_BITS 10
> +#define TEST_CLUSTER_BITS 3
> +#define TEST_BLOCKS_PER_GROUP 8192
> +#define TEST_GROUP_COUNT 4
> +#define TEST_DESC_SIZE 64
> +#define TEST_GOAL_GROUP 1

Rather then defining this statically, can we add a test for testing
different options. like for e.g. bs=1k, 4k, and 64k to be tested in a
loop or something with maybe diffrent group_count values to start with? 

At a high level, I went over the code and this looks like a good
first start to me. One suggestion is to prefix the mballoc kunit tests
with some string to differentiate what is kunit related and what is
actual ext4/mballoc calls. Once this kunit test grows, I am sure it will
be difficult to make out the differece between test related APIs and
actual kernel APIs.

maybe something like mbt_

so mb_grp_ctx -> mbt_grp_ctx
mb_ctx -> mbt_ctx
fake_super_block -> mbt_ext4_super_block
alloc/free_fake_super_block -> mbt_ext4_alloc/free_super_block

ext4_block_layout -> mbt_ext4_block_layout

init_sb_layout -> mbt_init_sb_layout

mb_grp_ctx_init/release -> mbt_grp_ctx_init/release
mb_ctx_mark_used -> mbt_ctx_mark_used

mballoc_test_init -> mbt_kunit_init
ext4_mballoc_test_cases -> mbt_test_cases 

funtions with _stub looks ok to me, as we can clearly differentiate them
from actual functions. 


-ritesh


> +static int mballoc_test_init(struct kunit *test)
> +{
> +	struct ext4_block_layout layout = {
> +		.blocksize_bits = TEST_BLOCKSIZE_BITS,
> +		.cluster_bits = TEST_CLUSTER_BITS,
> +		.blocks_per_group = TEST_BLOCKS_PER_GROUP,
> +		.group_count = TEST_GROUP_COUNT,
> +		.desc_size = TEST_DESC_SIZE,
> +	};
> +	struct super_block *sb;
> +	int ret;
> +
> +	sb = alloc_fake_super_block();
> +	if (sb == NULL)
> +		return -ENOMEM;
> +
> +	init_sb_layout(sb, &layout);
> +
> +	ret = mb_ctx_init(sb);
> +	if (ret != 0) {
> +		free_fake_super_block(sb);
> +		return ret;
> +	}
> +
> +	test->priv = sb;
> +	kunit_activate_static_stub(test,
> +				   ext4_read_block_bitmap_nowait,
> +				   ext4_read_block_bitmap_nowait_stub);
> +	kunit_activate_static_stub(test,
> +				   ext4_wait_block_bitmap,
> +				   ext4_wait_block_bitmap_stub);
> +	kunit_activate_static_stub(test,
> +				   ext4_get_group_desc,
> +				   ext4_get_group_desc_stub);
> +	kunit_activate_static_stub(test,
> +				   ext4_mb_mark_group_bb,
> +				   ext4_mb_mark_group_bb_stub);
> +	return 0;
> +}
> +
> +static void mballoc_test_exit(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +
> +	mb_ctx_release(sb);
> +	free_fake_super_block(sb);
> +}
> +
> +static void test_new_blocks_simple(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +	struct inode inode = { .i_sb = sb, };
> +	struct ext4_allocation_request ar;
> +	ext4_group_t i, goal_group = TEST_GOAL_GROUP;
> +	int err = 0;
> +	ext4_fsblk_t found;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	ar.inode = &inode;
> +
> +	/* get block at goal */
> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
> +	found = ext4_mb_new_blocks_simple(&ar, &err);
> +	KUNIT_ASSERT_EQ_MSG(test, ar.goal, found,
> +		"failed to alloc block at goal, expected %llu found %llu",
> +		ar.goal, found);
> +
> +	/* get block after goal in goal group */
> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
> +	found = ext4_mb_new_blocks_simple(&ar, &err);
> +	KUNIT_ASSERT_EQ_MSG(test, ar.goal + EXT4_C2B(sbi, 1), found,
> +		"failed to alloc block after goal in goal group, expected %llu found %llu",
> +		ar.goal + 1, found);
> +
> +	/* get block after goal group */
> +	mb_ctx_mark_used(sb, goal_group, 0, EXT4_CLUSTERS_PER_GROUP(sb));
> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
> +	found = ext4_mb_new_blocks_simple(&ar, &err);
> +	KUNIT_ASSERT_EQ_MSG(test,
> +		ext4_group_first_block_no(sb, goal_group + 1), found,
> +		"failed to alloc block after goal group, expected %llu found %llu",
> +		ext4_group_first_block_no(sb, goal_group + 1), found);
> +
> +	/* get block before goal group */
> +	for (i = goal_group; i < ext4_get_groups_count(sb); i++)
> +		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
> +	found = ext4_mb_new_blocks_simple(&ar, &err);
> +	KUNIT_ASSERT_EQ_MSG(test,
> +		ext4_group_first_block_no(sb, 0) + EXT4_C2B(sbi, 1), found,
> +		"failed to alloc block before goal group, expected %llu found %llu",
> +		ext4_group_first_block_no(sb, 0 + EXT4_C2B(sbi, 1)), found);
> +
> +	/* no block available, fail to allocate block */
> +	for (i = 0; i < ext4_get_groups_count(sb); i++)
> +		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
> +	found = ext4_mb_new_blocks_simple(&ar, &err);
> +	KUNIT_ASSERT_NE_MSG(test, err, 0,
> +		"unexpectedly get block when no block is available");
> +}
> +
> +
> +static struct kunit_case ext4_mballoc_test_cases[] = {
> +	KUNIT_CASE(test_new_blocks_simple),
> +	{}
> +};
> +
> +static struct kunit_suite ext4_mballoc_test_suite = {
> +	.name = "ext4_mballoc_test",
> +	.init = mballoc_test_init,
> +	.exit = mballoc_test_exit,
> +	.test_cases = ext4_mballoc_test_cases,
> +};
> +
> +kunit_test_suites(&ext4_mballoc_test_suite);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index dd2fc0546c0b..b6b963412cdc 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6954,3 +6954,7 @@ ext4_mballoc_query_range(
>  
>  	return error;
>  }
> +
> +#ifdef CONFIG_EXT4_KUNIT_TESTS
> +#include "mballoc-test.c"
> +#endif
> -- 
> 2.30.0

Re: [PATCH v5 8/8] ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

[Kemeng Shi]

on 8/3/2023 10:41 PM, Ritesh Harjani wrote:
> Kemeng Shi <shikemeng@huaweicloud.com> writes:
> 
>> Here are prepared work:
>> 1. Include mballoc-test.c to mballoc.c to be able test static function
>> in mballoc.c.
>> 2. Implement static stub to avoid read IO to disk.
>> 3. Construct fake super_block. Only partial members are set, more members
>> will be set when more functions are tested.
>> Then unit test for ext4_mb_new_blocks_simple is added.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/mballoc-test.c | 323 +++++++++++++++++++++++++++++++++++++++++
>>  fs/ext4/mballoc.c      |   4 +
>>  2 files changed, 327 insertions(+)
>>  create mode 100644 fs/ext4/mballoc-test.c
>>
>> diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
>> new file mode 100644
>> index 000000000000..184e6cb2070f
>> --- /dev/null
>> +++ b/fs/ext4/mballoc-test.c
>> @@ -0,0 +1,323 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * KUnit test of ext4 multiblocks allocation.
>> + */
>> +
>> +#include <kunit/test.h>
>> +#include <kunit/static_stub.h>
>> +
>> +#include "ext4.h"
>> +
>> +struct mb_grp_ctx {
>> +	struct buffer_head bitmap_bh;
>> +	struct ext4_group_desc desc;
>> +	/* one group descriptor for each group descriptor for simplicity */
>> +	struct buffer_head gd_bh;
>> +};
> 
> I suppose desc and gd_bh are just the place holders so that
> ext4_mb_new_blocks_simple() doesn't fail right? Is there any other use
> of this? Because I don't see we initializing these.
> 
>> +
>> +struct mb_ctx {
>> +	struct mb_grp_ctx *grp_ctx;
>> +};
>> +
>> +struct fake_super_block {
>> +	struct super_block sb;
>> +	struct mb_ctx mb_ctx;
>> +};
>> +
>> +#define MB_CTX(_sb) (&(container_of((_sb), struct fake_super_block, sb)->mb_ctx))
>> +#define MB_GRP_CTX(_sb, _group) (&MB_CTX(_sb)->grp_ctx[_group])
>> +
>> +static struct super_block *alloc_fake_super_block(void)
>> +{
>> +	struct ext4_super_block *es = kzalloc(sizeof(*es), GFP_KERNEL);
>> +	struct ext4_sb_info *sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> +	struct fake_super_block *fsb = kzalloc(sizeof(*fsb), GFP_KERNEL);
>> +
>> +	if (fsb == NULL || sbi == NULL || es == NULL)
>> +		goto out;
>> +
>> +	sbi->s_es = es;
>> +	fsb->sb.s_fs_info = sbi;
>> +	return &fsb->sb;
>> +
>> +out:
>> +	kfree(fsb);
>> +	kfree(sbi);
>> +	kfree(es);
>> +	return NULL;
>> +}
>> +
>> +static void free_fake_super_block(struct super_block *sb)
>> +{
>> +	struct fake_super_block *fsb = container_of(sb, struct fake_super_block, sb);
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +
>> +	kfree(sbi->s_es);
>> +	kfree(sbi);
>> +	kfree(fsb);
>> +}
>> +
>> +struct ext4_block_layout {
>> +	unsigned char blocksize_bits;
>> +	unsigned int cluster_bits;
>> +	unsigned long blocks_per_group;
>> +	ext4_group_t group_count;
>> +	unsigned long desc_size;
>> +};
>> +
>> +static void init_sb_layout(struct super_block *sb,
>> +			  struct ext4_block_layout *layout)
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +	struct ext4_super_block *es = sbi->s_es;
>> +
>> +	sb->s_blocksize = 1UL << layout->blocksize_bits;
>> +	sb->s_blocksize_bits = layout->blocksize_bits;
>> +
>> +	sbi->s_groups_count = layout->group_count;
>> +	sbi->s_blocks_per_group = layout->blocks_per_group;
>> +	sbi->s_cluster_bits = layout->cluster_bits;
>> +	sbi->s_cluster_ratio = 1U << layout->cluster_bits;
>> +	sbi->s_clusters_per_group = layout->blocks_per_group >>
>> +				    layout->cluster_bits;
>> +	sbi->s_desc_size = layout->desc_size;
>> +
>> +	es->s_first_data_block = cpu_to_le32(0);
>> +	es->s_blocks_count_lo = cpu_to_le32(layout->blocks_per_group *
>> +					    layout->group_count);
>> +}
>> +
>> +static int mb_grp_ctx_init(struct super_block *sb,
>> +			   struct mb_grp_ctx *grp_ctx)
>> +{
>> +	grp_ctx->bitmap_bh.b_data = kzalloc(EXT4_BLOCK_SIZE(sb), GFP_KERNEL);
>> +	if (grp_ctx->bitmap_bh.b_data == NULL)
>> +		return -ENOMEM;
>> +
>> +	get_bh(&grp_ctx->bitmap_bh);
>> +	get_bh(&grp_ctx->gd_bh);
>> +	return 0;
>> +}
>> +
>> +static void mb_grp_ctx_release(struct mb_grp_ctx *grp_ctx)
>> +{
>> +	kfree(grp_ctx->bitmap_bh.b_data);
>> +	grp_ctx->bitmap_bh.b_data = NULL;
> 
> No brelse() here?
>>> +}
>> +
>> +static void mb_ctx_mark_used(struct super_block *sb, ext4_group_t group,
>> +			     unsigned int start, unsigned int len)
>> +{
>> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, group);
>> +
>> +	mb_set_bits(grp_ctx->bitmap_bh.b_data, start, len);
>> +}
>> +
>> +/* called after init_sb_layout */
>> +static int mb_ctx_init(struct super_block *sb)
>> +{
>> +	struct mb_ctx *ctx = MB_CTX(sb);
>> +	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
>> +
>> +	ctx->grp_ctx = kcalloc(ngroups, sizeof(struct mb_grp_ctx),
>> +			       GFP_KERNEL);
>> +	if (ctx->grp_ctx == NULL)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < ngroups; i++)
>> +		if (mb_grp_ctx_init(sb, &ctx->grp_ctx[i]))
>> +			goto out;
>> +
>> +	/*
>> +	 * first data block(first cluster in first group) is used by
>> +	 * metadata, mark it used to avoid to alloc data block at first
>> +	 * block which will fail ext4_sb_block_valid check.
>> +	 */
>> +	mb_set_bits(ctx->grp_ctx[0].bitmap_bh.b_data, 0, 1);
>> +
>> +	return 0;
>> +out:
>> +	while (i-- > 0)
>> +		mb_grp_ctx_release(&ctx->grp_ctx[i]);
>> +	kfree(ctx->grp_ctx);
>> +	return -ENOMEM;
>> +}
>> +
>> +static void mb_ctx_release(struct super_block *sb)
>> +{
>> +	struct mb_ctx *ctx = MB_CTX(sb);
>> +	ext4_group_t i, ngroups = ext4_get_groups_count(sb);
>> +
>> +	for (i = 0; i < ngroups; i++)
>> +		mb_grp_ctx_release(&ctx->grp_ctx[i]);
>> +	kfree(ctx->grp_ctx);
>> +}
>> +
>> +static struct buffer_head *
>> +ext4_read_block_bitmap_nowait_stub(struct super_block *sb, ext4_group_t block_group,
>> +				   bool ignore_locked)
>> +{
>> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
>> +
>> +	get_bh(&grp_ctx->bitmap_bh);
> 
> I don't know how will you call brelse() for this?
> It should be ok anyways since it's not a real buffer_head. But may be it
> will be good to add a comment about it.
> 
Sure, but I'm considering to simply remove all get_bh for fake buffer_heador.
And comments will be added if these get_bhs are kept for some unkonw limitation.
>> +	return &grp_ctx->bitmap_bh;
>> +}
>> +
>> +static int ext4_wait_block_bitmap_stub(struct super_block *sb,
>> +				ext4_group_t block_group,
>> +				struct buffer_head *bh)
>> +{
>> +	return 0;
>> +}
>> +
>> +static struct ext4_group_desc *
>> +ext4_get_group_desc_stub(struct super_block *sb, ext4_group_t block_group,
>> +			 struct buffer_head **bh)
>> +{
>> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(sb, block_group);
>> +
>> +	if (bh != NULL)
>> +		*bh = &grp_ctx->gd_bh;
>> +
>> +	return &grp_ctx->desc;
>> +}
>> +
>> +static int ext4_mb_mark_group_bb_stub(struct ext4_mark_context *mc,
>> +			       ext4_group_t group, ext4_grpblk_t blkoff,
>> +			       ext4_grpblk_t len, int flags)
>> +{
>> +	struct mb_grp_ctx *grp_ctx = MB_GRP_CTX(mc->sb, group);
>> +	struct buffer_head *bitmap_bh = &grp_ctx->bitmap_bh;
>> +
>> +	if (mc->state)
>> +		mb_set_bits(bitmap_bh->b_data, blkoff, len);
>> +	else
>> +		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
>> +
>> +	return 0;
>> +}
>> +
>> +#define TEST_BLOCKSIZE_BITS 10
>> +#define TEST_CLUSTER_BITS 3
>> +#define TEST_BLOCKS_PER_GROUP 8192
>> +#define TEST_GROUP_COUNT 4
>> +#define TEST_DESC_SIZE 64
>> +#define TEST_GOAL_GROUP 1
> 
> Rather then defining this statically, can we add a test for testing
> different options. like for e.g. bs=1k, 4k, and 64k to be tested in a
> loop or something with maybe diffrent group_count values to start with? 
> 
Yes, it looks better in this way. I will add several configures to test.
> At a high level, I went over the code and this looks like a good
> first start to me. One suggestion is to prefix the mballoc kunit tests
> with some string to differentiate what is kunit related and what is
> actual ext4/mballoc calls. Once this kunit test grows, I am sure it will
> be difficult to make out the differece between test related APIs and
> actual kernel APIs.
> 
> maybe something like mbt_
> 
> so mb_grp_ctx -> mbt_grp_ctx
> mb_ctx -> mbt_ctx
> fake_super_block -> mbt_ext4_super_block
> alloc/free_fake_super_block -> mbt_ext4_alloc/free_super_block
> 
> ext4_block_layout -> mbt_ext4_block_layout
> 
> init_sb_layout -> mbt_init_sb_layout
> 
> mb_grp_ctx_init/release -> mbt_grp_ctx_init/release
> mb_ctx_mark_used -> mbt_ctx_mark_used
> 
> mballoc_test_init -> mbt_kunit_init
> ext4_mballoc_test_cases -> mbt_test_cases 
> 
Sure, prefix mbt just looks good to me.
> funtions with _stub looks ok to me, as we can clearly differentiate them
> from actual functions. 
> 
> 
> -ritesh
> 
Thanks for all advices!
> 
>> +static int mballoc_test_init(struct kunit *test)
>> +{
>> +	struct ext4_block_layout layout = {
>> +		.blocksize_bits = TEST_BLOCKSIZE_BITS,
>> +		.cluster_bits = TEST_CLUSTER_BITS,
>> +		.blocks_per_group = TEST_BLOCKS_PER_GROUP,
>> +		.group_count = TEST_GROUP_COUNT,
>> +		.desc_size = TEST_DESC_SIZE,
>> +	};
>> +	struct super_block *sb;
>> +	int ret;
>> +
>> +	sb = alloc_fake_super_block();
>> +	if (sb == NULL)
>> +		return -ENOMEM;
>> +
>> +	init_sb_layout(sb, &layout);
>> +
>> +	ret = mb_ctx_init(sb);
>> +	if (ret != 0) {
>> +		free_fake_super_block(sb);
>> +		return ret;
>> +	}
>> +
>> +	test->priv = sb;
>> +	kunit_activate_static_stub(test,
>> +				   ext4_read_block_bitmap_nowait,
>> +				   ext4_read_block_bitmap_nowait_stub);
>> +	kunit_activate_static_stub(test,
>> +				   ext4_wait_block_bitmap,
>> +				   ext4_wait_block_bitmap_stub);
>> +	kunit_activate_static_stub(test,
>> +				   ext4_get_group_desc,
>> +				   ext4_get_group_desc_stub);
>> +	kunit_activate_static_stub(test,
>> +				   ext4_mb_mark_group_bb,
>> +				   ext4_mb_mark_group_bb_stub);
>> +	return 0;
>> +}
>> +
>> +static void mballoc_test_exit(struct kunit *test)
>> +{
>> +	struct super_block *sb = (struct super_block *)test->priv;
>> +
>> +	mb_ctx_release(sb);
>> +	free_fake_super_block(sb);
>> +}
>> +
>> +static void test_new_blocks_simple(struct kunit *test)
>> +{
>> +	struct super_block *sb = (struct super_block *)test->priv;
>> +	struct inode inode = { .i_sb = sb, };
>> +	struct ext4_allocation_request ar;
>> +	ext4_group_t i, goal_group = TEST_GOAL_GROUP;
>> +	int err = 0;
>> +	ext4_fsblk_t found;
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +
>> +	ar.inode = &inode;
>> +
>> +	/* get block at goal */
>> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
>> +	found = ext4_mb_new_blocks_simple(&ar, &err);
>> +	KUNIT_ASSERT_EQ_MSG(test, ar.goal, found,
>> +		"failed to alloc block at goal, expected %llu found %llu",
>> +		ar.goal, found);
>> +
>> +	/* get block after goal in goal group */
>> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
>> +	found = ext4_mb_new_blocks_simple(&ar, &err);
>> +	KUNIT_ASSERT_EQ_MSG(test, ar.goal + EXT4_C2B(sbi, 1), found,
>> +		"failed to alloc block after goal in goal group, expected %llu found %llu",
>> +		ar.goal + 1, found);
>> +
>> +	/* get block after goal group */
>> +	mb_ctx_mark_used(sb, goal_group, 0, EXT4_CLUSTERS_PER_GROUP(sb));
>> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
>> +	found = ext4_mb_new_blocks_simple(&ar, &err);
>> +	KUNIT_ASSERT_EQ_MSG(test,
>> +		ext4_group_first_block_no(sb, goal_group + 1), found,
>> +		"failed to alloc block after goal group, expected %llu found %llu",
>> +		ext4_group_first_block_no(sb, goal_group + 1), found);
>> +
>> +	/* get block before goal group */
>> +	for (i = goal_group; i < ext4_get_groups_count(sb); i++)
>> +		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
>> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
>> +	found = ext4_mb_new_blocks_simple(&ar, &err);
>> +	KUNIT_ASSERT_EQ_MSG(test,
>> +		ext4_group_first_block_no(sb, 0) + EXT4_C2B(sbi, 1), found,
>> +		"failed to alloc block before goal group, expected %llu found %llu",
>> +		ext4_group_first_block_no(sb, 0 + EXT4_C2B(sbi, 1)), found);
>> +
>> +	/* no block available, fail to allocate block */
>> +	for (i = 0; i < ext4_get_groups_count(sb); i++)
>> +		mb_ctx_mark_used(sb, i, 0, EXT4_CLUSTERS_PER_GROUP(sb));
>> +	ar.goal = ext4_group_first_block_no(sb, goal_group);
>> +	found = ext4_mb_new_blocks_simple(&ar, &err);
>> +	KUNIT_ASSERT_NE_MSG(test, err, 0,
>> +		"unexpectedly get block when no block is available");
>> +}
>> +
>> +
>> +static struct kunit_case ext4_mballoc_test_cases[] = {
>> +	KUNIT_CASE(test_new_blocks_simple),
>> +	{}
>> +};
>> +
>> +static struct kunit_suite ext4_mballoc_test_suite = {
>> +	.name = "ext4_mballoc_test",
>> +	.init = mballoc_test_init,
>> +	.exit = mballoc_test_exit,
>> +	.test_cases = ext4_mballoc_test_cases,
>> +};
>> +
>> +kunit_test_suites(&ext4_mballoc_test_suite);
>> +
>> +MODULE_LICENSE("GPL");
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index dd2fc0546c0b..b6b963412cdc 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -6954,3 +6954,7 @@ ext4_mballoc_query_range(
>>  
>>  	return error;
>>  }
>> +
>> +#ifdef CONFIG_EXT4_KUNIT_TESTS
>> +#include "mballoc-test.c"
>> +#endif
>> -- 
>> 2.30.0
> 

-- 
Best wishes
Kemeng Shi

Re: [PATCH v5 8/8] ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

[Theodore Ts'o]

Hi, it sounds like you are planning on a few additional changes to
this patch series, right?  When might you be planning on pushing out
the next version?

Thanks,

					- Ted

Re: [PATCH v5 8/8] ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc

[Kemeng Shi]

on 8/18/2023 1:07 PM, Theodore Ts'o wrote:
> Hi, it sounds like you are planning on a few additional changes to
> this patch series, right?  When might you be planning on pushing out
> the next version?
> 
> Thanks,
Yes, Ritesh kindly offered advises and I'm going to finish this by end
of next week. Otherwise if anything comes up, I'll  target at the week
after.
> 
> 					- Ted
>