Re: [f2fs-dev] [PATCH RESEND 3/5] f2fs: punch largest extent instead of dropping it entirely on overlap

On 6/25/26 15:49, Chao Yu via Linux-f2fs-devel wrote:
> On 6/23/26 17:31, Yongpeng Yang wrote:
>>
>> On 6/22/26 08:45, Chao Yu via Linux-f2fs-devel wrote:
>>> On 6/22/26 00:07, Yongpeng Yang wrote:
>>>>
>>>> On 6/20/26 2:15 PM, Chao Yu via Linux-f2fs-devel wrote:
>>>>> On 6/19/26 22:38, Yongpeng Yang wrote:
>>>>>>
>>>>>> On 6/15/26 8:05 PM, Chao Yu via Linux-f2fs-devel wrote:
>>>>>>> On 6/12/26 19:58, Yongpeng Yang wrote:
>>>>>>>> From: Yongpeng Yang <[email protected]>
>>>>>>>>
>>>>>>>> Previously, when an extent being inserted overlaps with the largest
>>>>>>>> extent, the largest extent is dropped entirely. This was done to
>>>>>>>> handle
>>>>>>>
>>>>>>> Please correct me if I missed anything, I remember that we will add
>>>>>>> largest
>>>>>>> extent in below path?
>>>>>>>
>>>>>>> - __update_extent_tree_range
>>>>>>>   - __insert_extent_tree
>>>>>>>    - __try_update_largest_extent : update largest w/ right extent
>>>>>>>   - __try_update_largest_extent : update largest w/ left extent
>>>>>>
>>>>>> The largest extent might not reside in the extent tree. If
>>>>>> __update_extent_tree_range invokes __drop_largest_extent, the length of
>>>>>
>>>>> I meant __update_extent_tree_range invokes __drop_largest_extent to drop
>>>>> largest first, and then, it tries to update largest extent w/ larger-
>>>>> size
>>>>> one of separated two extents.
>>>>>
>>>>>> the largest extent will have been set to zero. As a result, the largest
>>>>>> extent updated inside __try_update_largest_extent can end up smaller
>>>>>> than the largest extent obtained after __punch_largest_extent completes
>>>>>> its punch operation.
>>>>>
>>>>> I didn't get it, can you give an example for this?
>>>>
>>>> The only distinction between punch and drop arises when the largest
>>>> extent is not present in the extent tree.
>>>>
>>>> Before this patch(extent format [fofs, len, blk])
>>>> 1. inital state
>>>>      largest extent: [0, 1024, 10], extent tree: empty
>>>
>>>                                                    ^^^^^
>>>
>>> It's not empty w/o patch 2/5.
>>
>> Sorry for the confusion. "empty" means that the largest extent has been
>> shrunk.
> 
> Okay,
> 
> Hmm...I found it's hard to reproduce such condition "only shrink extent slab
> cache and keep inode slab cache", so I wonder how common this issue will be
> in product.
> 
> Have you suffered real issues in product? or just insight from LLM or code
> review?

I was able to reproduce cases where the largest extent was shrunk, and
then a 4K overwrite on that largest extent caused it to be dropped. The
test scenario is roughly as follows, although the reproduction is not
100% reliable:

1. Construct a largest extent, then perform sparse writes to other
regions so that the largest extent is neither updated nor accessed.
2. Keep a background thread continuously opening the file to prevent the
inode from being shrunk.
3. Trigger memory reclaim.
4. Perform a 4K overwrite within the largest extent.

I have not observed this in a production environment. It was identified
through code review while investigating the fix for [PATCH 1/5], and
then verified with the above test case.

Thanks
Yongpeng,

> 
> Thanks,
> 
>>
>> Thanks
>> Yongpeng,
>>
>>>
>>> Thanks,
>>>
>>>> 2. insert [511, 1, 10000]
>>>>      largest extent: [511, 1, 10000], extent tree: [511, 1, 10000]
>>>>
>>>> After this patch:
>>>> 1. inital state
>>>>      largest extent: [0, 1024, 10], extent tree: empty
>>>> 2. insert [511, 1, 10000]
>>>>      largest extent: [512, 512, 522], extent tree: [511, 1, 10000]
>>>>
>>>> Thanks
>>>> Yongpeng,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Yongpeng,
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>> the case where the largest extent is not in memory, avoiding
>>>>>>>> inconsistency between the largest extent and the extent tree.
>>>>>>>>
>>>>>>>> This patch changes the semantics of __drop_largest_extent (renamed to
>>>>>>>> __punch_largest_extent): instead of discarding the entire largest
>>>>>>>> extent when any overlap is detected, keep the larger remaining
>>>>>>>> portion
>>>>>>>> (left or right) after the punch. This preserves extent cache coverage
>>>>>>>> for truncate and overwrite operations that only partially overlap the
>>>>>>>> largest extent.
>>>>>>>>
>>>>>>>> Signed-off-by: Yongpeng Yang <[email protected]>
>>>>>>>> ---
>>>>>>>>   fs/f2fs/extent_cache.c | 31 ++++++++++++++++++++++++-------
>>>>>>>>   1 file changed, 24 insertions(+), 7 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>>>>>>> index f8d94db60dc6..82d84c4e98b2 100644
>>>>>>>> --- a/fs/f2fs/extent_cache.c
>>>>>>>> +++ b/fs/f2fs/extent_cache.c
>>>>>>>> @@ -397,14 +397,31 @@ static unsigned int
>>>>>>>> __free_extent_tree(struct f2fs_sb_info *sbi,
>>>>>>>>       return count;
>>>>>>>>   }
>>>>>>>> -static void __drop_largest_extent(struct extent_tree *et,
>>>>>>>> +static void __punch_largest_extent(struct extent_tree *et,
>>>>>>>>                       pgoff_t fofs, unsigned int len)
>>>>>>>>   {
>>>>>>>> -    if (fofs < (pgoff_t)et->largest.fofs + et->largest.len &&
>>>>>>>> -            fofs + len > et->largest.fofs) {
>>>>>>>> -        et->largest.len = 0;
>>>>>>>> -        et->largest_updated = true;
>>>>>>>> +    unsigned int largest_end, punch_end;
>>>>>>>> +    unsigned int left_len, right_len;
>>>>>>>> +
>>>>>>>> +    if (fofs >= (pgoff_t)et->largest.fofs + et->largest.len ||
>>>>>>>> +            fofs + len <= et->largest.fofs)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    /* Punch [fofs, fofs + len) from largest extent. */
>>>>>>>> +    largest_end = et->largest.fofs + et->largest.len;
>>>>>>>> +    punch_end = fofs + len;
>>>>>>>> +
>>>>>>>> +    left_len = fofs > et->largest.fofs ? fofs - et-
>>>>>>>>> largest.fofs : 0;
>>>>>>>> +    right_len = largest_end > punch_end ? largest_end -
>>>>>>>> punch_end : 0;
>>>>>>>> +
>>>>>>>> +    if (left_len >= right_len) {
>>>>>>>> +        et->largest.len = left_len;
>>>>>>>> +    } else {
>>>>>>>> +        et->largest.blk += punch_end - et->largest.fofs;
>>>>>>>> +        et->largest.fofs = punch_end;
>>>>>>>> +        et->largest.len = right_len;
>>>>>>>>       }
>>>>>>>> +    et->largest_updated = true;
>>>>>>>>   }
>>>>>>>>   void f2fs_init_read_extent_tree(struct inode *inode, struct
>>>>>>>> folio *ifolio)
>>>>>>>> @@ -680,10 +697,10 @@ static void
>>>>>>>> __update_extent_tree_range(struct inode *inode,
>>>>>>>>           dei.len = 0;
>>>>>>>>           /*
>>>>>>>> -         * drop largest extent before lookup, in case it's already
>>>>>>>> +         * punch largest extent before lookup, in case it's already
>>>>>>>>            * been shrunk from extent tree
>>>>>>>>            */
>>>>>>>> -        __drop_largest_extent(et, fofs, len);
>>>>>>>> +        __punch_largest_extent(et, fofs, len);
>>>>>>>>       }
>>>>>>>>       if (et->largest.len != 0 &&
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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