Re: [PATCH] f2fs: fix node_cnt race between extent node destroy and writeback

[Chao Yu <chao@kernel.org>]

On 4/3/26 22:40, Yongpeng Yang wrote:
> From: Yongpeng Yang <yangyongpeng@xiaomi.com>
> 
> f2fs_destroy_extent_node() does not set FI_NO_EXTENT before clearing
> extent nodes. When called from f2fs_drop_inode() with I_SYNC set,
> concurrent kworker writeback can insert new extent nodes into the same
> extent tree, racing with the destroy and triggering f2fs_bug_on() in
> __destroy_extent_node(). The scenario is as follows:
> 
> drop inode                            writeback
>   - iput
>    - f2fs_drop_inode  // I_SYNC set
>     - f2fs_destroy_extent_node
>      - __destroy_extent_node
>       - while (node_cnt) {
>          write_lock(&et->lock)
>          __free_extent_tree
>          write_unlock(&et->lock)
>                                         - __writeback_single_inode
>                                          - f2fs_outplace_write_data
>                                           - f2fs_update_read_extent_cache
>                                            - __update_extent_tree_range
>                                             // FI_NO_EXTENT not set,
>                                             // insert new extent node
>         } // node_cnt == 0, exit while
>       - f2fs_bug_on(node_cnt)  // node_cnt > 0
> 
> Additionally, __update_extent_tree_range() only checks FI_NO_EXTENT for
> EX_READ type, leaving EX_BLOCK_AGE updates completely unprotected.
> 
> This patch set FI_NO_EXTENT under et->lock in __destroy_extent_node(),
> consistent with other callers (__update_extent_tree_range and
> __drop_extent_tree) and check FI_NO_EXTENT for both EX_READ and
> EX_BLOCK_AGE tree.

I suffered below test failure, then I bisect to this change.

     generic/475  84s ... [failed, exit status 1]- output mismatch (see /share/git/fstests/results//generic/475.out.bad)
     --- tests/generic/475.out   2025-01-12 21:57:40.279440664 +0800
     +++ /share/git/fstests/results//generic/475.out.bad 2026-04-17 12:08:28.000000000 +0800
     @@ -1,2 +1,6 @@
      QA output created by 475
      Silence is golden.
     +mount: /mnt/scratch_f2fs: mount system call failed: Structure needs cleaning.
     +       dmesg(1) may have more information after failed mount system call.
     +mount failed
     +(see /share/git/fstests/results//generic/475.full for details)
     ...
     (Run 'diff -u /share/git/fstests/tests/generic/475.out /share/git/fstests/results//generic/475.out.bad'  to see the entire diff)


     generic/388  73s ... [failed, exit status 1]- output mismatch (see /share/git/fstests/results//generic/388.out.bad)
     --- tests/generic/388.out   2025-01-12 21:57:40.275440602 +0800
     +++ /share/git/fstests/results//generic/388.out.bad 2026-04-17 11:58:05.000000000 +0800
     @@ -1,2 +1,6 @@
      QA output created by 388
      Silence is golden.
     +mount: /mnt/scratch_f2fs: mount system call failed: Structure needs cleaning.
     +       dmesg(1) may have more information after failed mount system call.
     +cycle mount failed
     +(see /share/git/fstests/results//generic/388.full for details)
     ...
     (Run 'diff -u /share/git/fstests/tests/generic/388.out /share/git/fstests/results//generic/388.out.bad'  to see the entire diff)


     F2FS-fs (dm-0): sanity_check_extent_cache: inode (ino=1761) extent info [220057, 57, 6] is incorrect, run fsck to fix

I suspect we may miss any extent updates after we set FI_NO_EXTENT in
__destroy_extent_node(), result in failing in sanity_check_extent_cache().

Can we just relocate f2fs_bug_on(node_cnt) rather than complicated change?
Thoughts?

Thanks,

> 
> Fixes: 3fc5d5a182f6 ("f2fs: fix to shrink read extent node in batches")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yongpeng Yang <yangyongpeng@xiaomi.com>
> ---
>   fs/f2fs/extent_cache.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 0ed84cc065a7..87169fd29d89 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -119,9 +119,10 @@ static bool __may_extent_tree(struct inode *inode, enum extent_type type)
>   	if (!__init_may_extent_tree(inode, type))
>   		return false;
>   
> +	if (is_inode_flag_set(inode, FI_NO_EXTENT))
> +		return false;
> +
>   	if (type == EX_READ) {
> -		if (is_inode_flag_set(inode, FI_NO_EXTENT))
> -			return false;
>   		if (is_inode_flag_set(inode, FI_COMPRESSED_FILE) &&
>   				 !f2fs_sb_has_readonly(F2FS_I_SB(inode)))
>   			return false;
> @@ -644,6 +645,8 @@ static unsigned int __destroy_extent_node(struct inode *inode,
>   
>   	while (atomic_read(&et->node_cnt)) {
>   		write_lock(&et->lock);
> +		if (!is_inode_flag_set(inode, FI_NO_EXTENT))
> +			set_inode_flag(inode, FI_NO_EXTENT);
>   		node_cnt += __free_extent_tree(sbi, et, nr_shrink);
>   		write_unlock(&et->lock);
>   	}
> @@ -688,12 +691,12 @@ static void __update_extent_tree_range(struct inode *inode,
>   
>   	write_lock(&et->lock);
>   
> -	if (type == EX_READ) {
> -		if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> -			write_unlock(&et->lock);
> -			return;
> -		}
> +	if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> +		write_unlock(&et->lock);
> +		return;
> +	}
>   
> +	if (type == EX_READ) {
>   		prev = et->largest;
>   		dei.len = 0;
>