Re: [f2fs-dev] [PATCH v2 2/2] f2fs: introduce get_section_mtime

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 2024/9/19 10:23, Zhiguo Niu wrote:
> Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>
> 于2024年9月18日周三 14:45写道：
>>
>> On 2024/9/12 14:40, liuderong@oppo.com wrote:
>>> From: liuderong <liuderong@oppo.com>
>>>
>>> When segs_per_sec is larger than 1, section may contain free segments,
>>> mtime should be the mean value of each valid segments,
>>> so introduce get_section_mtime to exclude free segments in a section.
>>>
>>> Signed-off-by: liuderong <liuderong@oppo.com>
>>> ---
>>>    fs/f2fs/f2fs.h    |  2 ++
>>>    fs/f2fs/gc.c      | 15 ++-------------
>>>    fs/f2fs/segment.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>>    3 files changed, 40 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 4dcdcdd..d6adf0f 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -3762,6 +3762,8 @@ enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi,
>>>    unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi);
>>>    unsigned int f2fs_usable_blks_in_seg(struct f2fs_sb_info *sbi,
>>>                        unsigned int segno);
>>> +unsigned long long get_section_mtime(struct f2fs_sb_info *sbi,
>>> +                     unsigned int segno);
>>
>> Hi Derong,
>>
>> It needs to add "f2fs_" prefix for get_section_mtime() to avoid global
>> namespace pollution.
>>
>>>
>>>    #define DEF_FRAGMENT_SIZE   4
>>>    #define MIN_FRAGMENT_SIZE   1
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 6299639..03c6117 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -332,20 +332,14 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
>>>    static unsigned int get_cb_cost(struct f2fs_sb_info *sbi, unsigned int segno)
>>>    {
>>>        struct sit_info *sit_i = SIT_I(sbi);
>>> -     unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>> -     unsigned int start = GET_SEG_FROM_SEC(sbi, secno);
>>>        unsigned long long mtime = 0;
>>>        unsigned int vblocks;
>>>        unsigned char age = 0;
>>>        unsigned char u;
>>> -     unsigned int i;
>>>        unsigned int usable_segs_per_sec = f2fs_usable_segs_in_sec(sbi);
>>>
>>> -     for (i = 0; i < usable_segs_per_sec; i++)
>>> -             mtime += get_seg_entry(sbi, start + i)->mtime;
>>> +     mtime = get_section_mtime(sbi, segno);
>>>        vblocks = get_valid_blocks(sbi, segno, true);
>>> -
>>> -     mtime = div_u64(mtime, usable_segs_per_sec);
>>>        vblocks = div_u64(vblocks, usable_segs_per_sec);
>>>
>>>        u = BLKS_TO_SEGS(sbi, vblocks * 100);
>>> @@ -485,10 +479,7 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
>>>                                struct victim_sel_policy *p, unsigned int segno)
>>>    {
>>>        struct sit_info *sit_i = SIT_I(sbi);
>>> -     unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>> -     unsigned int start = GET_SEG_FROM_SEC(sbi, secno);
>>>        unsigned long long mtime = 0;
>>> -     unsigned int i;
>>>
>>>        if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
>>>                if (p->gc_mode == GC_AT &&
>>> @@ -496,9 +487,7 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
>>>                        return;
>>>        }
>>>
>>> -     for (i = 0; i < SEGS_PER_SEC(sbi); i++)
>>> -             mtime += get_seg_entry(sbi, start + i)->mtime;
>>> -     mtime = div_u64(mtime, SEGS_PER_SEC(sbi));
>>> +     mtime = get_section_mtime(sbi, segno);
>>>
>>>        /* Handle if the system time has changed by the user */
>>>        if (mtime < sit_i->min_mtime)
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 6627394..e62e722 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -5389,6 +5389,41 @@ unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi)
>>>        return SEGS_PER_SEC(sbi);
>>>    }
>>>
>>> +unsigned long long get_section_mtime(struct f2fs_sb_info *sbi,
>>> +     unsigned int segno)
>>> +{
>>> +     unsigned int usable_segs_per_sec = f2fs_usable_segs_in_sec(sbi);
>>> +     unsigned int secno = 0, start = 0;
>>> +     struct free_segmap_info *free_i = FREE_I(sbi);
>>> +     unsigned int valid_seg_count = 0;
>>> +     unsigned long long mtime = 0;
>>> +     unsigned int i = 0;
>>> +
>>> +     if (segno == NULL_SEGNO)
>>> +             return 0;
>>
>> No needed.
>>
>>> +
>>> +     secno = GET_SEC_FROM_SEG(sbi, segno);
>>> +     start = GET_SEG_FROM_SEC(sbi, secno);
>>> +
>>> +     if (!__is_large_section(sbi))
>>> +             return get_seg_entry(sbi, start + i)->mtime;
>>> +
>>> +     for (i = 0; i < usable_segs_per_sec; i++) {
>>> +             /* for large section, only check the mtime of valid segments */
>>> +             spin_lock(&free_i->segmap_lock);
>>> +             if (test_bit(start + i, free_i->free_segmap)) {
>>> +                     mtime += get_seg_entry(sbi, start + i)->mtime;
>>> +                     valid_seg_count++;
>>> +             }
>>> +             spin_unlock(&free_i->segmap_lock);
>>> +     }
>>
>> After commit 6f3a01ae9b72 ("f2fs: record average update time of segment"),
>> mtime of segment starts to indicate average update time of segment.
>>
>> So it needs to change like this?
>>
>> for (i = 0; i < usable_segs_per_sec; i++) {
>>          struct seg_entry *se = get_seg_entry(sbi, start + i);
>>
>>          mtime += se->mtime * se->valid_blocks;
>>          total_valid_blocks += se->valid_blocks;
>> }
> hi Chao,
> after I read this patch from Derong and base on your this comment,
> I have some doubts：
> mtime is update in update_segment_mtime, and this API is called by
> more than one path， such as f2fs_invalidate_blocks and f2fs_allocate_data_block,
> and se->mtime is calculated by the following  flow if se->mtime is not null.
> --------------------------------
> se->mtime = div_u64(se->mtime * se->valid_blocks + mtime,
> se->valid_blocks + 1);
> --------------------------------
> if this is called from f2fs_invalidate_blocks, se->mtime is still calculated by
> mtime / se->valid_blocks + 1, but the real value of se->valid_blocks will
> will be reduced  1，So isn’t it a bit inaccurate just calculating valid
> blocks in this patch?

When target block was invalidated, I want to superpose its mtime into
average mtime of segment, so that segment mtime can indicate there are
still updates on this segment, and the segment will not become too elder
to be selected by BGGC, so, I used that calculation method.

Thanks,

> thanks!
>>
>> if (total_valid_blocks == 0)
>>          return 0;
>>
>> return div_u64(mtime, total_valid_blocks);
>>
>> Thanks,
>>
>>> +
>>> +     if (valid_seg_count == 0)
>>> +             return 0;
>>> +
>>> +     return div_u64(mtime, valid_seg_count);
>>> +}
>>> +
>>>    /*
>>>     * Update min, max modified time for cost-benefit GC algorithm
>>>     */
>>> @@ -5402,13 +5437,9 @@ static void init_min_max_mtime(struct f2fs_sb_info *sbi)
>>>        sit_i->min_mtime = ULLONG_MAX;
>>>
>>>        for (segno = 0; segno < MAIN_SEGS(sbi); segno += SEGS_PER_SEC(sbi)) {
>>> -             unsigned int i;
>>>                unsigned long long mtime = 0;
>>>
>>> -             for (i = 0; i < SEGS_PER_SEC(sbi); i++)
>>> -                     mtime += get_seg_entry(sbi, segno + i)->mtime;
>>> -
>>> -             mtime = div_u64(mtime, SEGS_PER_SEC(sbi));
>>> +             mtime = get_section_mtime(sbi, segno);
>>>
>>>                if (sit_i->min_mtime > mtime)
>>>                        sit_i->min_mtime = mtime;
>>
>>
>>
>> _______________________________________________
>> 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