Re: [PATCH 01/27] ext4: add KUinit tests for fs/ext4/extents_status.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] ext4: data race in __es_shrink() [1]
--

commit 45574cf43b458dd12781bdd04f9e4dfbf9ef71c4
Author: Theodore Ts'o <tytso@mit.edu>

ext4: add KUinit tests for fs/ext4/extents_status.c

This commit adds KUnit tests for the ext4 extent status tree and bigalloc
pending reservations, and exports the required symbols for the test module.

> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 6e4a191e82191..64bac0c4c1fa0 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -2390,3 +2390,22 @@ static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
>  out:
>  	return (ret < 0) ? ret : pendings;
>  }
> +
> +#if IS_ENABLED(CONFIG_EXT4_KUNIT_TESTS)
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_init_tree);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_extent);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_cache_extent);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_remove_extent);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_find_extent_range);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_lookup_extent);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_range);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_clu);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_register_shrinker);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_unregister_shrinker);
> +EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_seq_es_shrinker_info_show);

[Severity: Medium]
This problem wasn't introduced by this patch, but can concurrent shrinker
runs race on the 64-bit statistics fields in sbi->s_es_stats [1]?

fs/ext4/extents_status.c:__es_shrink() {
    ...
	spin_unlock(&sbi->s_es_lock);
    ...
out:
	scan_time = ktime_to_ns(ktime_sub(ktime_get(), start_time));
	if (likely(es_stats->es_stats_scan_time))
		es_stats->es_stats_scan_time = (scan_time +
				es_stats->es_stats_scan_time*3) / 4;
	else
		es_stats->es_stats_scan_time = scan_time;
	if (scan_time > es_stats->es_stats_max_scan_time)
		es_stats->es_stats_max_scan_time = scan_time;
	if (likely(es_stats->es_stats_shrunk))
		es_stats->es_stats_shrunk = (nr_shrunk +
				es_stats->es_stats_shrunk*3) / 4;
	else
		es_stats->es_stats_shrunk = nr_shrunk;
}

__es_shrink() drops sbi->s_es_lock before reaching the out label, so
concurrent memory reclaim threads can update es_stats_scan_time,
es_stats_max_scan_time, and es_stats_shrunk simultaneously without
synchronization.  On 32-bit architectures, these plain 64-bit read-modify-write
accesses can tear and corrupt the shrinker metrics.

[1] https://sashiko.dev/#/bug/linux-ffbda43a-8d25-4cbd-be48-64c4f05ffa5f

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