* [PATCH v2 0/2] f2fs: modify the calculation method of mtime @ 2024-09-12 6:40 liuderong 2024-09-12 6:40 ` [PATCH v2 1/2] f2fs: remove unused parameters liuderong 2024-09-12 6:40 ` [PATCH v2 2/2] f2fs: introduce get_section_mtime liuderong 0 siblings, 2 replies; 6+ messages in thread From: liuderong @ 2024-09-12 6:40 UTC (permalink / raw) To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, liuderong From: liuderong <liuderong@oppo.com> In cost-benefit GC algorithm, mtime will affect the selection of victim segment.For a large section, mtime should be the mean value of valid segments, in order to select correct victim segment, it needs to modify the calculation method of mtime. v2: - modify new API:get_section_mtime liuderong (2): f2fs: remove unused parameters f2fs: introduce get_section_mtime fs/f2fs/f2fs.h | 5 +++-- fs/f2fs/gc.c | 21 +++++---------------- fs/f2fs/segment.c | 44 +++++++++++++++++++++++++++++++++++++------- fs/f2fs/segment.h | 4 ++-- 4 files changed, 47 insertions(+), 27 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] f2fs: remove unused parameters 2024-09-12 6:40 [PATCH v2 0/2] f2fs: modify the calculation method of mtime liuderong @ 2024-09-12 6:40 ` liuderong 2024-09-12 6:40 ` [PATCH v2 2/2] f2fs: introduce get_section_mtime liuderong 1 sibling, 0 replies; 6+ messages in thread From: liuderong @ 2024-09-12 6:40 UTC (permalink / raw) To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, liuderong From: liuderong <liuderong@oppo.com> Remove unused parameter segno from f2fs_usable_segs_in_sec. Signed-off-by: liuderong <liuderong@oppo.com> --- fs/f2fs/f2fs.h | 3 +-- fs/f2fs/gc.c | 6 +++--- fs/f2fs/segment.c | 3 +-- fs/f2fs/segment.h | 4 ++-- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index ac19c61..4dcdcdd 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -3759,8 +3759,7 @@ void f2fs_destroy_segment_manager_caches(void); int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, enum rw_hint hint); enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi, enum page_type type, enum temp_type temp); -unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi, - unsigned int segno); +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); diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 724bbcb..6299639 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -339,7 +339,7 @@ static unsigned int get_cb_cost(struct f2fs_sb_info *sbi, unsigned int segno) unsigned char age = 0; unsigned char u; unsigned int i; - unsigned int usable_segs_per_sec = f2fs_usable_segs_in_sec(sbi, segno); + 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; @@ -1707,7 +1707,7 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi, */ if (f2fs_sb_has_blkzoned(sbi)) end_segno -= SEGS_PER_SEC(sbi) - - f2fs_usable_segs_in_sec(sbi, segno); + f2fs_usable_segs_in_sec(sbi); sanity_check_seg_type(sbi, get_seg_entry(sbi, segno)->type); @@ -1881,7 +1881,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) total_freed += seg_freed; - if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) { + if (seg_freed == f2fs_usable_segs_in_sec(sbi)) { sec_freed++; total_sec_freed++; } diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 78c3198..6627394 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -5381,8 +5381,7 @@ unsigned int f2fs_usable_blks_in_seg(struct f2fs_sb_info *sbi, return BLKS_PER_SEG(sbi); } -unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi, - unsigned int segno) +unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi) { if (f2fs_sb_has_blkzoned(sbi)) return CAP_SEGS_PER_SEC(sbi); diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h index bfc01a5..9e61ded 100644 --- a/fs/f2fs/segment.h +++ b/fs/f2fs/segment.h @@ -430,7 +430,7 @@ static inline void __set_free(struct f2fs_sb_info *sbi, unsigned int segno) unsigned int secno = GET_SEC_FROM_SEG(sbi, segno); unsigned int start_segno = GET_SEG_FROM_SEC(sbi, secno); unsigned int next; - unsigned int usable_segs = f2fs_usable_segs_in_sec(sbi, segno); + unsigned int usable_segs = f2fs_usable_segs_in_sec(sbi); spin_lock(&free_i->segmap_lock); clear_bit(segno, free_i->free_segmap); @@ -464,7 +464,7 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, unsigned int secno = GET_SEC_FROM_SEG(sbi, segno); unsigned int start_segno = GET_SEG_FROM_SEC(sbi, secno); unsigned int next; - unsigned int usable_segs = f2fs_usable_segs_in_sec(sbi, segno); + unsigned int usable_segs = f2fs_usable_segs_in_sec(sbi); spin_lock(&free_i->segmap_lock); if (test_and_clear_bit(segno, free_i->free_segmap)) { -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] f2fs: introduce get_section_mtime 2024-09-12 6:40 [PATCH v2 0/2] f2fs: modify the calculation method of mtime liuderong 2024-09-12 6:40 ` [PATCH v2 1/2] f2fs: remove unused parameters liuderong @ 2024-09-12 6:40 ` liuderong 2024-09-18 6:43 ` Chao Yu 1 sibling, 1 reply; 6+ messages in thread From: liuderong @ 2024-09-12 6:40 UTC (permalink / raw) To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, liuderong 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); #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; + + 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); + } + + 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; -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] f2fs: introduce get_section_mtime 2024-09-12 6:40 ` [PATCH v2 2/2] f2fs: introduce get_section_mtime liuderong @ 2024-09-18 6:43 ` Chao Yu 2024-09-19 2:23 ` [f2fs-dev] " Zhiguo Niu 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2024-09-18 6:43 UTC (permalink / raw) To: liuderong, jaegeuk; +Cc: chao, linux-f2fs-devel, linux-kernel 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; } 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH v2 2/2] f2fs: introduce get_section_mtime 2024-09-18 6:43 ` Chao Yu @ 2024-09-19 2:23 ` Zhiguo Niu 2024-09-19 3:23 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Zhiguo Niu @ 2024-09-19 2:23 UTC (permalink / raw) To: Chao Yu; +Cc: liuderong, jaegeuk, linux-kernel, linux-f2fs-devel 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? 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [PATCH v2 2/2] f2fs: introduce get_section_mtime 2024-09-19 2:23 ` [f2fs-dev] " Zhiguo Niu @ 2024-09-19 3:23 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2024-09-19 3:23 UTC (permalink / raw) To: Zhiguo Niu, Chao Yu; +Cc: liuderong, jaegeuk, linux-kernel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-19 3:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-12 6:40 [PATCH v2 0/2] f2fs: modify the calculation method of mtime liuderong 2024-09-12 6:40 ` [PATCH v2 1/2] f2fs: remove unused parameters liuderong 2024-09-12 6:40 ` [PATCH v2 2/2] f2fs: introduce get_section_mtime liuderong 2024-09-18 6:43 ` Chao Yu 2024-09-19 2:23 ` [f2fs-dev] " Zhiguo Niu 2024-09-19 3:23 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome