[PATCH 0/5] f2fs: fix and improve for victim_secmap

[PATCH 0/5] f2fs: fix and improve for victim_secmap

[Yunlong Song]

There are some fixes and improvements for using with victim_secmap.

Yunlong Song (5):
  f2fs: clear victim_secmap when section has full valid blocks
  f2fs: add cur_victim_sec for BG_GC to avoid skipping BG_GC victim
  f2fs: clear_bit the SSR selected section in the victim_secmap
  f2fs: let BG_GC check every dirty segments and gc over a threshold
  f2fs: add proc entry to show victim_secmap bitmap

 fs/f2fs/f2fs.h              |  5 ++++-
 fs/f2fs/gc.c                | 39 ++++++++++++++++++++++++++++++---------
 fs/f2fs/segment.c           |  4 +++-
 fs/f2fs/segment.h           | 12 +++++++++++-
 fs/f2fs/super.c             |  3 ++-
 fs/f2fs/sysfs.c             | 25 +++++++++++++++++++++++++
 include/trace/events/f2fs.h | 18 ++++++++++++------
 7 files changed, 87 insertions(+), 19 deletions(-)

-- 
1.8.5.2

[PATCH 1/5] f2fs: clear victim_secmap when section has full valid blocks

[Yunlong Song]

Without this patch, f2fs only clears victim_secmap when it finds out
that the section has no valid blocks at all, but forgets to clear the
victim_secmap when the whole section has full valid blocks.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index cfff7cf..255bff5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -776,7 +776,9 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
 		if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
 			dirty_i->nr_dirty[t]--;
 
-		if (get_valid_blocks(sbi, segno, true) == 0)
+		if (get_valid_blocks(sbi, segno, true) == 0 ||
+			get_valid_blocks(sbi, segno, true) ==
+			(sbi->segs_per_sec << sbi->log_blocks_per_seg))
 			clear_bit(GET_SEC_FROM_SEG(sbi, segno),
 						dirty_i->victim_secmap);
 	}
-- 
1.8.5.2

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

[Yunlong Song]

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
failed each time. Besides, SSR also uses BG_GC to get ssr segment, if
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)
 );
-- 
1.8.5.2

[PATCH 3/5] f2fs: clear_bit the SSR selected section in the victim_secmap

[Yunlong Song]

SSR uses get_victim to select ssr segment to allocate data blocks, which
makes the previous result of victim_secmap inaccurately, so we would
better clear the bit of the section in the victim_secmap.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/gc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 705d419..0e7a265 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -394,7 +394,8 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
 				set_bit(secno, dirty_i->victim_secmap);
 				sbi->cur_bg_victim_sec = secno;
 			}
-		}
+		} else
+			clear_bit(secno, dirty_i->victim_secmap);
 		*result = (p.min_segno / p.ofs_unit) * p.ofs_unit;
 
 		trace_f2fs_get_victim(sbi->sb, type, gc_type, &p,
-- 
1.8.5.2

[PATCH 4/5] f2fs: let BG_GC check every dirty segments and gc over a threshold

[Yunlong Song]

BG_GC is triggered in idle time, so it is better check every dirty
segment and finds the best victim to gc. Otherwise, BG_GC will be
limited to only 8G areas, and probably select a victim which has nearly
full of valid blocks, resulting a big WAI. Besides, we also add a
bggc_threshold (which is the old "fggc_threshold", so revert commit
"299254") to stop BG_GC when there is no good choice. This is especially
good for large section case to reduce WAI.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/f2fs.h    |  2 ++
 fs/f2fs/gc.c      | 23 ++++++++++++++++++++---
 fs/f2fs/segment.h |  9 +++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index f8a7b42..24a9d7f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1220,6 +1220,8 @@ struct f2fs_sb_info {
 	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 */
+	/* threshold for selecting bg victims */
+	u64 bggc_threshold;
 	/* 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 0e7a265..21e8d59 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -189,9 +189,8 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
 		p->ofs_unit = sbi->segs_per_sec;
 	}
 
-	/* we need to check every dirty segments in the FG_GC case */
-	if (gc_type != FG_GC &&
-			(sbi->gc_mode != GC_URGENT) &&
+	/* we need to check every dirty segments in the GC case */
+	if (p->alloc_mode == SSR &&
 			p->max_search > sbi->max_victim_search)
 		p->max_search = sbi->max_victim_search;
 
@@ -230,6 +229,10 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
 	for_each_set_bit(secno, dirty_i->victim_secmap, MAIN_SECS(sbi)) {
 		if (sec_usage_check(sbi, secno))
 			continue;
+
+		if (no_bggc_candidate(sbi, secno))
+			continue;
+
 		clear_bit(secno, dirty_i->victim_secmap);
 		return GET_SEG_FROM_SEC(sbi, secno);
 	}
@@ -368,6 +371,10 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
 		if (sec_usage_check(sbi, secno))
 			goto next;
 
+		if (gc_type == BG_GC && p.alloc_mode == LFS &&
+					no_bggc_candidate(sbi, secno))
+			goto next;
+
 		cost = get_gc_cost(sbi, segno, &p);
 
 		if (p.min_cost > cost) {
@@ -1140,8 +1147,18 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
 
 void f2fs_build_gc_manager(struct f2fs_sb_info *sbi)
 {
+	u64 main_count, resv_count, ovp_count;
+
 	DIRTY_I(sbi)->v_ops = &default_v_ops;
 
+	/* threshold of # of valid blocks in a section for victims of BG_GC */
+	main_count = SM_I(sbi)->main_segments << sbi->log_blocks_per_seg;
+	resv_count = SM_I(sbi)->reserved_segments << sbi->log_blocks_per_seg;
+	ovp_count = SM_I(sbi)->ovp_segments << sbi->log_blocks_per_seg;
+
+	sbi->bggc_threshold = div64_u64((main_count - ovp_count) *
+					BLKS_PER_SEC(sbi), (main_count - resv_count));
+
 	sbi->gc_pin_file_threshold = DEF_GC_FAILED_PINNED_FILES;
 
 	/* give warm/cold data area from slower device */
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index b21bb96..932e59b 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -785,6 +785,15 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
 				- (base + 1) + type;
 }
 
+static inline bool no_bggc_candidate(struct f2fs_sb_info *sbi,
+                        unsigned int secno)
+{
+    if (get_valid_blocks(sbi, GET_SEG_FROM_SEC(sbi, secno), true) >
+                        sbi->bggc_threshold)
+        return true;
+    return false;
+}
+
 static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
 {
 	if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
-- 
1.8.5.2

[PATCH 5/5] f2fs: add proc entry to show victim_secmap bitmap

[Yunlong Song]

This patch adds a new proc entry to show victim_secmap information in
more detail, which is very helpful to know the get_victim candidate
status clearly, and helpful to debug problems (e.g., some sections can
not gc all of its blocks, since some blocks belong to atomic file,
leaving victim_secmap with section bit setting, in extrem case, this
will lead all bytes of victim_secmap setting with 0xff).

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/sysfs.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index bca1236..f22782a 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -615,6 +615,28 @@ static int __maybe_unused iostat_info_seq_show(struct seq_file *seq,
 	return 0;
 }
 
+static int __maybe_unused victim_bits_seq_show(struct seq_file *seq,
+						void *offset)
+{
+	struct super_block *sb = seq->private;
+	struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	int i;
+
+	seq_puts(seq, "format: victim_secmap bitmaps\n");
+
+	for (i = 0; i < MAIN_SECS(sbi); i++) {
+		if ((i % 10) == 0)
+			seq_printf(seq, "%-10d", i);
+		seq_printf(seq, "%d", test_bit(i, dirty_i->victim_secmap) ? 1 : 0);
+		if ((i % 10) == 9 || i == (MAIN_SECS(sbi) - 1))
+			seq_putc(seq, '\n');
+		else
+			seq_putc(seq, ' ');
+	}
+	return 0;
+}
+
 int __init f2fs_init_sysfs(void)
 {
 	int ret;
@@ -664,6 +686,8 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
 				segment_bits_seq_show, sb);
 		proc_create_single_data("iostat_info", S_IRUGO, sbi->s_proc,
 				iostat_info_seq_show, sb);
+		proc_create_single_data("victim_bits", S_IRUGO, sbi->s_proc,
+				victim_bits_seq_show, sb);
 	}
 	return 0;
 }
@@ -674,6 +698,7 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
 		remove_proc_entry("iostat_info", sbi->s_proc);
 		remove_proc_entry("segment_info", sbi->s_proc);
 		remove_proc_entry("segment_bits", sbi->s_proc);
+		remove_proc_entry("victim_bits", sbi->s_proc);
 		remove_proc_entry(sbi->sb->s_id, f2fs_proc_root);
 	}
 	kobject_del(&sbi->s_kobj);
-- 
1.8.5.2

Re: [PATCH 3/5] f2fs: clear_bit the SSR selected section in the victim_secmap

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]

Hi Yunlong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on f2fs/dev-test]
[also build test WARNING on v4.18-rc6 next-20180723]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Yunlong-Song/f2fs-clear-victim_secmap-when-section-has-full-valid-blocks/20180724-042247
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git dev-test
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=parisc 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/linux/bitops.h:38:0,
                    from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/wait.h:7,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from fs/f2fs/gc.c:11:
   fs/f2fs/gc.c: In function 'get_victim_by_default':
>> arch/parisc/include/asm/bitops.h:27:51: warning: 'secno' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define CHOP_SHIFTCOUNT(x) (((unsigned long) (x)) & (BITS_PER_LONG - 1))
                                                      ^
   fs/f2fs/gc.c:310:15: note: 'secno' was declared here
     unsigned int secno, last_victim;
                  ^~~~~
--
   In file included from include/linux/bitops.h:38:0,
                    from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/wait.h:7,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from fs//f2fs/gc.c:11:
   fs//f2fs/gc.c: In function 'get_victim_by_default':
>> arch/parisc/include/asm/bitops.h:27:51: warning: 'secno' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define CHOP_SHIFTCOUNT(x) (((unsigned long) (x)) & (BITS_PER_LONG - 1))
                                                      ^
   fs//f2fs/gc.c:310:15: note: 'secno' was declared here
     unsigned int secno, last_victim;
                  ^~~~~

vim +/secno +27 arch/parisc/include/asm/bitops.h

2ad5d52d4 arch/parisc/include/asm/bitops.h Helge Deller   2017-01-28  26  
a366064c3 include/asm-parisc/bitops.h      Grant Grundler 2005-10-21 @27  #define CHOP_SHIFTCOUNT(x) (((unsigned long) (x)) & (BITS_PER_LONG - 1))
^1da177e4 include/asm-parisc/bitops.h      Linus Torvalds 2005-04-16  28  
^1da177e4 include/asm-parisc/bitops.h      Linus Torvalds 2005-04-16  29  

:::::: The code at line 27 was first introduced by commit
:::::: a366064c3ff46c985a3c7243468be197d29874dc [PARISC] Update bitops from parisc tree

:::::: TO: Grant Grundler <grundler@parisc-linux.org>
:::::: CC: Kyle McMartin <kyle@parisc-linux.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53528 bytes --]

Re: [PATCH 1/5] f2fs: clear victim_secmap when section has full valid blocks

[Chao Yu]

On 2018/7/23 22:10, Yunlong Song wrote:
> Without this patch, f2fs only clears victim_secmap when it finds out
> that the section has no valid blocks at all, but forgets to clear the
> victim_secmap when the whole section has full valid blocks.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index cfff7cf..255bff5 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -776,7 +776,9 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
>  		if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
>  			dirty_i->nr_dirty[t]--;
>  
> -		if (get_valid_blocks(sbi, segno, true) == 0)
> +		if (get_valid_blocks(sbi, segno, true) == 0 ||
> +			get_valid_blocks(sbi, segno, true) ==
> +			(sbi->segs_per_sec << sbi->log_blocks_per_seg))

BLKS_PER_SEC(sbi)?

Thanks,

>  			clear_bit(GET_SEC_FROM_SEG(sbi, segno),
>  						dirty_i->victim_secmap);
>  	}
> 

[PATCH v2] f2fs: clear victim_secmap when section has full valid blocks

[Yunlong Song]

Without this patch, f2fs only clears victim_secmap when it finds out
that the section has no valid blocks at all, but forgets to clear the
victim_secmap when the whole section has full valid blocks.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index cfff7cf..0a79554 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -776,7 +776,8 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
 		if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
 			dirty_i->nr_dirty[t]--;
 
-		if (get_valid_blocks(sbi, segno, true) == 0)
+		if (get_valid_blocks(sbi, segno, true) == 0 ||
+			get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi))
 			clear_bit(GET_SEC_FROM_SEG(sbi, segno),
 						dirty_i->victim_secmap);
 	}
-- 
1.8.5.2

Re: [PATCH v2] f2fs: clear victim_secmap when section has full valid blocks

[Chao Yu]

On 2018/7/24 17:27, Yunlong Song wrote:
> Without this patch, f2fs only clears victim_secmap when it finds out
> that the section has no valid blocks at all, but forgets to clear the
> victim_secmap when the whole section has full valid blocks.

Look this patch again, I have a question, why bggc selected section can
get back to full state?

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index cfff7cf..0a79554 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -776,7 +776,8 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
>  		if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
>  			dirty_i->nr_dirty[t]--;
>  
> -		if (get_valid_blocks(sbi, segno, true) == 0)
> +		if (get_valid_blocks(sbi, segno, true) == 0 ||
> +			get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi))
>  			clear_bit(GET_SEC_FROM_SEG(sbi, segno),
>  						dirty_i->victim_secmap);
>  	}
> 

Re: [PATCH v2] f2fs: clear victim_secmap when section has full valid blocks

[Yunlong Song]

Just in case, or maybe this patch should be put after [patch 2/5],which 
let BG_GC avoids
skipping BG_GC victim, then SSR can also select the BG_GC victim to 
allocate data blocks,
which can make bggc selected section get back to full state.

On 2018/7/24 17:36, Chao Yu wrote:
> On 2018/7/24 17:27, Yunlong Song wrote:
>> Without this patch, f2fs only clears victim_secmap when it finds out
>> that the section has no valid blocks at all, but forgets to clear the
>> victim_secmap when the whole section has full valid blocks.
> Look this patch again, I have a question, why bggc selected section can
> get back to full state?
>
> Thanks,
>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>   fs/f2fs/segment.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index cfff7cf..0a79554 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -776,7 +776,8 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
>>   		if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
>>   			dirty_i->nr_dirty[t]--;
>>   
>> -		if (get_valid_blocks(sbi, segno, true) == 0)
>> +		if (get_valid_blocks(sbi, segno, true) == 0 ||
>> +			get_valid_blocks(sbi, segno, true) == BLKS_PER_SEC(sbi))
>>   			clear_bit(GET_SEC_FROM_SEG(sbi, segno),
>>   						dirty_i->victim_secmap);
>>   	}
>>
>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

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?

> 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?

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)
>  );
> 

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

[Yunlong Song]

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 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.
>
>> 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)
>>   );
>>
>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

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.

> 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.

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)
>>>   );
>>>
>>
>> .
>>
> 

Re: [PATCH 4/5] f2fs: let BG_GC check every dirty segments and gc over a threshold

[Chao Yu]

On 2018/7/23 22:10, Yunlong Song wrote:
> BG_GC is triggered in idle time, so it is better check every dirty
> segment and finds the best victim to gc. Otherwise, BG_GC will be
> limited to only 8G areas, and probably select a victim which has nearly

If 8GB range is not enough and just hard code now, we can export it in sysfs and
do the configuration.

> full of valid blocks, resulting a big WAI. Besides, we also add a

BGGC should move cold data anway, if we only consider WA, hot data section can
be selected with very high probability, but hot data can do OPU itself sooner or
later, so moving them will cause higher WA.

I think the better way is we can export a sysfs entry to adjust factor to
control weight of aging or valid block of section. So that, user can adjust it
to select less valid block candidate first instead of high aging one.

Thanks,

> bggc_threshold (which is the old "fggc_threshold", so revert commit
> "299254") to stop BG_GC when there is no good choice. This is especially
> good for large section case to reduce WAI.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/f2fs.h    |  2 ++
>  fs/f2fs/gc.c      | 23 ++++++++++++++++++++---
>  fs/f2fs/segment.h |  9 +++++++++
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index f8a7b42..24a9d7f 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1220,6 +1220,8 @@ struct f2fs_sb_info {
>  	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 */
> +	/* threshold for selecting bg victims */
> +	u64 bggc_threshold;
>  	/* 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 0e7a265..21e8d59 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -189,9 +189,8 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
>  		p->ofs_unit = sbi->segs_per_sec;
>  	}
>  
> -	/* we need to check every dirty segments in the FG_GC case */
> -	if (gc_type != FG_GC &&
> -			(sbi->gc_mode != GC_URGENT) &&
> +	/* we need to check every dirty segments in the GC case */
> +	if (p->alloc_mode == SSR &&
>  			p->max_search > sbi->max_victim_search)
>  		p->max_search = sbi->max_victim_search;
>  
> @@ -230,6 +229,10 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
>  	for_each_set_bit(secno, dirty_i->victim_secmap, MAIN_SECS(sbi)) {
>  		if (sec_usage_check(sbi, secno))
>  			continue;
> +
> +		if (no_bggc_candidate(sbi, secno))
> +			continue;
> +
>  		clear_bit(secno, dirty_i->victim_secmap);
>  		return GET_SEG_FROM_SEC(sbi, secno);
>  	}
> @@ -368,6 +371,10 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  		if (sec_usage_check(sbi, secno))
>  			goto next;
>  
> +		if (gc_type == BG_GC && p.alloc_mode == LFS &&
> +					no_bggc_candidate(sbi, secno))
> +			goto next;
> +
>  		cost = get_gc_cost(sbi, segno, &p);
>  
>  		if (p.min_cost > cost) {
> @@ -1140,8 +1147,18 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  
>  void f2fs_build_gc_manager(struct f2fs_sb_info *sbi)
>  {
> +	u64 main_count, resv_count, ovp_count;
> +
>  	DIRTY_I(sbi)->v_ops = &default_v_ops;
>  
> +	/* threshold of # of valid blocks in a section for victims of BG_GC */
> +	main_count = SM_I(sbi)->main_segments << sbi->log_blocks_per_seg;
> +	resv_count = SM_I(sbi)->reserved_segments << sbi->log_blocks_per_seg;
> +	ovp_count = SM_I(sbi)->ovp_segments << sbi->log_blocks_per_seg;
> +
> +	sbi->bggc_threshold = div64_u64((main_count - ovp_count) *
> +					BLKS_PER_SEC(sbi), (main_count - resv_count));
> +
>  	sbi->gc_pin_file_threshold = DEF_GC_FAILED_PINNED_FILES;
>  
>  	/* give warm/cold data area from slower device */
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index b21bb96..932e59b 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -785,6 +785,15 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
>  				- (base + 1) + type;
>  }
>  
> +static inline bool no_bggc_candidate(struct f2fs_sb_info *sbi,
> +                        unsigned int secno)
> +{
> +    if (get_valid_blocks(sbi, GET_SEG_FROM_SEC(sbi, secno), true) >
> +                        sbi->bggc_threshold)
> +        return true;
> +    return false;
> +}
> +
>  static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
>  {
>  	if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
> 

Re: [PATCH 5/5] f2fs: add proc entry to show victim_secmap bitmap

[Chao Yu]

On 2018/7/23 22:10, Yunlong Song wrote:
> This patch adds a new proc entry to show victim_secmap information in
> more detail, which is very helpful to know the get_victim candidate
> status clearly, and helpful to debug problems (e.g., some sections can
> not gc all of its blocks, since some blocks belong to atomic file,
> leaving victim_secmap with section bit setting, in extrem case, this
> will lead all bytes of victim_secmap setting with 0xff).
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

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

[Yunlong Song]

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.

>
>> 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)
>>>>    );
>>>>
>>> .
>>>
> .
>

-- 
Thanks,
Yunlong Song

Re: [PATCH 4/5] f2fs: let BG_GC check every dirty segments and gc over a threshold

[Yunlong Song]

On 2018/7/24 22:52, Chao Yu wrote:
> On 2018/7/23 22:10, Yunlong Song wrote:
>> BG_GC is triggered in idle time, so it is better check every dirty
>> segment and finds the best victim to gc. Otherwise, BG_GC will be
>> limited to only 8G areas, and probably select a victim which has nearly
> If 8GB range is not enough and just hard code now, we can export it in sysfs and
> do the configuration.
There is already a sysfs entry called max_victim_search, but the default 
value is defined
as DEF_MAX_VICTIM_SEARCH, i.e., 4096, equals to 8GB. So can we increase 
this default
value to UINT_MAX?
>
>> full of valid blocks, resulting a big WAI. Besides, we also add a
> BGGC should move cold data anway, if we only consider WA, hot data section can
> be selected with very high probability, but hot data can do OPU itself sooner or
> later, so moving them will cause higher WA.
Yes, but this problem also appears in the default 8G area, even in 8G 
area, perhaps there is
still a victim section which has fewest valid blocks but with hot data 
type. This patch adds
a bggc_threshold to avoid big WA and wishes SSR write data to the 
section whose threshold
is over bggc_threshold but with cold data type. Since the initial 
min_cost in BG_GC is valued
as UINT_MAX, BG_GC can always successfully select a victim and move 
blocks in common case,
but sometimes it is not needed, for example, there are already enough 
free sections and each
dirty section has same valid blocks, if BG_GC continue its job, then 
there is a big WA.

>
> I think the better way is we can export a sysfs entry to adjust factor to
> control weight of aging or valid block of section. So that, user can adjust it
> to select less valid block candidate first instead of high aging one.
How about export the bggc_threshold as sysfs entry, the default value is 
defined as the old
fggc_threshold, i.e., (main - ovp) / (main - rsvd) * BLKS_PER_SEC. User 
can adjust this value
to control the WA and non-WA requirement.

>
> Thanks,
>
>> bggc_threshold (which is the old "fggc_threshold", so revert commit
>> "299254") to stop BG_GC when there is no good choice. This is especially
>> good for large section case to reduce WAI.
>>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>   fs/f2fs/f2fs.h    |  2 ++
>>   fs/f2fs/gc.c      | 23 ++++++++++++++++++++---
>>   fs/f2fs/segment.h |  9 +++++++++
>>   3 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index f8a7b42..24a9d7f 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -1220,6 +1220,8 @@ struct f2fs_sb_info {
>>   	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 */
>> +	/* threshold for selecting bg victims */
>> +	u64 bggc_threshold;
>>   	/* 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 0e7a265..21e8d59 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -189,9 +189,8 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
>>   		p->ofs_unit = sbi->segs_per_sec;
>>   	}
>>   
>> -	/* we need to check every dirty segments in the FG_GC case */
>> -	if (gc_type != FG_GC &&
>> -			(sbi->gc_mode != GC_URGENT) &&
>> +	/* we need to check every dirty segments in the GC case */
>> +	if (p->alloc_mode == SSR &&
>>   			p->max_search > sbi->max_victim_search)
>>   		p->max_search = sbi->max_victim_search;
>>   
>> @@ -230,6 +229,10 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
>>   	for_each_set_bit(secno, dirty_i->victim_secmap, MAIN_SECS(sbi)) {
>>   		if (sec_usage_check(sbi, secno))
>>   			continue;
>> +
>> +		if (no_bggc_candidate(sbi, secno))
>> +			continue;
>> +
>>   		clear_bit(secno, dirty_i->victim_secmap);
>>   		return GET_SEG_FROM_SEC(sbi, secno);
>>   	}
>> @@ -368,6 +371,10 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   		if (sec_usage_check(sbi, secno))
>>   			goto next;
>>   
>> +		if (gc_type == BG_GC && p.alloc_mode == LFS &&
>> +					no_bggc_candidate(sbi, secno))
>> +			goto next;
>> +
>>   		cost = get_gc_cost(sbi, segno, &p);
>>   
>>   		if (p.min_cost > cost) {
>> @@ -1140,8 +1147,18 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   
>>   void f2fs_build_gc_manager(struct f2fs_sb_info *sbi)
>>   {
>> +	u64 main_count, resv_count, ovp_count;
>> +
>>   	DIRTY_I(sbi)->v_ops = &default_v_ops;
>>   
>> +	/* threshold of # of valid blocks in a section for victims of BG_GC */
>> +	main_count = SM_I(sbi)->main_segments << sbi->log_blocks_per_seg;
>> +	resv_count = SM_I(sbi)->reserved_segments << sbi->log_blocks_per_seg;
>> +	ovp_count = SM_I(sbi)->ovp_segments << sbi->log_blocks_per_seg;
>> +
>> +	sbi->bggc_threshold = div64_u64((main_count - ovp_count) *
>> +					BLKS_PER_SEC(sbi), (main_count - resv_count));
>> +
>>   	sbi->gc_pin_file_threshold = DEF_GC_FAILED_PINNED_FILES;
>>   
>>   	/* give warm/cold data area from slower device */
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index b21bb96..932e59b 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -785,6 +785,15 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
>>   				- (base + 1) + type;
>>   }
>>   
>> +static inline bool no_bggc_candidate(struct f2fs_sb_info *sbi,
>> +                        unsigned int secno)
>> +{
>> +    if (get_valid_blocks(sbi, GET_SEG_FROM_SEC(sbi, secno), true) >
>> +                        sbi->bggc_threshold)
>> +        return true;
>> +    return false;
>> +}
>> +
>>   static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
>>   {
>>   	if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
>>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

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)
>>>>>    );
>>>>>
>>>> .
>>>>
>> .
>>
>