Re: [PATCH 2/5] f2fs: add cur_victim_sec for BG_GC to avoid skipping BG_GC victim

[Chao Yu <chao@kernel.org>]

On 2018/7/24 23:19, Yunlong Song wrote:
> 
> 
> On 2018/7/24 22:17, Chao Yu wrote:
>> On 2018/7/24 21:39, Yunlong Song wrote:
>>>
>>> On 2018/7/24 21:11, Chao Yu wrote:
>>>> On 2018/7/23 22:10, Yunlong Song wrote:
>>>>> If f2fs aborts BG_GC, then the section bit of victim_secmap will be set,
>>>>> which will cause the section skipped in the future get_victim of BG_GC.
>>>>> In a worst case that each section in the victim_secmap is set and there
>>>>> are enough free sections (so FG_GC can not be triggered), then BG_GC
>>>>> will skip all the sections and cannot find any victims, causing BG_GC
>>>> If f2fs aborts BG_GC, we'd better to clear victim_secmap?
>>> We can keep the bit set in victim_secmap for FG_GC use next time as before, the
>> No, I don't think we could assume that FGGC will come soon, and in adaptive
>> mode, after we triggered SSR agressively, FG_GC will be much less.
>>
>> For your case, we need to clear victim_secmap.
> However, if it is cleared, then FG_GC will lose the chance to have a quick
> selection of the victim
> candidate, which BG_GC has selected and aborted in last round or there are still
> some blocks
> ungced because these blocks belong to an opening atomic file. Especially for the
> large section
> case, when BG_GC stops its job if IO state change from idle to busy, then it is
> better that FG_GC
> can continue to gc the section selected before. So how about adding another map
> to record these
> sections, and make FG_GC/BG_GC select these sections, as for the old
> victim_secmap, keep its
> old logic, BG_GC can not select those sections in victim_secmap, but FG_GC can.

Let's discuss optimization ideas on GC offline? it will be fast and direct than
in mailing list. :)

Thanks,

> 
>>
>>> diffierent
>>> is that this patch will make BG_GC ignore the bit set in victim_secmap, so BG_GC
>>> can still
>>> get the the section (which is in set) as victim and do GC jobs.
>> I guess this scenario is the case our previous scheme tries to prevent, since if
>> in selected section, all block there are cached and set dirty, BGGC will end up
>> with doing nothing, it's inefficient.
> 
> OK, I understand.
> 
>>
>> Thanks,
>>
>>>>> failed each time. Besides, SSR also uses BG_GC to get ssr segment, if
>>>> Looks like foreground GC will try to grab section which is selected as
>>>> victim of background GC?
>>> Yes, this is exactly the value of victim_secmap, it helps FG_GC reduce time in
>>> selecting victims
>>> and continue the job which BG_GC has not finished.
>>>
>>>> Thanks,
>>>>
>>>>> many sections in the victim_secmap are set, then SSR cannot get a proper
>>>>> ssr segment to allocate blocks, which makes SSR inefficiently. To fix
>>>>> this problem, we can add cur_victim_sec for BG_GC similar like that in
>>>>> FG_GC to avoid selecting the same section repeatedly.
>>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>> ---
>>>>>    fs/f2fs/f2fs.h              |  3 ++-
>>>>>    fs/f2fs/gc.c                | 15 +++++++++------
>>>>>    fs/f2fs/segment.h           |  3 ++-
>>>>>    fs/f2fs/super.c             |  3 ++-
>>>>>    include/trace/events/f2fs.h | 18 ++++++++++++------
>>>>>    5 files changed, 27 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 57a8851..f8a7b42 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -1217,7 +1217,8 @@ struct f2fs_sb_info {
>>>>>        /* for cleaning operations */
>>>>>        struct mutex gc_mutex;            /* mutex for GC */
>>>>>        struct f2fs_gc_kthread    *gc_thread;    /* GC thread */
>>>>> -    unsigned int cur_victim_sec;        /* current victim section num */
>>>>> +    unsigned int cur_fg_victim_sec;        /* current FG_GC victim section
>>>>> num */
>>>>> +    unsigned int cur_bg_victim_sec;        /* current BG_GC victim section
>>>>> num */
>>>>>        unsigned int gc_mode;            /* current GC state */
>>>>>        /* for skip statistic */
>>>>>        unsigned long long skipped_atomic_files[2];    /* FG_GC and BG_GC */
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index 2ba470d..705d419 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -367,8 +367,6 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>>>>              if (sec_usage_check(sbi, secno))
>>>>>                goto next;
>>>>> -        if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>>>> -            goto next;
>>>>>              cost = get_gc_cost(sbi, segno, &p);
>>>>>    @@ -391,14 +389,17 @@ static int get_victim_by_default(struct f2fs_sb_info
>>>>> *sbi,
>>>>>            if (p.alloc_mode == LFS) {
>>>>>                secno = GET_SEC_FROM_SEG(sbi, p.min_segno);
>>>>>                if (gc_type == FG_GC)
>>>>> -                sbi->cur_victim_sec = secno;
>>>>> -            else
>>>>> +                sbi->cur_fg_victim_sec = secno;
>>>>> +            else {
>>>>>                    set_bit(secno, dirty_i->victim_secmap);
>>>>> +                sbi->cur_bg_victim_sec = secno;
>>>>> +            }
>>>>>            }
>>>>>            *result = (p.min_segno / p.ofs_unit) * p.ofs_unit;
>>>>>              trace_f2fs_get_victim(sbi->sb, type, gc_type, &p,
>>>>> -                sbi->cur_victim_sec,
>>>>> +                sbi->cur_fg_victim_sec,
>>>>> +                sbi->cur_bg_victim_sec,
>>>>>                    prefree_segments(sbi), free_segments(sbi));
>>>>>        }
>>>>>    out:
>>>>> @@ -1098,7 +1099,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>>>        }
>>>>>          if (gc_type == FG_GC)
>>>>> -        sbi->cur_victim_sec = NULL_SEGNO;
>>>>> +        sbi->cur_fg_victim_sec = NULL_SEGNO;
>>>>> +    else
>>>>> +        sbi->cur_bg_victim_sec = NULL_SEGNO;
>>>>>          if (!sync) {
>>>>>            if (has_not_enough_free_secs(sbi, sec_freed, 0)) {
>>>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>>>> index 5049551..b21bb96 100644
>>>>> --- a/fs/f2fs/segment.h
>>>>> +++ b/fs/f2fs/segment.h
>>>>> @@ -787,7 +787,8 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info
>>>>> *sbi, int base, int type)
>>>>>      static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int
>>>>> secno)
>>>>>    {
>>>>> -    if (IS_CURSEC(sbi, secno) || (sbi->cur_victim_sec == secno))
>>>>> +    if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
>>>>> +        (sbi->cur_bg_victim_sec == secno))
>>>>>            return true;
>>>>>        return false;
>>>>>    }
>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>>>> index 7187885..ef69ebf 100644
>>>>> --- a/fs/f2fs/super.c
>>>>> +++ b/fs/f2fs/super.c
>>>>> @@ -2386,7 +2386,8 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
>>>>>        sbi->root_ino_num = le32_to_cpu(raw_super->root_ino);
>>>>>        sbi->node_ino_num = le32_to_cpu(raw_super->node_ino);
>>>>>        sbi->meta_ino_num = le32_to_cpu(raw_super->meta_ino);
>>>>> -    sbi->cur_victim_sec = NULL_SECNO;
>>>>> +    sbi->cur_fg_victim_sec = NULL_SECNO;
>>>>> +    sbi->cur_bg_victim_sec = NULL_SECNO;
>>>>>        sbi->max_victim_search = DEF_MAX_VICTIM_SEARCH;
>>>>>          sbi->dir_level = DEF_DIR_LEVEL;
>>>>> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
>>>>> index 7956989..0f01f82 100644
>>>>> --- a/include/trace/events/f2fs.h
>>>>> +++ b/include/trace/events/f2fs.h
>>>>> @@ -693,10 +693,12 @@
>>>>>    TRACE_EVENT(f2fs_get_victim,
>>>>>          TP_PROTO(struct super_block *sb, int type, int gc_type,
>>>>> -            struct victim_sel_policy *p, unsigned int pre_victim,
>>>>> +            struct victim_sel_policy *p, unsigned int pre_fg_victim,
>>>>> +            unsigned int pre_bg_victim,
>>>>>                unsigned int prefree, unsigned int free),
>>>>>    -    TP_ARGS(sb, type, gc_type, p, pre_victim, prefree, free),
>>>>> +    TP_ARGS(sb, type, gc_type, p, pre_fg_victim, pre_bg_victim,
>>>>> +        prefree, free),
>>>>>          TP_STRUCT__entry(
>>>>>            __field(dev_t,    dev)
>>>>> @@ -707,7 +709,8 @@
>>>>>            __field(unsigned int,    victim)
>>>>>            __field(unsigned int,    cost)
>>>>>            __field(unsigned int,    ofs_unit)
>>>>> -        __field(unsigned int,    pre_victim)
>>>>> +        __field(unsigned int,    pre_fg_victim)
>>>>> +        __field(unsigned int,    pre_bg_victim)
>>>>>            __field(unsigned int,    prefree)
>>>>>            __field(unsigned int,    free)
>>>>>        ),
>>>>> @@ -721,14 +724,16 @@
>>>>>            __entry->victim        = p->min_segno;
>>>>>            __entry->cost        = p->min_cost;
>>>>>            __entry->ofs_unit    = p->ofs_unit;
>>>>> -        __entry->pre_victim    = pre_victim;
>>>>> +        __entry->pre_fg_victim    = pre_fg_victim;
>>>>> +        __entry->pre_bg_victim    = pre_bg_victim;
>>>>>            __entry->prefree    = prefree;
>>>>>            __entry->free        = free;
>>>>>        ),
>>>>>          TP_printk("dev = (%d,%d), type = %s, policy = (%s, %s, %s), "
>>>>>            "victim = %u, cost = %u, ofs_unit = %u, "
>>>>> -        "pre_victim_secno = %d, prefree = %u, free = %u",
>>>>> +        "pre_fg_victim_secno = %d, pre_bg_victim_secno = %d, "
>>>>> +        "prefree = %u, free = %u",
>>>>>            show_dev(__entry->dev),
>>>>>            show_data_type(__entry->type),
>>>>>            show_gc_type(__entry->gc_type),
>>>>> @@ -737,7 +742,8 @@
>>>>>            __entry->victim,
>>>>>            __entry->cost,
>>>>>            __entry->ofs_unit,
>>>>> -        (int)__entry->pre_victim,
>>>>> +        (int)__entry->pre_fg_victim,
>>>>> +        (int)__entry->pre_bg_victim,
>>>>>            __entry->prefree,
>>>>>            __entry->free)
>>>>>    );
>>>>>
>>>> .
>>>>
>> .
>>
>