Re: [PATCH 06/27] ext4: expand KUnit tests for fs/ext4/mballoc.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [Medium] ext4: test_mb_good_group() and test_mb_prefetch() fail on
  16K/64K PAGE_SIZE
- [Low] ext4: unpinned per-CPU locality group lookups across sleeps in
  mballoc-test.c
- [Low] ext4: unaligned __u32 accesses in mb_set_bits() and
  mb_test_and_clear_bits()
- [Low] ext4: test_mb_regular_allocator() step 3 skips CR_BEST_AVAIL_LEN
  trimming
- [Low] ext4: missing bb_free update in
  test_mb_simple_and_aligned_scan()
- [Low] ext4: unused *_test() wrapper functions in fs/ext4/mballoc.c
--

commit 72a0b9666c2cc1afd5fa22724c492669c1b39ccf
Author: Theodore Ts'o <tytso@mit.edu>

ext4: expand KUnit tests for fs/ext4/mballoc.c

This commit expands the ext4_mballoc_test KUnit suite in
fs/ext4/mballoc-test.c with 25 new parameterized tests and exports the
corresponding helper wrappers from fs/ext4/mballoc.c.

> diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
> index d31780075c21e..402f4efdf25a4 100644
> --- a/fs/ext4/mballoc-test.c
> +++ b/fs/ext4/mballoc-test.c

[ ... ]

> @@ -949,6 +1008,1747 @@ static void test_mb_mark_used_cost(struct kunit *test)
>  	ext4_mb_unload_buddy_test(&e4b);
>  }
>  
> +static void test_mb_bit_ops(struct kunit *test)
> +{
> +	u8 *buf;
> +	void *unaligned;
> +	int bit, i;
> +
> +	buf = kunit_kzalloc(test, 128, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> +
> +	/*
> +	 * Use an unaligned pointer (offset by 3 bytes) to exercise
> +	 * mb_correct_addr_and_bit() on 64-bit/32-bit architectures.
> +	 */
> +	unaligned = buf + 3;

[ ... ]

> +	/* Multi-word unaligned range spanning head bits, 32-bit words, and tail */
> +	mb_set_bits(unaligned, 5, 75);
> +	KUNIT_EXPECT_EQ(test, mb_find_next_bit_test(unaligned, 256, 0), 5);
> +	KUNIT_EXPECT_EQ(test, mb_find_next_zero_bit_test(unaligned, 256, 5), 80);
> +	/* Clamping when no zero bit exists before max */
> +	KUNIT_EXPECT_EQ(test, mb_find_next_zero_bit_test(unaligned, 50, 5), 50);
> +
> +	/* Clear a sub-range across 32-bit boundaries */
> +	mb_clear_bits_test(unaligned, 13, 50);
> +	for (i = 0; i < 100; i++) {
> +		int expected = (i >= 5 && i < 13) || (i >= 63 && i < 80);
> +
> +		KUNIT_EXPECT_EQ(test, !!mb_test_bit_test(i, unaligned), expected);
> +	}
> +}
> +
> +static void test_mb_test_and_clear_bits(struct kunit *test)
> +{
> +	u8 *buf;
> +	void *bm;
> +	int ret, i;
> +
> +	buf = kunit_kzalloc(test, 128, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
> +	bm = buf + 3;
> +
> +	/* Case 1: All bits in range [5, 85) are set -> returns -1 and clears all */
> +	memset(buf, 0xff, 128);
> +	ret = mb_test_and_clear_bits_test(bm, 5, 80);

[Severity: Low]
Does passing buf + 3 to mb_set_bits() and mb_test_and_clear_bits_test()
cause misaligned 32-bit accesses?

Unlike the single-bit helpers that align the base address via
mb_correct_addr_and_bit(), mb_set_bits() and mb_test_and_clear_bits() take
a 32-bit word fast path when (cur & 31) == 0 && (len - cur) >= 32 and
dereference __u32 *addr = bm + (cur >> 3) directly.  When cur reaches 32,
addr is buf + 7, which triggers UBSAN alignment warnings under
CONFIG_UBSAN_ALIGNMENT or alignment faults on strict-alignment
architectures.

[ ... ]

> +static void test_mb_good_group(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_buddy e4b;
> +	struct ext4_allocation_context ac = {};
> +	struct ext4_group_info *grp1, *grp2;
> +	int ret;
> +
> +	if (sb->s_blocksize > PAGE_SIZE)
> +		kunit_skip(test, "blocksize exceeds pagesize");
> +
> +	/* Initialize group 1 buddy cache so bb_fragments and orders are populated */
> +	ret = ext4_mb_load_buddy_test(sb, TEST_GOAL_GROUP, &e4b);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	ext4_mb_unload_buddy_test(&e4b);

[ ... ]

> +	/*
> +	 * Test ext4_mb_good_group_nolock on uninitialized group 2:
> +	 * At inexpensive CR_POWER2_ALIGNED, it skips initializing group 2 and returns 0.
> +	 * At expensive CR_GOAL_LEN_SLOW (with STRICT_CHECK), it initializes group 2
> +	 * and returns 1.
> +	 */
> +	grp2 = ext4_get_group_info(sb, 2);
> +	KUNIT_ASSERT_NOT_NULL(test, grp2);
> +	KUNIT_EXPECT_TRUE(test, EXT4_MB_GRP_NEED_INIT(grp2));
> +
> +	ac.ac_criteria = CR_POWER2_ALIGNED;\n> +	ret = ext4_mb_good_group_nolock_test(&ac, 2, CR_POWER2_ALIGNED);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	KUNIT_EXPECT_TRUE(test, EXT4_MB_GRP_NEED_INIT(grp2));

[Severity: Medium]
Will test_mb_good_group() and test_mb_prefetch() fail on 16K or 64K
PAGE_SIZE kernels?

ext4_mb_init_cache() initializes groups_per_folio =
DIV_ROUND_UP(folio_size(folio) / blocksize, 2) consecutive groups at once
whenever a buddy-cache folio is populated.  When blocksize is 1K on a 16K
page kernel, or 1K/4K on a 64K page kernel, groups_per_folio is at least
8, so loading group 1's buddy (or calling ext4_mb_prefetch_fini() on
groups [0, 2) in test_mb_prefetch()) also initializes group 2 on folio 0
and clears EXT4_GROUP_INFO_NEED_INIT_BIT on grp2, causing the
EXT4_MB_GRP_NEED_INIT(grp2) expectations in both tests to fail.

[ ... ]

> +static void test_mb_simple_and_aligned_scan(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_buddy e4b;
> +	struct ext4_allocation_context ac = {};
> +	struct inode *inode;
> +	int ret;
> +
> +	if (sb->s_blocksize > PAGE_SIZE)
> +		kunit_skip(test, "blocksize exceeds pagesize");
> +
> +	inode = mbt_alloc_test_inode(test, sb, S_IFREG);
> +
> +	/*
> +	 * Mark cluster 0 used in group 1 so:
> +	 * - order 3 buddy [8, 16) is the first order-3 buddy
> +	 * - first 16-cluster stripe [0, 16) is not completely free, so
> +	 *   stripe-aligned scan must skip to cluster 16.
> +	 */
> +	mbt_ctx_mark_used(sb, TEST_GOAL_GROUP, 0, 1);
> +
> +	ret = ext4_mb_load_buddy_test(sb, TEST_GOAL_GROUP, &e4b);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Low]
Does marking cluster 0 used here without decrementing
ext4_get_group_info(sb, TEST_GOAL_GROUP)->bb_free trigger a false bitmap
corruption error?

When ext4_mb_load_buddy_test() calls ext4_mb_generate_buddy(), the bitmap
free count is 1 less than grp->bb_free, which logs an inconsistency error
via ext4_grp_locked_error() and sets EXT4_GROUP_INFO_BBITMAP_CORRUPT on
TEST_GOAL_GROUP.

[ ... ]

> +static void test_mb_regular_allocator(struct kunit *test)
> +{

[ ... ]

> +	/*
> +	 * 3. MB_OPTIMIZE_SCAN with CR_BEST_AVAIL_LEN goal length trimming:
> +	 * Make group 2 the only group with free blocks, fragmented into two
> +	 * 8-cluster chunks [0, 8) and [16, 24) so largest_free_order = 3
> +	 * and avg_fragment_size = 8.
> +	 * Note: group 2 may have already been prefetched during step 2, so
> +	 * mark its blocks used via mb_mark_used_test on the loaded buddy.
> +	 */

[ ... ]

> +	memset(&ac, 0, sizeof(ac));
> +	ac.ac_sb = sb;
> +	ac.ac_inode = inode;
> +	ac.ac_status = AC_STATUS_CONTINUE;
> +	ac.ac_flags = EXT4_MB_HINT_DATA;
> +	ac.ac_g_ex.fe_group = 0;
> +	ac.ac_g_ex.fe_len = 16;
> +	ac.ac_orig_goal_len = 16;
> +	ac.ac_o_ex.fe_len = 8;
> +
> +	ret = ext4_mb_regular_allocator_test(&ac);

[Severity: Low]
Does setting ac_o_ex.fe_len = 8 prevent ext4_mb_scan_groups_best_avail()
from trimming ac_g_ex.fe_len down to 8 at CR_BEST_AVAIL_LEN?

In ext4_mb_scan_groups_best_avail(), order = fls(16) - 1 = 4 and min_order
starts at 4 - 3 = 1, but because (1 << min_order) < ac->ac_o_ex.fe_len, it
clamps min_order to fls(8) = 4:

fs/ext4/mballoc.c:ext4_mb_scan_groups_best_avail() {
    ...
	order = fls(ac->ac_g_ex.fe_len) - 1;
    ...
	min_order = order - sbi->s_mb_best_avail_max_trim_order;
	if (min_order < 0)
		min_order = 0;
	if (1 << min_order < ac->ac_o_ex.fe_len)
		min_order = fls(ac->ac_o_ex.fe_len);

	for (i = order; i >= min_order; i--) {
    ...
}

As a result, the loop only checks i = 4 (fe_len = 16) and never trims to
i = 3 (fe_len = 8), so the allocation only succeeds after falling back to
CR_GOAL_LEN_SLOW and ext4_mb_try_best_found().  Setting ac_o_ex.fe_len <= 4
would allow min_order <= 3 so CR_BEST_AVAIL_LEN trims fe_len to 8.

[ ... ]

> +static void test_mb_group_pa_lifecycle(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct inode *inode1, *inode2;
> +	struct ext4_allocation_request ar = {};
> +	struct ext4_group_info *grp;
> +	ext4_fsblk_t blk1, blk2;
> +	int err = 0, discarded;
> +
> +	if (sb->s_blocksize > PAGE_SIZE)
> +		kunit_skip(test, "blocksize exceeds pagesize");
> +
> +	inode1 = mbt_alloc_test_inode(test, sb, S_IFREG);
> +	inode2 = mbt_alloc_test_inode(test, sb, S_IFREG);
> +	inode2->i_ino = 13;
> +
> +	/* Small files (<= s_mb_stream_request) use locality group PA (MB_GROUP_PA) */
> +	i_size_write(inode1, sb->s_blocksize);
> +	i_size_write(inode2, sb->s_blocksize);
> +
> +	ar.inode = inode1;
> +	ar.logical = 0;
> +	ar.len = 1;
> +	ar.goal = ext4_group_first_block_no(sb, TEST_GOAL_GROUP);
> +	ar.flags = EXT4_MB_HINT_DATA;
> +
> +	blk1 = ext4_mb_new_blocks(NULL, &ar, &err);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	KUNIT_EXPECT_EQ(test, blk1, ext4_group_first_block_no(sb, TEST_GOAL_GROUP));
> +
> +	/* Second small file allocation reuses the locality group PA */
> +	ar.inode = inode2;
> +	ar.logical = 0;
> +	ar.len = 1;
> +	ar.goal = ext4_group_first_block_no(sb, TEST_GOAL_GROUP);
> +	blk2 = ext4_mb_new_blocks(NULL, &ar, &err);
> +	KUNIT_ASSERT_EQ(test, err, 0);
> +	KUNIT_EXPECT_EQ(test, blk2, blk1 + EXT4_C2B(sbi, 1));
> +
> +	/* Discard all group preallocations and verify free count is restored */
> +	discarded = ext4_mb_discard_preallocations_test(sb, 0);
> +	rcu_barrier();
> +	KUNIT_EXPECT_EQ(test, discarded, (int)sbi->s_mb_group_prealloc - 2);
> +
> +	grp = ext4_get_group_info(sb, TEST_GOAL_GROUP);
> +	KUNIT_EXPECT_EQ(test, grp->bb_free, EXT4_CLUSTERS_PER_GROUP(sb) - 2);
> +}
> +
> +static void test_mb_lg_list_trim(struct kunit *test)
> +{
> +	struct super_block *sb = (struct super_block *)test->priv;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	struct ext4_locality_group *lg;
> +	struct inode *inode;
> +	struct ext4_buddy e4b;
> +	struct ext4_prealloc_space *pa;
> +	int i, ret, count = 0;
> +	int order = fls(15) - 1; /* pa_free = 15 -> order 3 */
> +
> +	if (sb->s_blocksize > PAGE_SIZE)
> +		kunit_skip(test, "blocksize exceeds pagesize");
> +
> +	inode = mbt_alloc_test_inode(test, sb, S_IFREG);
> +	sbi->s_mb_group_prealloc = 16;
> +
> +	ret = ext4_mb_load_buddy_test(sb, TEST_GOAL_GROUP, &e4b);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	/*
> +	 * Create 9 locality-group PAs in bucket order 3 (each of length 16 with
> +	 * 1 cluster used -> pa_free = 15). When the 9th PA is added via
> +	 * ext4_mb_release_context -> ext4_mb_add_n_trim, lg_prealloc_count > 8
> +	 * triggers ext4_mb_discard_lg_preallocations() to trim the list to 5!
> +	 */
> +	for (i = 0; i < 9; i++) {
> +		struct ext4_allocation_context ac = {};
> +		struct ext4_free_extent ex = {
> +			.fe_group = TEST_GOAL_GROUP,
> +			.fe_start = i * 32,
> +			.fe_len = 16,
> +		};
> +
> +		ac.ac_sb = sb;
> +		ac.ac_inode = inode;
> +		ac.ac_flags = EXT4_MB_HINT_DATA | EXT4_MB_HINT_GROUP_ALLOC;
> +		ac.ac_lg = raw_cpu_ptr(sbi->s_locality_groups);
> +		mutex_lock(&ac.ac_lg->lg_mutex);
> +
> +		ret = ext4_mb_pa_alloc_test(&ac);
> +		if (ret) {
> +			mutex_unlock(&ac.ac_lg->lg_mutex);
> +			ext4_mb_unload_buddy_test(&e4b);
> +			KUNIT_ASSERT_EQ(test, ret, 0);
> +		}
> +
> +		ext4_lock_group(sb, TEST_GOAL_GROUP);
> +		mb_mark_used_test(&e4b, &ex);
> +		ext4_unlock_group(sb, TEST_GOAL_GROUP);
> +
> +		ac.ac_status = AC_STATUS_FOUND;
> +		ac.ac_o_ex.fe_len = 1;
> +		ac.ac_b_ex = ex;
> +		ac.ac_f_ex = ex;
> +		ext4_mb_new_preallocation_test(&ac);
> +		/* Release context updates pa_free to 15 and runs ext4_mb_add_n_trim */
> +		ext4_mb_release_context_test(&ac);
> +	}
> +
> +	ext4_mb_unload_buddy_test(&e4b);
> +
> +	lg = raw_cpu_ptr(sbi->s_locality_groups);
> +	spin_lock(&lg->lg_prealloc_lock);
> +	list_for_each_entry(pa, &lg->lg_prealloc_list[order], pa_node.lg_list)
> +		count++;
> +	spin_unlock(&lg->lg_prealloc_lock);
> +
> +	KUNIT_EXPECT_EQ(test, count, 5);
> +}

[Severity: Low]
Can CPU migration across sleeping calls cause test_mb_group_pa_lifecycle()
and test_mb_lg_list_trim() to use different per-CPU sbi->s_locality_groups
instances?

Both tests run in a preemptible KUnit kthread and call sleeping functions
(ext4_mb_new_blocks(), mutex_lock(), and ext4_mb_pa_alloc_test()).  If the
kthread migrates CPUs between the two ext4_mb_new_blocks() calls in
test_mb_group_pa_lifecycle(), the second call looks up the new CPU's
empty lg_prealloc_list instead of reusing the first PA.  Similarly,
test_mb_lg_list_trim() re-evaluates raw_cpu_ptr(sbi->s_locality_groups) on
each loop iteration and again when counting lg_prealloc_list[order], so a
migration splits the 9 PAs across per-CPU lists and fails the count == 5
check.

> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 0f73fb826dbf4..46ba07e017a31 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c

[ ... ]

> +void mb_regenerate_buddy_test(struct ext4_buddy *e4b)
> +{
> +	mb_regenerate_buddy(e4b);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(mb_regenerate_buddy_test);

[ ... ]

> +void ext4_mb_measure_extent_test(struct ext4_allocation_context *ac,
> +				 struct ext4_free_extent *ex,
> +				 struct ext4_buddy *e4b)
> +{
> +	ext4_mb_measure_extent(ac, ex, e4b);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_mb_measure_extent_test);

[ ... ]

> +bool ext4_mb_use_preallocated_test(struct ext4_allocation_context *ac)
> +{
> +	return ext4_mb_use_preallocated(ac);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_mb_use_preallocated_test);

[ ... ]

> +void ext4_mb_pa_put_free_test(struct ext4_allocation_context *ac)
> +{
> +	ext4_mb_pa_put_free(ac);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_mb_pa_put_free_test);
> +
> +void ext4_mb_generate_from_pa_test(struct super_block *sb, void *bitmap,
> +				   ext4_group_t group)
> +{
> +	ext4_mb_generate_from_pa(sb, bitmap, group);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_mb_generate_from_pa_test);
> +
> +int ext4_mb_discard_group_preallocations_test(struct super_block *sb,
> +					      ext4_group_t group, int *busy)
> +{
> +	return ext4_mb_discard_group_preallocations(sb, group, busy);
> +}
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_mb_discard_group_preallocations_test);

[Severity: Low]
This isn't a bug, but mb_regenerate_buddy_test(),
ext4_mb_measure_extent_test(), ext4_mb_use_preallocated_test(),
ext4_mb_pa_put_free_test(), and
ext4_mb_discard_group_preallocations_test() have no callers in
fs/ext4/mballoc-test.c.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112040638.1576836-1-tytso@mit.edu?part=6