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

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 04/21, Chao Yu wrote:
> 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?

Yes, I just sent a pull request. If the merge patch doesn't break anything,
let's apply a new one only.

> 
> 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;
> > > > > > > 
> > > > > > > 
> > > > > > > 
> >