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

[Chao Yu <chao@kernel.org>]

On 4/21/2026 4:44 PM, Yongpeng Yang wrote:
> 
> On 4/20/26 15:28, Chao Yu via Linux-f2fs-devel wrote:
>> On 4/19/2026 12:29 AM, Yongpeng Yang wrote:
>>>
>>> On 4/18/26 8:51 AM, Chao Yu via Linux-f2fs-devel wrote:
>>>> 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()?
>>>
>>> Yes. BTW, is it correct to remove the call to f2fs_destroy_extent_node()
>>> in f2fs_drop_inode()? It seems this call is unnecessary, since
>>> f2fs_evict_inode() will eventually delete all extent nodes properly.
>>
>> I think it's fine to keep it according to original intention "destroy
>> extent_tree for the truncation case" introduced from 3e72f721390d
>> ("f2fs: use extent_cache by default"). It helps the performance w/
>> in batch extent node release.
> 
> Oh, I see. This patch has already been merged into the dev branch. Which
> of the following approaches would be more appropriate?
> 1. Drop the current patch from the dev branch, then submit a patch to
> remove the f2fs_bug_on() in __destroy_extent_node.
> 2. Send two patches: the first reverts the change, and the second
> removes the f2fs_bug_on() in __destroy_extent_node().

It's near the end of merge window, I think we need to keep dev as
it is, and create another patch to revert previous change and drop
the f2fs_bug_on() as well, what do you think?

To Jaegeuk, thoughts?

Thanks,

> 
> Thanks
> Yongpeng,
> 
>>
>> Thanks,
>>
>>>
>>> Thanks
>>> Yongpeng,
>>>
>>>>
>>>> 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;
>>>>>>
>>>>>>
>>>>>>
>