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

[Chao Yu <chao@kernel.org>]

On 4/17/26 21:26, Yongpeng Yang wrote:
> 
> On 4/17/26 17:00, Chao Yu via Linux-f2fs-devel wrote:
>> 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?
> 
> Oh, I overlooked largest extent. How about relocate
> f2fs_bug_on(node_cnt) to __destroy_extent_tree?
> 
> static void __destroy_extent_tree(struct inode *inode, enum extent_type
> type)
> 
>          /* free all extent info belong to this extent tree */
>          node_cnt = __destroy_extent_node(inode, type);
> +       f2fs_bug_on(sbi, atomic_read(&et->node_cnt));

	/* free all extent info belong to this extent tree */
	node_cnt = __destroy_extent_node(inode, type);

	/* delete extent tree entry in radix tree */
	mutex_lock(&eti->extent_tree_lock);
	f2fs_bug_on(sbi, atomic_read(&et->node_cnt));  <---

Oh, it has already checked node_cnt, so, maybe we can just remove the check in
__destroy_extent_node()?

Thanks,


> 
> Thanks
> Yongpeng,
> 
>>
>> 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;
>>>    
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>