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

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.

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