From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Yu Subject: Re: [PATCH 2/5] f2fs: add cur_victim_sec for BG_GC to avoid skipping BG_GC victim Date: Wed, 25 Jul 2018 23:48:49 +0800 Message-ID: References: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com> <1532355022-163029-3-git-send-email-yunlong.song@huawei.com> <42824b9f-6ebb-2280-0a62-c74954fff39c@huawei.com> <07419cf7-057a-e92a-2478-4f827a1d6b2f@kernel.org> <93455370-8524-6a1d-8104-4cf19510ec15@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <93455370-8524-6a1d-8104-4cf19510ec15@huawei.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Yunlong Song , Chao Yu , jaegeuk@kernel.org, yunlong.song@icloud.com Cc: miaoxie@huawei.com, bintian.wang@huawei.com, shengyong1@huawei.com, heyunlei@huawei.com, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org List-Id: linux-f2fs-devel.lists.sourceforge.net 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 >>>>> --- >>>>>    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) >>>>>    ); >>>>> >>>> . >>>> >> . >> >