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

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