Re: [f2fs-dev] [PATCH] f2fs: fix incorrect FI_NO_EXTENT handling in __destroy_extent_node()

[Yongpeng Yang <monty_pavel@sina.com>]

On 4/27/26 15:38, Chao Yu via Linux-f2fs-devel wrote:
> On 4/24/26 17:45, Yongpeng Yang wrote:
>>
>> On 4/22/26 20:33, Chao Yu via Linux-f2fs-devel wrote:
>>> On 4/22/2026 3:35 PM, Yongpeng Yang wrote:
>>>> From: yangyongpeng <yangyongpeng@xiaomi.com>
>>>>
>>>> When __destroy_extent_node() sets the inode flag FI_NO_EXTENT, it does
>>>> not reset the length of the largest extent to 0 and update the inode
>>>> folio. Since modifications to the extent tree are disallowed afterward,
>>>> the cached largest extent may become stale. This can trigger the
>>>> following error in xfstests generic/388:
>>>>
>>>> F2FS-fs (dm-0): sanity_check_extent_cache: inode (ino=1761) extent
>>>> info [220057, 57, 6] is incorrect, run fsck to fix
>>>>
>>>> In the f2fs_drop_inode path, __destroy_extent_node() does not need to
>>>> guarantee that et->node_cnt is 0, because concurrency with writeback
>>>> is expected in this path, and writeback may update the extent cache.
>>>>
>>>> This patch updates __destroy_extent_node() to avoid setting the inode
>>>> flag FI_NO_EXTENT, and to remove the check zero of et->node_cnt.
>>>>
>>>> Fixes: ed78aeebef05 ("f2fs: fix node_cnt race between extent node
>>>> destroy and writeback")
>>>> Cc: stable@vger.kernel.org
>>>> Reported-by: Chao Yu <chao@kernel.org>
>>>> Suggested-by: Chao Yu <chao@kernel.org>
>>>> Signed-off-by: yangyongpeng <yangyongpeng@xiaomi.com>
>>>> ---
>>>>    fs/f2fs/extent_cache.c | 4 ----
>>>>    1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>>> index 87169fd29d89..3adbead27953 100644
>>>> --- a/fs/f2fs/extent_cache.c
>>>> +++ b/fs/f2fs/extent_cache.c
>>>> @@ -645,14 +645,10 @@ 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);
>>>
>>> We'd better revert all change lines in "f2fs: fix node_cnt race between
>>> extent node destroy and writeback"?
>>
>> The others all check whether FI_NO_EXTENT is set. When it is set,
>> inserting an age extent is disallowed, so nothing was removed.
> 
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 87169fd29d89..0ed84cc065a7 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -119,10 +119,9 @@ 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;

This should be revert. The EX_BLOCK_AGE extent tree type was added later
and has never been governed by FI_NO_EXTENT. After reverting the commit
ed78aeebef05, having FI_NO_EXTENT set no longer implies that the inode
needs to be evicted, so rejecting updates to EX_BLOCK_AGE extents based
on this flag no longer makes sense. Therefore, this part of the change
should be dropped.

>                 if (is_inode_flag_set(inode, FI_COMPRESSED_FILE) &&
>                                  !f2fs_sb_has_readonly(F2FS_I_SB(inode)))
>                         return false;
> 
> ...
> 
> @@ -691,12 +688,12 @@ static void __update_extent_tree_range(struct
> inode *inode,
> 
>         write_lock(&et->lock);
> 
> -       if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> -               write_unlock(&et->lock);
> -               return;
> -       }

This also should be revert. All callers of this function already invoke
__may_extent_tree() to verify whether FI_NO_EXTENT is set. So, this
check are dead code.

> -
>         if (type == EX_READ) {
> +               if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> +                       write_unlock(&et->lock);
> +                       return;
> +               }
> +
>                 prev = et->largest;
>                 dei.len = 0;
> 
> Hmm, I'm not sure I understood you correctly, if you want to keep above
> codes, what
> about changing in another patch w/ correct commit message?

Yes, I mean that, but I was mistaken. I'll fix above issue in v2 patch.

Thanks
Yongpeng,

> 
> Thanks,
> 
> 
>>
>> Thanks
>> Yongpeng,
>>
>>>
>>> Thanks,
>>>
>>>>            node_cnt += __free_extent_tree(sbi, et, nr_shrink);
>>>>            write_unlock(&et->lock);
>>>>        }
>>>>    -    f2fs_bug_on(sbi, atomic_read(&et->node_cnt));
>>>> -
>>>>        return node_cnt;
>>>>    }
>>>>    
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel