[f2fs-dev] [PATCH v2 0/2] f2fs: modify the calculation method of mtime

[f2fs-dev] [PATCH v2 0/2] f2fs: modify the calculation method of mtime

[liuderong--- via Linux-f2fs-devel]

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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v2 1/2] f2fs: remove unused parameters

[liuderong--- via Linux-f2fs-devel]

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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[liuderong--- via Linux-f2fs-devel]

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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu via Linux-f2fs-devel]

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;



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Zhiguo Niu]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu via 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



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel