Re: [f2fs-dev] [PATCH RESEND 2/5] f2fs: only initialize largest extent without extent_node at inode init

On 6/25/26 15:25, Chao Yu via Linux-f2fs-devel wrote:
> On 6/23/26 17:29, Yongpeng Yang wrote:
>>
>> On 6/22/26 08:42, Chao Yu via Linux-f2fs-devel wrote:
>>> On 6/21/26 23:48, Yongpeng Yang wrote:
>>>> On 6/20/26 11:31 AM, Chao Yu via Linux-f2fs-devel wrote:
>>>>> On 6/19/26 22:34, Yongpeng Yang wrote:
>>>>>>
>>>>>> On 6/15/26 7:55 PM, Chao Yu via Linux-f2fs-devel wrote:
>>>>>>> On 6/12/26 19:58, Yongpeng Yang wrote:
>>>>>>>> From: Yongpeng Yang <[email protected]>
>>>>>>>>
>>>>>>>> The largest extent takes effect during both read mapping and write
>>>>>>>> mapping lookups, while read mapping does not need to access the
>>>>>>>> extent_node. For write mapping, the case where the largest extent is
>>>>>>>> not in the extent tree can already be handled by the merge logic, and
>>>>>>>> cases that cannot be merged do not require the largest extent to
>>>>>>>> participate either.
>>>>>>>>
>>>>>>>> Therefore, the largest extent does not need to initialize a
>>>>>>>> corresponding extent_node, reducing memory footprint.
>>>>>>>>
>>>>>>>> Signed-off-by: Yongpeng Yang <[email protected]>
>>>>>>>> ---
>>>>>>>>   fs/f2fs/extent_cache.c | 18 +-----------------
>>>>>>>>   1 file changed, 1 insertion(+), 17 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>>>>>>> index aa368a01b035..f8d94db60dc6 100644
>>>>>>>> --- a/fs/f2fs/extent_cache.c
>>>>>>>> +++ b/fs/f2fs/extent_cache.c
>>>>>>>> @@ -410,10 +410,8 @@ static void __drop_largest_extent(struct
>>>>>>>> extent_tree *et,
>>>>>>>>   void f2fs_init_read_extent_tree(struct inode *inode, struct
>>>>>>>> folio *ifolio)
>>>>>>>>   {
>>>>>>>>       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>>>>>> -    struct extent_tree_info *eti = &sbi->extent_tree[EX_READ];
>>>>>>>>       struct f2fs_extent *i_ext = &F2FS_INODE(ifolio)->i_ext;
>>>>>>>>       struct extent_tree *et;
>>>>>>>> -    struct extent_node *en;
>>>>>>>>       struct extent_info ei = {0};
>>>>>>>>       if (!__may_extent_tree(inode, EX_READ)) {
>>>>>>>> @@ -435,21 +433,7 @@ void f2fs_init_read_extent_tree(struct inode
>>>>>>>> *inode, struct folio *ifolio)
>>>>>>>>       if (atomic_read(&et->node_cnt) || !ei.len)
>>>>>>>>           goto skip;
>>>>>>>> -    if (IS_DEVICE_ALIASING(inode)) {
>>>>>>>> -        et->largest = ei;
>>>>>>>> -        goto skip;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>> -    en = __attach_extent_node(sbi, et, &ei, NULL,
>>>>>>>> -                &et->root.rb_root.rb_node, true);
>>>>>>>> -    if (en) {
>>>>>>>> -        et->largest = en->ei;
>>>>>>>> -        et->cached_en = en;
>>>>>>>> -
>>>>>>>> -        spin_lock(&eti->extent_lock);
>>>>>>>> -        list_add_tail(&en->list, &eti->extent_list);
>>>>>>>> -        spin_unlock(&eti->extent_lock);
>>>>>>>> -    }
>>>>>>>> +    et->largest = ei;
>>>>>>>
>>>>>>> Previously, we can split largest extent node to two if we punched
>>>>>>> it, now
>>>>>>> we can not? IIUC.
>>>>>>
>>>>>> Prior to this change, the largest extent could also be shrunk, so the
>>>>>> set of scenarios that need handling during punch remains identical
>>>>>> before and after the modification.
>>>>>>
>>>>>> For the largest extent, it only needs to guarantee that the mapping
>>>>>> information it records stays consistent with the mappings tracked in
>>>>>> the
>>>>>> extent tree and multi-level indirect indices. The punch operation does
>>>>>> not break this consistency. Moreover, the largest extent is not
>>>>>> required
>>>>>> to be the longest extent among all entries in the extent tree. It
>>>>>> merely
>>>>>> needs to match the mappings stored in multi-level indirect indices and
>>>>>> be no smaller than the maximum extent present in the extent tree.
>>>>>
>>>>> What I meant is: e.g. previously, if largest extent is 1024, punching
>>>>> in the
>>>>> middle of the extent will make it being split to two extents (512,
>>>>> 511) in
>>>>> __update_extent_tree_range().
>>>>>
>>>>> If we do not add largest extent node into rb tree in
>>>>> f2fs_init_read_extent_tree(),
>>>>> then we may has no chance to keep small-sized(511) extent in above
>>>>> punch scenario,
>>>>> can you verify that?
>>>>
>>>> Yes, the 511-block mapping will be dropped. I considered this case
>>>> earlier: when the newly inserted mapping overlaps with or adjacent
>>>> with the largest extent, we reinsert the largest extent back into the
>>>> extent tree. With this logic, the 511-block split extent will remain
>>>> present in the extent tree. Instead of performing punch handling on
>>>> the largest extent, we can simply drop it directly.
>>>> However, this approach does not work well for updates targeting the
>>>> tail of the original extent. For instance, punch 1024 into 1022 and 1
>>>> for overwrite case, which will also alloc one more extent_node.
>>>
>>> We will only add extent which has size >= F2FS_MIN_EXTENT_LEN, so 1022
>>> will be added, and 1 will be dropped.
>>
>> Oh, yes, this example is not appropriate. Do you think this approach
> 
> Since there will be potential side-effect w/ current patch, so I prefer
> to keep it as it is.

We have observed that the "Hit ratio" reported by "Extent Cache (Read)"
is quite low. The main reason is that the total number of extent nodes
in the read extent cache is limited, but increasing that limit may not
be easy.

I'm wondering whether we can improve cache utilization instead. Adding
an extent_node for the largest extent and inserting it into the read
extent tree does not seem to help the hit ratio directly. On the other
hand, when a file is reopened, its largest extent causes the
corresponding extent_node to be added to the tail of
sbi->extent_tree[EX_READ].extent_list. Under memory pressure, this may
result in other non-largest extents being reclaimed first.

Would it make sense to place the largest extent_node at the head of
sbi-> extent_tree[EX_READ].extent_list when restoring the largest extent
in f2fs_init_read_extent_tree() instead? This seems like a simpler
approach to improve the utilization of the existing read extent cache.

Thanks
Yongpeng,

> 
> Thanks,
> 
>> would be better, or is [PATCH 1/5] and [PATCH 3/5] preferable?
>>
>> Thanks
>> Yongpeng,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>> Thanks
>>>> Yongpeng,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Yongpeng,
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>   skip:
>>>>>>>>       /* Let's drop, if checkpoint got corrupted. */
>>>>>>>>       if (f2fs_cp_error(sbi)) {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Linux-f2fs-devel mailing list
>>>>>>> [email protected]
>>>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> [email protected]
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> [email protected]
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel