All of lore.kernel.org
 help / color / mirror / Atom feed
* [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-06 15:26 ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-06 15:26 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel

Previously, during foreground GC, if victims contain data of pinned file,
it will fail migration of the data, and meanwhile i_gc_failures of that
pinned file may increase, and when it exceeds threshold, GC will unpin
the file, result in breaking pinfile's semantics.

In order to mitigate such condition, let's record and skip section which
has pinned file's data and give priority to select unpinned one.

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
v3:
- check pin status before pinning section in pin_section().
 fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
 fs/f2fs/segment.c |  7 ++++++
 fs/f2fs/segment.h |  2 ++
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 6a7e4148ff9d..df23824ae3c2 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
 	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
 }
 
+static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
+{
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
+
+	if (test_bit(secno, dirty_i->pinned_secmap))
+		return;
+	set_bit(secno, dirty_i->pinned_secmap);
+	dirty_i->pinned_secmap_cnt++;
+}
+
+static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
+{
+	return dirty_i->pinned_secmap_cnt;
+}
+
+static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
+						unsigned int secno)
+{
+	return pinned_section_exists(dirty_i) &&
+			test_bit(secno, dirty_i->pinned_secmap);
+}
+
+static void unpin_all_sections(struct f2fs_sb_info *sbi)
+{
+	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
+
+	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
+	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
+}
+
 /*
  * This function is called from two paths.
  * One is garbage collection and the other is SSR segment selection.
@@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
 		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
 			goto next;
 
+		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
+			goto next;
+
 		if (is_atgc) {
 			add_victim_entry(sbi, &p, segno);
 			goto next;
@@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
 	}
 
 	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
+		if (gc_type == FG_GC) {
 			f2fs_pin_file_control(inode, true);
+			pin_section(F2FS_I_SB(inode), segno);
+		}
 		err = -EAGAIN;
 		goto out;
 	}
@@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
 		goto out;
 	}
 	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
+		if (gc_type == FG_GC) {
 			f2fs_pin_file_control(inode, true);
+			pin_section(F2FS_I_SB(inode), segno);
+		}
 		err = -EAGAIN;
 		goto out;
 	}
@@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 							gc_type == FG_GC) {
 				f2fs_pin_file_control(inode, true);
 				iput(inode);
+				pin_section(sbi, segno);
 				return submitted;
 			}
 
@@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
 		ret = -EINVAL;
 		goto stop;
 	}
+retry:
 	ret = __get_victim(sbi, &segno, gc_type);
-	if (ret)
+	if (ret) {
+		/* allow to search victim from sections has pinned data */
+		if (ret == -ENODATA && gc_type == FG_GC &&
+				pinned_section_exists(DIRTY_I(sbi))) {
+			unpin_all_sections(sbi);
+			goto retry;
+		}
 		goto stop;
+	}
 
 	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
 	if (gc_type == FG_GC &&
@@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
 	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
 	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
 
+	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
+		unpin_all_sections(sbi);
+
 	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
 				get_pages(sbi, F2FS_DIRTY_NODES),
 				get_pages(sbi, F2FS_DIRTY_DENTS),
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 012524db7437..1c20d7c9eca3 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
 	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
 	if (!dirty_i->victim_secmap)
 		return -ENOMEM;
+
+	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
+	if (!dirty_i->pinned_secmap)
+		return -ENOMEM;
+
+	dirty_i->pinned_secmap_cnt = 0;
 	return 0;
 }
 
@@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
 {
 	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
 
+	kvfree(dirty_i->pinned_secmap);
 	kvfree(dirty_i->victim_secmap);
 }
 
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c94caf0c0a1..fd6f246e649c 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -294,6 +294,8 @@ struct dirty_seglist_info {
 	struct mutex seglist_lock;		/* lock for segment bitmaps */
 	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
 	unsigned long *victim_secmap;		/* background GC victims */
+	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
+	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
 };
 
 /* victim selection function for cleaning and SSR */
-- 
2.32.0



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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-06 15:26 ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-06 15:26 UTC (permalink / raw)
  To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu, Chao Yu

Previously, during foreground GC, if victims contain data of pinned file,
it will fail migration of the data, and meanwhile i_gc_failures of that
pinned file may increase, and when it exceeds threshold, GC will unpin
the file, result in breaking pinfile's semantics.

In order to mitigate such condition, let's record and skip section which
has pinned file's data and give priority to select unpinned one.

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
v3:
- check pin status before pinning section in pin_section().
 fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
 fs/f2fs/segment.c |  7 ++++++
 fs/f2fs/segment.h |  2 ++
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 6a7e4148ff9d..df23824ae3c2 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
 	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
 }
 
+static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
+{
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
+
+	if (test_bit(secno, dirty_i->pinned_secmap))
+		return;
+	set_bit(secno, dirty_i->pinned_secmap);
+	dirty_i->pinned_secmap_cnt++;
+}
+
+static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
+{
+	return dirty_i->pinned_secmap_cnt;
+}
+
+static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
+						unsigned int secno)
+{
+	return pinned_section_exists(dirty_i) &&
+			test_bit(secno, dirty_i->pinned_secmap);
+}
+
+static void unpin_all_sections(struct f2fs_sb_info *sbi)
+{
+	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
+
+	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
+	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
+}
+
 /*
  * This function is called from two paths.
  * One is garbage collection and the other is SSR segment selection.
@@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
 		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
 			goto next;
 
+		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
+			goto next;
+
 		if (is_atgc) {
 			add_victim_entry(sbi, &p, segno);
 			goto next;
@@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
 	}
 
 	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
+		if (gc_type == FG_GC) {
 			f2fs_pin_file_control(inode, true);
+			pin_section(F2FS_I_SB(inode), segno);
+		}
 		err = -EAGAIN;
 		goto out;
 	}
@@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
 		goto out;
 	}
 	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
+		if (gc_type == FG_GC) {
 			f2fs_pin_file_control(inode, true);
+			pin_section(F2FS_I_SB(inode), segno);
+		}
 		err = -EAGAIN;
 		goto out;
 	}
@@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 							gc_type == FG_GC) {
 				f2fs_pin_file_control(inode, true);
 				iput(inode);
+				pin_section(sbi, segno);
 				return submitted;
 			}
 
@@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
 		ret = -EINVAL;
 		goto stop;
 	}
+retry:
 	ret = __get_victim(sbi, &segno, gc_type);
-	if (ret)
+	if (ret) {
+		/* allow to search victim from sections has pinned data */
+		if (ret == -ENODATA && gc_type == FG_GC &&
+				pinned_section_exists(DIRTY_I(sbi))) {
+			unpin_all_sections(sbi);
+			goto retry;
+		}
 		goto stop;
+	}
 
 	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
 	if (gc_type == FG_GC &&
@@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
 	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
 	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
 
+	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
+		unpin_all_sections(sbi);
+
 	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
 				get_pages(sbi, F2FS_DIRTY_NODES),
 				get_pages(sbi, F2FS_DIRTY_DENTS),
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 012524db7437..1c20d7c9eca3 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
 	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
 	if (!dirty_i->victim_secmap)
 		return -ENOMEM;
+
+	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
+	if (!dirty_i->pinned_secmap)
+		return -ENOMEM;
+
+	dirty_i->pinned_secmap_cnt = 0;
 	return 0;
 }
 
@@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
 {
 	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
 
+	kvfree(dirty_i->pinned_secmap);
 	kvfree(dirty_i->victim_secmap);
 }
 
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c94caf0c0a1..fd6f246e649c 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -294,6 +294,8 @@ struct dirty_seglist_info {
 	struct mutex seglist_lock;		/* lock for segment bitmaps */
 	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
 	unsigned long *victim_secmap;		/* background GC victims */
+	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
+	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
 };
 
 /* victim selection function for cleaning and SSR */
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
  2022-04-06 15:26 ` Chao Yu
@ 2022-04-11 20:20   ` Jaegeuk Kim
  -1 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-11 20:20 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu

On 04/06, Chao Yu wrote:
> Previously, during foreground GC, if victims contain data of pinned file,
> it will fail migration of the data, and meanwhile i_gc_failures of that
> pinned file may increase, and when it exceeds threshold, GC will unpin
> the file, result in breaking pinfile's semantics.
> 
> In order to mitigate such condition, let's record and skip section which
> has pinned file's data and give priority to select unpinned one.
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
> v3:
> - check pin status before pinning section in pin_section().
>  fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>  fs/f2fs/segment.c |  7 ++++++
>  fs/f2fs/segment.h |  2 ++
>  3 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 6a7e4148ff9d..df23824ae3c2 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>  }
>  
> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)

Need f2fs_...?

> +{
> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> +
> +	if (test_bit(secno, dirty_i->pinned_secmap))
> +		return;
> +	set_bit(secno, dirty_i->pinned_secmap);
> +	dirty_i->pinned_secmap_cnt++;
> +}
> +
> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> +{
> +	return dirty_i->pinned_secmap_cnt;
> +}
> +
> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> +						unsigned int secno)
> +{
> +	return pinned_section_exists(dirty_i) &&
> +			test_bit(secno, dirty_i->pinned_secmap);
> +}
> +
> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> +{
> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> +
> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> +}
> +
>  /*
>   * This function is called from two paths.
>   * One is garbage collection and the other is SSR segment selection.
> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>  			goto next;
>  
> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> +			goto next;
> +
>  		if (is_atgc) {
>  			add_victim_entry(sbi, &p, segno);
>  			goto next;
> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>  	}
>  
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> +		if (gc_type == FG_GC) {
>  			f2fs_pin_file_control(inode, true);
> +			pin_section(F2FS_I_SB(inode), segno);

Do we need to check unpinning the inode?
			if (!f2fs_pin_file_control())
				f2fs_set_pin_section();

> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  		goto out;
>  	}
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> +		if (gc_type == FG_GC) {
>  			f2fs_pin_file_control(inode, true);
> +			pin_section(F2FS_I_SB(inode), segno);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  							gc_type == FG_GC) {
>  				f2fs_pin_file_control(inode, true);
>  				iput(inode);
> +				pin_section(sbi, segno);

We don't have this code.

>  				return submitted;
>  			}
>  
> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  		ret = -EINVAL;
>  		goto stop;
>  	}
> +retry:
>  	ret = __get_victim(sbi, &segno, gc_type);
> -	if (ret)
> +	if (ret) {
> +		/* allow to search victim from sections has pinned data */
> +		if (ret == -ENODATA && gc_type == FG_GC &&
> +				pinned_section_exists(DIRTY_I(sbi))) {
> +			unpin_all_sections(sbi);
> +			goto retry;
> +		}
>  		goto stop;
> +	}
>  
>  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>  	if (gc_type == FG_GC &&
> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>  
> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> +		unpin_all_sections(sbi);
> +
>  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>  				get_pages(sbi, F2FS_DIRTY_NODES),
>  				get_pages(sbi, F2FS_DIRTY_DENTS),
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 012524db7437..1c20d7c9eca3 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>  	if (!dirty_i->victim_secmap)
>  		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> +	if (!dirty_i->pinned_secmap)
> +		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap_cnt = 0;
>  	return 0;
>  }
>  
> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>  {
>  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>  
> +	kvfree(dirty_i->pinned_secmap);
>  	kvfree(dirty_i->victim_secmap);
>  }
>  
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 5c94caf0c0a1..fd6f246e649c 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>  	struct mutex seglist_lock;		/* lock for segment bitmaps */
>  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>  	unsigned long *victim_secmap;		/* background GC victims */
> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>  };
>  
>  /* victim selection function for cleaning and SSR */
> -- 
> 2.32.0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-11 20:20   ` Jaegeuk Kim
  0 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-11 20:20 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 04/06, Chao Yu wrote:
> Previously, during foreground GC, if victims contain data of pinned file,
> it will fail migration of the data, and meanwhile i_gc_failures of that
> pinned file may increase, and when it exceeds threshold, GC will unpin
> the file, result in breaking pinfile's semantics.
> 
> In order to mitigate such condition, let's record and skip section which
> has pinned file's data and give priority to select unpinned one.
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
> v3:
> - check pin status before pinning section in pin_section().
>  fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>  fs/f2fs/segment.c |  7 ++++++
>  fs/f2fs/segment.h |  2 ++
>  3 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 6a7e4148ff9d..df23824ae3c2 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>  }
>  
> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)

Need f2fs_...?

> +{
> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> +
> +	if (test_bit(secno, dirty_i->pinned_secmap))
> +		return;
> +	set_bit(secno, dirty_i->pinned_secmap);
> +	dirty_i->pinned_secmap_cnt++;
> +}
> +
> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> +{
> +	return dirty_i->pinned_secmap_cnt;
> +}
> +
> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> +						unsigned int secno)
> +{
> +	return pinned_section_exists(dirty_i) &&
> +			test_bit(secno, dirty_i->pinned_secmap);
> +}
> +
> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> +{
> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> +
> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> +}
> +
>  /*
>   * This function is called from two paths.
>   * One is garbage collection and the other is SSR segment selection.
> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>  			goto next;
>  
> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> +			goto next;
> +
>  		if (is_atgc) {
>  			add_victim_entry(sbi, &p, segno);
>  			goto next;
> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>  	}
>  
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> +		if (gc_type == FG_GC) {
>  			f2fs_pin_file_control(inode, true);
> +			pin_section(F2FS_I_SB(inode), segno);

Do we need to check unpinning the inode?
			if (!f2fs_pin_file_control())
				f2fs_set_pin_section();

> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  		goto out;
>  	}
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> +		if (gc_type == FG_GC) {
>  			f2fs_pin_file_control(inode, true);
> +			pin_section(F2FS_I_SB(inode), segno);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  							gc_type == FG_GC) {
>  				f2fs_pin_file_control(inode, true);
>  				iput(inode);
> +				pin_section(sbi, segno);

We don't have this code.

>  				return submitted;
>  			}
>  
> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  		ret = -EINVAL;
>  		goto stop;
>  	}
> +retry:
>  	ret = __get_victim(sbi, &segno, gc_type);
> -	if (ret)
> +	if (ret) {
> +		/* allow to search victim from sections has pinned data */
> +		if (ret == -ENODATA && gc_type == FG_GC &&
> +				pinned_section_exists(DIRTY_I(sbi))) {
> +			unpin_all_sections(sbi);
> +			goto retry;
> +		}
>  		goto stop;
> +	}
>  
>  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>  	if (gc_type == FG_GC &&
> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>  
> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> +		unpin_all_sections(sbi);
> +
>  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>  				get_pages(sbi, F2FS_DIRTY_NODES),
>  				get_pages(sbi, F2FS_DIRTY_DENTS),
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 012524db7437..1c20d7c9eca3 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>  	if (!dirty_i->victim_secmap)
>  		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> +	if (!dirty_i->pinned_secmap)
> +		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap_cnt = 0;
>  	return 0;
>  }
>  
> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>  {
>  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>  
> +	kvfree(dirty_i->pinned_secmap);
>  	kvfree(dirty_i->victim_secmap);
>  }
>  
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 5c94caf0c0a1..fd6f246e649c 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>  	struct mutex seglist_lock;		/* lock for segment bitmaps */
>  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>  	unsigned long *victim_secmap;		/* background GC victims */
> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>  };
>  
>  /* victim selection function for cleaning and SSR */
> -- 
> 2.32.0


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
  2022-04-11 20:20   ` [f2fs-dev] " Jaegeuk Kim
@ 2022-04-11 21:25     ` Jaegeuk Kim
  -1 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-11 21:25 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 04/11, Jaegeuk Kim wrote:
> On 04/06, Chao Yu wrote:
> > Previously, during foreground GC, if victims contain data of pinned file,
> > it will fail migration of the data, and meanwhile i_gc_failures of that
> > pinned file may increase, and when it exceeds threshold, GC will unpin
> > the file, result in breaking pinfile's semantics.
> > 
> > In order to mitigate such condition, let's record and skip section which
> > has pinned file's data and give priority to select unpinned one.
> > 
> > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > ---
> > v3:
> > - check pin status before pinning section in pin_section().
> >  fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
> >  fs/f2fs/segment.c |  7 ++++++
> >  fs/f2fs/segment.h |  2 ++
> >  3 files changed, 62 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 6a7e4148ff9d..df23824ae3c2 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
> >  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
> >  }
> >  
> > +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> 
> Need f2fs_...?
> 
> > +{
> > +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > +
> > +	if (test_bit(secno, dirty_i->pinned_secmap))
> > +		return;
> > +	set_bit(secno, dirty_i->pinned_secmap);
> > +	dirty_i->pinned_secmap_cnt++;
> > +}
> > +
> > +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> > +{
> > +	return dirty_i->pinned_secmap_cnt;
> > +}
> > +
> > +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> > +						unsigned int secno)
> > +{
> > +	return pinned_section_exists(dirty_i) &&
> > +			test_bit(secno, dirty_i->pinned_secmap);
> > +}
> > +
> > +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> > +{
> > +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> > +
> > +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> > +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> > +}
> > +
> >  /*
> >   * This function is called from two paths.
> >   * One is garbage collection and the other is SSR segment selection.
> > @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
> >  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
> >  			goto next;
> >  
> > +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> > +			goto next;
> > +
> >  		if (is_atgc) {
> >  			add_victim_entry(sbi, &p, segno);
> >  			goto next;
> > @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
> >  	}
> >  
> >  	if (f2fs_is_pinned_file(inode)) {
> > -		if (gc_type == FG_GC)
> > +		if (gc_type == FG_GC) {
> >  			f2fs_pin_file_control(inode, true);
> > +			pin_section(F2FS_I_SB(inode), segno);
> 
> Do we need to check unpinning the inode?
> 			if (!f2fs_pin_file_control())
> 				f2fs_set_pin_section();
> 
> > +		}
> >  		err = -EAGAIN;
> >  		goto out;
> >  	}
> > @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
> >  		goto out;
> >  	}
> >  	if (f2fs_is_pinned_file(inode)) {
> > -		if (gc_type == FG_GC)
> > +		if (gc_type == FG_GC) {
> >  			f2fs_pin_file_control(inode, true);
> > +			pin_section(F2FS_I_SB(inode), segno);
> > +		}
> >  		err = -EAGAIN;
> >  		goto out;
> >  	}
> > @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> >  							gc_type == FG_GC) {
> >  				f2fs_pin_file_control(inode, true);
> >  				iput(inode);
> > +				pin_section(sbi, segno);
> 
> We don't have this code.

Ok, you added this in other patch.

> 
> >  				return submitted;
> >  			}
> >  
> > @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> >  		ret = -EINVAL;
> >  		goto stop;
> >  	}
> > +retry:
> >  	ret = __get_victim(sbi, &segno, gc_type);
> > -	if (ret)
> > +	if (ret) {
> > +		/* allow to search victim from sections has pinned data */
> > +		if (ret == -ENODATA && gc_type == FG_GC &&
> > +				pinned_section_exists(DIRTY_I(sbi))) {
> > +			unpin_all_sections(sbi);
> > +			goto retry;
> > +		}
> >  		goto stop;
> > +	}
> >  
> >  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
> >  	if (gc_type == FG_GC &&
> > @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> >  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
> >  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> >  
> > +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> > +		unpin_all_sections(sbi);
> > +
> >  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
> >  				get_pages(sbi, F2FS_DIRTY_NODES),
> >  				get_pages(sbi, F2FS_DIRTY_DENTS),
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 012524db7437..1c20d7c9eca3 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
> >  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> >  	if (!dirty_i->victim_secmap)
> >  		return -ENOMEM;
> > +
> > +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > +	if (!dirty_i->pinned_secmap)
> > +		return -ENOMEM;
> > +
> > +	dirty_i->pinned_secmap_cnt = 0;
> >  	return 0;
> >  }
> >  
> > @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
> >  {
> >  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> >  
> > +	kvfree(dirty_i->pinned_secmap);
> >  	kvfree(dirty_i->victim_secmap);
> >  }
> >  
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 5c94caf0c0a1..fd6f246e649c 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -294,6 +294,8 @@ struct dirty_seglist_info {
> >  	struct mutex seglist_lock;		/* lock for segment bitmaps */
> >  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
> >  	unsigned long *victim_secmap;		/* background GC victims */
> > +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> > +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> >  };
> >  
> >  /* victim selection function for cleaning and SSR */
> > -- 
> > 2.32.0
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-11 21:25     ` Jaegeuk Kim
  0 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-11 21:25 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 04/11, Jaegeuk Kim wrote:
> On 04/06, Chao Yu wrote:
> > Previously, during foreground GC, if victims contain data of pinned file,
> > it will fail migration of the data, and meanwhile i_gc_failures of that
> > pinned file may increase, and when it exceeds threshold, GC will unpin
> > the file, result in breaking pinfile's semantics.
> > 
> > In order to mitigate such condition, let's record and skip section which
> > has pinned file's data and give priority to select unpinned one.
> > 
> > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > ---
> > v3:
> > - check pin status before pinning section in pin_section().
> >  fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
> >  fs/f2fs/segment.c |  7 ++++++
> >  fs/f2fs/segment.h |  2 ++
> >  3 files changed, 62 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index 6a7e4148ff9d..df23824ae3c2 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
> >  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
> >  }
> >  
> > +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> 
> Need f2fs_...?
> 
> > +{
> > +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > +
> > +	if (test_bit(secno, dirty_i->pinned_secmap))
> > +		return;
> > +	set_bit(secno, dirty_i->pinned_secmap);
> > +	dirty_i->pinned_secmap_cnt++;
> > +}
> > +
> > +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> > +{
> > +	return dirty_i->pinned_secmap_cnt;
> > +}
> > +
> > +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> > +						unsigned int secno)
> > +{
> > +	return pinned_section_exists(dirty_i) &&
> > +			test_bit(secno, dirty_i->pinned_secmap);
> > +}
> > +
> > +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> > +{
> > +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> > +
> > +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> > +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> > +}
> > +
> >  /*
> >   * This function is called from two paths.
> >   * One is garbage collection and the other is SSR segment selection.
> > @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
> >  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
> >  			goto next;
> >  
> > +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> > +			goto next;
> > +
> >  		if (is_atgc) {
> >  			add_victim_entry(sbi, &p, segno);
> >  			goto next;
> > @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
> >  	}
> >  
> >  	if (f2fs_is_pinned_file(inode)) {
> > -		if (gc_type == FG_GC)
> > +		if (gc_type == FG_GC) {
> >  			f2fs_pin_file_control(inode, true);
> > +			pin_section(F2FS_I_SB(inode), segno);
> 
> Do we need to check unpinning the inode?
> 			if (!f2fs_pin_file_control())
> 				f2fs_set_pin_section();
> 
> > +		}
> >  		err = -EAGAIN;
> >  		goto out;
> >  	}
> > @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
> >  		goto out;
> >  	}
> >  	if (f2fs_is_pinned_file(inode)) {
> > -		if (gc_type == FG_GC)
> > +		if (gc_type == FG_GC) {
> >  			f2fs_pin_file_control(inode, true);
> > +			pin_section(F2FS_I_SB(inode), segno);
> > +		}
> >  		err = -EAGAIN;
> >  		goto out;
> >  	}
> > @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> >  							gc_type == FG_GC) {
> >  				f2fs_pin_file_control(inode, true);
> >  				iput(inode);
> > +				pin_section(sbi, segno);
> 
> We don't have this code.

Ok, you added this in other patch.

> 
> >  				return submitted;
> >  			}
> >  
> > @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> >  		ret = -EINVAL;
> >  		goto stop;
> >  	}
> > +retry:
> >  	ret = __get_victim(sbi, &segno, gc_type);
> > -	if (ret)
> > +	if (ret) {
> > +		/* allow to search victim from sections has pinned data */
> > +		if (ret == -ENODATA && gc_type == FG_GC &&
> > +				pinned_section_exists(DIRTY_I(sbi))) {
> > +			unpin_all_sections(sbi);
> > +			goto retry;
> > +		}
> >  		goto stop;
> > +	}
> >  
> >  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
> >  	if (gc_type == FG_GC &&
> > @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> >  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
> >  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> >  
> > +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> > +		unpin_all_sections(sbi);
> > +
> >  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
> >  				get_pages(sbi, F2FS_DIRTY_NODES),
> >  				get_pages(sbi, F2FS_DIRTY_DENTS),
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 012524db7437..1c20d7c9eca3 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
> >  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> >  	if (!dirty_i->victim_secmap)
> >  		return -ENOMEM;
> > +
> > +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > +	if (!dirty_i->pinned_secmap)
> > +		return -ENOMEM;
> > +
> > +	dirty_i->pinned_secmap_cnt = 0;
> >  	return 0;
> >  }
> >  
> > @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
> >  {
> >  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> >  
> > +	kvfree(dirty_i->pinned_secmap);
> >  	kvfree(dirty_i->victim_secmap);
> >  }
> >  
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 5c94caf0c0a1..fd6f246e649c 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -294,6 +294,8 @@ struct dirty_seglist_info {
> >  	struct mutex seglist_lock;		/* lock for segment bitmaps */
> >  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
> >  	unsigned long *victim_secmap;		/* background GC victims */
> > +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> > +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> >  };
> >  
> >  /* victim selection function for cleaning and SSR */
> > -- 
> > 2.32.0
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
  2022-04-11 20:20   ` [f2fs-dev] " Jaegeuk Kim
@ 2022-04-12  3:54     ` Chao Yu
  -1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-12  3:54 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu

On 2022/4/12 4:20, Jaegeuk Kim wrote:
> On 04/06, Chao Yu wrote:
>> Previously, during foreground GC, if victims contain data of pinned file,
>> it will fail migration of the data, and meanwhile i_gc_failures of that
>> pinned file may increase, and when it exceeds threshold, GC will unpin
>> the file, result in breaking pinfile's semantics.
>>
>> In order to mitigate such condition, let's record and skip section which
>> has pinned file's data and give priority to select unpinned one.
>>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>> v3:
>> - check pin status before pinning section in pin_section().
>>   fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>>   fs/f2fs/segment.c |  7 ++++++
>>   fs/f2fs/segment.h |  2 ++
>>   3 files changed, 62 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6a7e4148ff9d..df23824ae3c2 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>   }
>>   
>> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> 
> Need f2fs_...?

Sure, I can add prefix...

It's a local function, it won't pollute global namespace w/o f2fs_ prefix
though.

> 
>> +{
>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>> +
>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>> +		return;
>> +	set_bit(secno, dirty_i->pinned_secmap);
>> +	dirty_i->pinned_secmap_cnt++;
>> +}
>> +
>> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
>> +{
>> +	return dirty_i->pinned_secmap_cnt;
>> +}
>> +
>> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
>> +						unsigned int secno)
>> +{
>> +	return pinned_section_exists(dirty_i) &&
>> +			test_bit(secno, dirty_i->pinned_secmap);
>> +}
>> +
>> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
>> +{
>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>> +
>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>> +}
>> +
>>   /*
>>    * This function is called from two paths.
>>    * One is garbage collection and the other is SSR segment selection.
>> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>   			goto next;
>>   
>> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
>> +			goto next;
>> +
>>   		if (is_atgc) {
>>   			add_victim_entry(sbi, &p, segno);
>>   			goto next;
>> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>   	}
>>   
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> +		if (gc_type == FG_GC) {
>>   			f2fs_pin_file_control(inode, true);
>> +			pin_section(F2FS_I_SB(inode), segno);
> 
> Do we need to check unpinning the inode?
> 			if (!f2fs_pin_file_control())
> 				f2fs_set_pin_section();

I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
so could you please check below logic:

 From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
From: Chao Yu <chao@kernel.org>
Date: Wed, 6 Apr 2022 23:26:51 +0800
Subject: [PATCH v4] f2fs: give priority to select unpinned section for
  foreground GC

Previously, during foreground GC, if victims contain data of pinned file,
it will fail migration of the data, and meanwhile i_gc_failures of that
pinned file may increase, and when it exceeds threshold, GC will unpin
the file, result in breaking pinfile's semantics.

In order to mitigate such condition, let's record and skip section which
has pinned file's data and give priority to select unpinned one.

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
v4:
- add f2fs_ prefix for newly introduced functions
- add bool type variable for functionality switch
- increase GC_FAILURE_PIN only if it disable pinning section
  fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
  fs/f2fs/segment.c |  8 ++++++
  fs/f2fs/segment.h |  3 +++
  3 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 6a7e4148ff9d..296b31e28d3d 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
  }

+static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
+{
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
+
+	if (!dirty_i->enable_pin_section)
+		return false;
+	if (test_bit(secno, dirty_i->pinned_secmap))
+		return true;
+	set_bit(secno, dirty_i->pinned_secmap);
+	dirty_i->pinned_secmap_cnt++;
+	return true;
+}
+
+static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
+{
+	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
+}
+
+static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
+						unsigned int secno)
+{
+	return f2fs_pinned_section_exists(dirty_i) &&
+			test_bit(secno, dirty_i->pinned_secmap);
+}
+
+static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
+{
+	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
+
+	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
+	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
+	DIRTY_I(sbi)->enable_pin_section = enable;
+}
+
  /*
   * This function is called from two paths.
   * One is garbage collection and the other is SSR segment selection.
@@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
  			goto next;

+		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
+			goto next;
+
  		if (is_atgc) {
  			add_victim_entry(sbi, &p, segno);
  			goto next;
@@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
  	}

  	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
-			f2fs_pin_file_control(inode, true);
+		if (gc_type == FG_GC) {
+			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
+				f2fs_pin_file_control(inode, true);
+		}
  		err = -EAGAIN;
  		goto out;
  	}
@@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
  		goto out;
  	}
  	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
-			f2fs_pin_file_control(inode, true);
+		if (gc_type == FG_GC) {
+			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
+				f2fs_pin_file_control(inode, true);
+		}
  		err = -EAGAIN;
  		goto out;
  	}
@@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,

  			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
  							gc_type == FG_GC) {
-				f2fs_pin_file_control(inode, true);
+				if (!f2fs_pin_section(sbi, segno))
+					f2fs_pin_file_control(inode, true);
  				iput(inode);
  				return submitted;
  			}
@@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
  		ret = -EINVAL;
  		goto stop;
  	}
+retry:
  	ret = __get_victim(sbi, &segno, gc_type);
-	if (ret)
+	if (ret) {
+		/* allow to search victim from sections has pinned data */
+		if (ret == -ENODATA && gc_type == FG_GC &&
+				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
+			f2fs_unpin_all_sections(sbi, false);
+			goto retry;
+		}
  		goto stop;
+	}

  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
  	if (gc_type == FG_GC &&
@@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;

+	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
+		f2fs_unpin_all_sections(sbi, true);
+
  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
  				get_pages(sbi, F2FS_DIRTY_NODES),
  				get_pages(sbi, F2FS_DIRTY_DENTS),
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 22dfeb991529..93c7bae57a25 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
  	if (!dirty_i->victim_secmap)
  		return -ENOMEM;
+
+	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
+	if (!dirty_i->pinned_secmap)
+		return -ENOMEM;
+
+	dirty_i->pinned_secmap_cnt = 0;
+	dirty_i->enable_pin_section = true;
  	return 0;
  }

@@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
  {
  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);

+	kvfree(dirty_i->pinned_secmap);
  	kvfree(dirty_i->victim_secmap);
  }

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c94caf0c0a1..8a591455d796 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -294,6 +294,9 @@ struct dirty_seglist_info {
  	struct mutex seglist_lock;		/* lock for segment bitmaps */
  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
  	unsigned long *victim_secmap;		/* background GC victims */
+	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
+	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
+	bool enable_pin_section;		/* enable pinning section */
  };

  /* victim selection function for cleaning and SSR */
-- 
2.25.1

Thanks,

> 
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>   		goto out;
>>   	}
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> +		if (gc_type == FG_GC) {
>>   			f2fs_pin_file_control(inode, true);
>> +			pin_section(F2FS_I_SB(inode), segno);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>   							gc_type == FG_GC) {
>>   				f2fs_pin_file_control(inode, true);
>>   				iput(inode);
>> +				pin_section(sbi, segno);
> 
> We don't have this code.
> 
>>   				return submitted;
>>   			}
>>   
>> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   		ret = -EINVAL;
>>   		goto stop;
>>   	}
>> +retry:
>>   	ret = __get_victim(sbi, &segno, gc_type);
>> -	if (ret)
>> +	if (ret) {
>> +		/* allow to search victim from sections has pinned data */
>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>> +				pinned_section_exists(DIRTY_I(sbi))) {
>> +			unpin_all_sections(sbi);
>> +			goto retry;
>> +		}
>>   		goto stop;
>> +	}
>>   
>>   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>   	if (gc_type == FG_GC &&
>> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>   
>> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
>> +		unpin_all_sections(sbi);
>> +
>>   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>   				get_pages(sbi, F2FS_DIRTY_NODES),
>>   				get_pages(sbi, F2FS_DIRTY_DENTS),
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 012524db7437..1c20d7c9eca3 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>   	if (!dirty_i->victim_secmap)
>>   		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> +	if (!dirty_i->pinned_secmap)
>> +		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap_cnt = 0;
>>   	return 0;
>>   }
>>   
>> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>   {
>>   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>   
>> +	kvfree(dirty_i->pinned_secmap);
>>   	kvfree(dirty_i->victim_secmap);
>>   }
>>   
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 5c94caf0c0a1..fd6f246e649c 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>>   	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>   	unsigned long *victim_secmap;		/* background GC victims */
>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>>   };
>>   
>>   /* victim selection function for cleaning and SSR */
>> -- 
>> 2.32.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-12  3:54     ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-12  3:54 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2022/4/12 4:20, Jaegeuk Kim wrote:
> On 04/06, Chao Yu wrote:
>> Previously, during foreground GC, if victims contain data of pinned file,
>> it will fail migration of the data, and meanwhile i_gc_failures of that
>> pinned file may increase, and when it exceeds threshold, GC will unpin
>> the file, result in breaking pinfile's semantics.
>>
>> In order to mitigate such condition, let's record and skip section which
>> has pinned file's data and give priority to select unpinned one.
>>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>> v3:
>> - check pin status before pinning section in pin_section().
>>   fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>>   fs/f2fs/segment.c |  7 ++++++
>>   fs/f2fs/segment.h |  2 ++
>>   3 files changed, 62 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6a7e4148ff9d..df23824ae3c2 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>   }
>>   
>> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> 
> Need f2fs_...?

Sure, I can add prefix...

It's a local function, it won't pollute global namespace w/o f2fs_ prefix
though.

> 
>> +{
>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>> +
>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>> +		return;
>> +	set_bit(secno, dirty_i->pinned_secmap);
>> +	dirty_i->pinned_secmap_cnt++;
>> +}
>> +
>> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
>> +{
>> +	return dirty_i->pinned_secmap_cnt;
>> +}
>> +
>> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
>> +						unsigned int secno)
>> +{
>> +	return pinned_section_exists(dirty_i) &&
>> +			test_bit(secno, dirty_i->pinned_secmap);
>> +}
>> +
>> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
>> +{
>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>> +
>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>> +}
>> +
>>   /*
>>    * This function is called from two paths.
>>    * One is garbage collection and the other is SSR segment selection.
>> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>   			goto next;
>>   
>> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
>> +			goto next;
>> +
>>   		if (is_atgc) {
>>   			add_victim_entry(sbi, &p, segno);
>>   			goto next;
>> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>   	}
>>   
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> +		if (gc_type == FG_GC) {
>>   			f2fs_pin_file_control(inode, true);
>> +			pin_section(F2FS_I_SB(inode), segno);
> 
> Do we need to check unpinning the inode?
> 			if (!f2fs_pin_file_control())
> 				f2fs_set_pin_section();

I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
so could you please check below logic:

 From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
From: Chao Yu <chao@kernel.org>
Date: Wed, 6 Apr 2022 23:26:51 +0800
Subject: [PATCH v4] f2fs: give priority to select unpinned section for
  foreground GC

Previously, during foreground GC, if victims contain data of pinned file,
it will fail migration of the data, and meanwhile i_gc_failures of that
pinned file may increase, and when it exceeds threshold, GC will unpin
the file, result in breaking pinfile's semantics.

In order to mitigate such condition, let's record and skip section which
has pinned file's data and give priority to select unpinned one.

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
v4:
- add f2fs_ prefix for newly introduced functions
- add bool type variable for functionality switch
- increase GC_FAILURE_PIN only if it disable pinning section
  fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
  fs/f2fs/segment.c |  8 ++++++
  fs/f2fs/segment.h |  3 +++
  3 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 6a7e4148ff9d..296b31e28d3d 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
  }

+static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
+{
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
+
+	if (!dirty_i->enable_pin_section)
+		return false;
+	if (test_bit(secno, dirty_i->pinned_secmap))
+		return true;
+	set_bit(secno, dirty_i->pinned_secmap);
+	dirty_i->pinned_secmap_cnt++;
+	return true;
+}
+
+static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
+{
+	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
+}
+
+static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
+						unsigned int secno)
+{
+	return f2fs_pinned_section_exists(dirty_i) &&
+			test_bit(secno, dirty_i->pinned_secmap);
+}
+
+static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
+{
+	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
+
+	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
+	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
+	DIRTY_I(sbi)->enable_pin_section = enable;
+}
+
  /*
   * This function is called from two paths.
   * One is garbage collection and the other is SSR segment selection.
@@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
  			goto next;

+		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
+			goto next;
+
  		if (is_atgc) {
  			add_victim_entry(sbi, &p, segno);
  			goto next;
@@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
  	}

  	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
-			f2fs_pin_file_control(inode, true);
+		if (gc_type == FG_GC) {
+			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
+				f2fs_pin_file_control(inode, true);
+		}
  		err = -EAGAIN;
  		goto out;
  	}
@@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
  		goto out;
  	}
  	if (f2fs_is_pinned_file(inode)) {
-		if (gc_type == FG_GC)
-			f2fs_pin_file_control(inode, true);
+		if (gc_type == FG_GC) {
+			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
+				f2fs_pin_file_control(inode, true);
+		}
  		err = -EAGAIN;
  		goto out;
  	}
@@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,

  			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
  							gc_type == FG_GC) {
-				f2fs_pin_file_control(inode, true);
+				if (!f2fs_pin_section(sbi, segno))
+					f2fs_pin_file_control(inode, true);
  				iput(inode);
  				return submitted;
  			}
@@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
  		ret = -EINVAL;
  		goto stop;
  	}
+retry:
  	ret = __get_victim(sbi, &segno, gc_type);
-	if (ret)
+	if (ret) {
+		/* allow to search victim from sections has pinned data */
+		if (ret == -ENODATA && gc_type == FG_GC &&
+				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
+			f2fs_unpin_all_sections(sbi, false);
+			goto retry;
+		}
  		goto stop;
+	}

  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
  	if (gc_type == FG_GC &&
@@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;

+	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
+		f2fs_unpin_all_sections(sbi, true);
+
  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
  				get_pages(sbi, F2FS_DIRTY_NODES),
  				get_pages(sbi, F2FS_DIRTY_DENTS),
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 22dfeb991529..93c7bae57a25 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
  	if (!dirty_i->victim_secmap)
  		return -ENOMEM;
+
+	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
+	if (!dirty_i->pinned_secmap)
+		return -ENOMEM;
+
+	dirty_i->pinned_secmap_cnt = 0;
+	dirty_i->enable_pin_section = true;
  	return 0;
  }

@@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
  {
  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);

+	kvfree(dirty_i->pinned_secmap);
  	kvfree(dirty_i->victim_secmap);
  }

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5c94caf0c0a1..8a591455d796 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -294,6 +294,9 @@ struct dirty_seglist_info {
  	struct mutex seglist_lock;		/* lock for segment bitmaps */
  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
  	unsigned long *victim_secmap;		/* background GC victims */
+	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
+	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
+	bool enable_pin_section;		/* enable pinning section */
  };

  /* victim selection function for cleaning and SSR */
-- 
2.25.1

Thanks,

> 
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>   		goto out;
>>   	}
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> +		if (gc_type == FG_GC) {
>>   			f2fs_pin_file_control(inode, true);
>> +			pin_section(F2FS_I_SB(inode), segno);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>   							gc_type == FG_GC) {
>>   				f2fs_pin_file_control(inode, true);
>>   				iput(inode);
>> +				pin_section(sbi, segno);
> 
> We don't have this code.
> 
>>   				return submitted;
>>   			}
>>   
>> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   		ret = -EINVAL;
>>   		goto stop;
>>   	}
>> +retry:
>>   	ret = __get_victim(sbi, &segno, gc_type);
>> -	if (ret)
>> +	if (ret) {
>> +		/* allow to search victim from sections has pinned data */
>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>> +				pinned_section_exists(DIRTY_I(sbi))) {
>> +			unpin_all_sections(sbi);
>> +			goto retry;
>> +		}
>>   		goto stop;
>> +	}
>>   
>>   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>   	if (gc_type == FG_GC &&
>> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>   
>> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
>> +		unpin_all_sections(sbi);
>> +
>>   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>   				get_pages(sbi, F2FS_DIRTY_NODES),
>>   				get_pages(sbi, F2FS_DIRTY_DENTS),
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 012524db7437..1c20d7c9eca3 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>   	if (!dirty_i->victim_secmap)
>>   		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> +	if (!dirty_i->pinned_secmap)
>> +		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap_cnt = 0;
>>   	return 0;
>>   }
>>   
>> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>   {
>>   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>   
>> +	kvfree(dirty_i->pinned_secmap);
>>   	kvfree(dirty_i->victim_secmap);
>>   }
>>   
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 5c94caf0c0a1..fd6f246e649c 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>>   	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>   	unsigned long *victim_secmap;		/* background GC victims */
>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>>   };
>>   
>>   /* victim selection function for cleaning and SSR */
>> -- 
>> 2.32.0


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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
  2022-04-12  3:54     ` [f2fs-dev] " Chao Yu
@ 2022-04-12 16:21       ` Jaegeuk Kim
  -1 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-12 16:21 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu

On 04/12, Chao Yu wrote:
> On 2022/4/12 4:20, Jaegeuk Kim wrote:
> > On 04/06, Chao Yu wrote:
> > > Previously, during foreground GC, if victims contain data of pinned file,
> > > it will fail migration of the data, and meanwhile i_gc_failures of that
> > > pinned file may increase, and when it exceeds threshold, GC will unpin
> > > the file, result in breaking pinfile's semantics.
> > > 
> > > In order to mitigate such condition, let's record and skip section which
> > > has pinned file's data and give priority to select unpinned one.
> > > 
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > > v3:
> > > - check pin status before pinning section in pin_section().
> > >   fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
> > >   fs/f2fs/segment.c |  7 ++++++
> > >   fs/f2fs/segment.h |  2 ++
> > >   3 files changed, 62 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > index 6a7e4148ff9d..df23824ae3c2 100644
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
> > >   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
> > >   }
> > > +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> > 
> > Need f2fs_...?
> 
> Sure, I can add prefix...
> 
> It's a local function, it won't pollute global namespace w/o f2fs_ prefix
> though.
> 
> > 
> > > +{
> > > +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > > +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > > +
> > > +	if (test_bit(secno, dirty_i->pinned_secmap))
> > > +		return;
> > > +	set_bit(secno, dirty_i->pinned_secmap);
> > > +	dirty_i->pinned_secmap_cnt++;
> > > +}
> > > +
> > > +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> > > +{
> > > +	return dirty_i->pinned_secmap_cnt;
> > > +}
> > > +
> > > +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> > > +						unsigned int secno)
> > > +{
> > > +	return pinned_section_exists(dirty_i) &&
> > > +			test_bit(secno, dirty_i->pinned_secmap);
> > > +}
> > > +
> > > +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> > > +{
> > > +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> > > +
> > > +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> > > +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> > > +}
> > > +
> > >   /*
> > >    * This function is called from two paths.
> > >    * One is garbage collection and the other is SSR segment selection.
> > > @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
> > >   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
> > >   			goto next;
> > > +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> > > +			goto next;
> > > +
> > >   		if (is_atgc) {
> > >   			add_victim_entry(sbi, &p, segno);
> > >   			goto next;
> > > @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
> > >   	}
> > >   	if (f2fs_is_pinned_file(inode)) {
> > > -		if (gc_type == FG_GC)
> > > +		if (gc_type == FG_GC) {
> > >   			f2fs_pin_file_control(inode, true);
> > > +			pin_section(F2FS_I_SB(inode), segno);
> > 
> > Do we need to check unpinning the inode?
> > 			if (!f2fs_pin_file_control())
> > 				f2fs_set_pin_section();
> 
> I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
> so could you please check below logic:
> 
> From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao@kernel.org>
> Date: Wed, 6 Apr 2022 23:26:51 +0800
> Subject: [PATCH v4] f2fs: give priority to select unpinned section for
>  foreground GC
> 
> Previously, during foreground GC, if victims contain data of pinned file,
> it will fail migration of the data, and meanwhile i_gc_failures of that
> pinned file may increase, and when it exceeds threshold, GC will unpin
> the file, result in breaking pinfile's semantics.
> 
> In order to mitigate such condition, let's record and skip section which
> has pinned file's data and give priority to select unpinned one.
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
> v4:
> - add f2fs_ prefix for newly introduced functions
> - add bool type variable for functionality switch
> - increase GC_FAILURE_PIN only if it disable pinning section
>  fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
>  fs/f2fs/segment.c |  8 ++++++
>  fs/f2fs/segment.h |  3 +++
>  3 files changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 6a7e4148ff9d..296b31e28d3d 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>  }
> 
> +static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> +{
> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> +
> +	if (!dirty_i->enable_pin_section)
> +		return false;
> +	if (test_bit(secno, dirty_i->pinned_secmap))
> +		return true;
> +	set_bit(secno, dirty_i->pinned_secmap);
> +	dirty_i->pinned_secmap_cnt++;

	if (!test_and_set_bit())
		dirty_i->pinned_secmap_cnt++;

> +	return true;
> +}
> +
> +static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
> +{
> +	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
> +}
> +
> +static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
> +						unsigned int secno)
> +{
> +	return f2fs_pinned_section_exists(dirty_i) &&
> +			test_bit(secno, dirty_i->pinned_secmap);
> +}
> +
> +static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
> +{
> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> +
> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> +	DIRTY_I(sbi)->enable_pin_section = enable;
> +}
> +
>  /*
>   * This function is called from two paths.
>   * One is garbage collection and the other is SSR segment selection.
> @@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>  			goto next;
> 
> +		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
> +			goto next;
> +
>  		if (is_atgc) {
>  			add_victim_entry(sbi, &p, segno);
>  			goto next;
> @@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>  	}
> 

Can we make a generic function?

f2fs_gc_pinned_control()
{
	if (!f2fs_is_pinned_file(inode))
		return 0;
	if (gc_type != FG_GC)
		return 0;
	if (!f2fs_pin_section())
		f2fs_pin_file_control();
	return -EAGAIN;
}

>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> -			f2fs_pin_file_control(inode, true);
> +		if (gc_type == FG_GC) {
> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))

> +				f2fs_pin_file_control(inode, true);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  		goto out;
>  	}
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> -			f2fs_pin_file_control(inode, true);
> +		if (gc_type == FG_GC) {
> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
> +				f2fs_pin_file_control(inode, true);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> 
>  			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
>  							gc_type == FG_GC) {
> -				f2fs_pin_file_control(inode, true);
> +				if (!f2fs_pin_section(sbi, segno))
> +					f2fs_pin_file_control(inode, true);
>  				iput(inode);
>  				return submitted;
>  			}
> @@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  		ret = -EINVAL;
>  		goto stop;
>  	}
> +retry:
>  	ret = __get_victim(sbi, &segno, gc_type);
> -	if (ret)
> +	if (ret) {
> +		/* allow to search victim from sections has pinned data */
> +		if (ret == -ENODATA && gc_type == FG_GC &&
> +				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
> +			f2fs_unpin_all_sections(sbi, false);
> +			goto retry;
> +		}
>  		goto stop;
> +	}
> 
>  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>  	if (gc_type == FG_GC &&
> @@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> 
> +	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
> +		f2fs_unpin_all_sections(sbi, true);
> +
>  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>  				get_pages(sbi, F2FS_DIRTY_NODES),
>  				get_pages(sbi, F2FS_DIRTY_DENTS),
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 22dfeb991529..93c7bae57a25 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>  	if (!dirty_i->victim_secmap)
>  		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> +	if (!dirty_i->pinned_secmap)
> +		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap_cnt = 0;
> +	dirty_i->enable_pin_section = true;
>  	return 0;
>  }
> 
> @@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>  {
>  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> 
> +	kvfree(dirty_i->pinned_secmap);
>  	kvfree(dirty_i->victim_secmap);
>  }
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 5c94caf0c0a1..8a591455d796 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -294,6 +294,9 @@ struct dirty_seglist_info {
>  	struct mutex seglist_lock;		/* lock for segment bitmaps */
>  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>  	unsigned long *victim_secmap;		/* background GC victims */
> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> +	bool enable_pin_section;		/* enable pinning section */
>  };
> 
>  /* victim selection function for cleaning and SSR */
> -- 
> 2.25.1
> 
> Thanks,
> 
> > 
> > > +		}
> > >   		err = -EAGAIN;
> > >   		goto out;
> > >   	}
> > > @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
> > >   		goto out;
> > >   	}
> > >   	if (f2fs_is_pinned_file(inode)) {
> > > -		if (gc_type == FG_GC)
> > > +		if (gc_type == FG_GC) {
> > >   			f2fs_pin_file_control(inode, true);
> > > +			pin_section(F2FS_I_SB(inode), segno);
> > > +		}
> > >   		err = -EAGAIN;
> > >   		goto out;
> > >   	}
> > > @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > >   							gc_type == FG_GC) {
> > >   				f2fs_pin_file_control(inode, true);
> > >   				iput(inode);
> > > +				pin_section(sbi, segno);
> > 
> > We don't have this code.
> > 
> > >   				return submitted;
> > >   			}
> > > @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> > >   		ret = -EINVAL;
> > >   		goto stop;
> > >   	}
> > > +retry:
> > >   	ret = __get_victim(sbi, &segno, gc_type);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		/* allow to search victim from sections has pinned data */
> > > +		if (ret == -ENODATA && gc_type == FG_GC &&
> > > +				pinned_section_exists(DIRTY_I(sbi))) {
> > > +			unpin_all_sections(sbi);
> > > +			goto retry;
> > > +		}
> > >   		goto stop;
> > > +	}
> > >   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
> > >   	if (gc_type == FG_GC &&
> > > @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> > >   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
> > >   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> > > +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> > > +		unpin_all_sections(sbi);
> > > +
> > >   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
> > >   				get_pages(sbi, F2FS_DIRTY_NODES),
> > >   				get_pages(sbi, F2FS_DIRTY_DENTS),
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 012524db7437..1c20d7c9eca3 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
> > >   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > >   	if (!dirty_i->victim_secmap)
> > >   		return -ENOMEM;
> > > +
> > > +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > > +	if (!dirty_i->pinned_secmap)
> > > +		return -ENOMEM;
> > > +
> > > +	dirty_i->pinned_secmap_cnt = 0;
> > >   	return 0;
> > >   }
> > > @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
> > >   {
> > >   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > > +	kvfree(dirty_i->pinned_secmap);
> > >   	kvfree(dirty_i->victim_secmap);
> > >   }
> > > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > > index 5c94caf0c0a1..fd6f246e649c 100644
> > > --- a/fs/f2fs/segment.h
> > > +++ b/fs/f2fs/segment.h
> > > @@ -294,6 +294,8 @@ struct dirty_seglist_info {
> > >   	struct mutex seglist_lock;		/* lock for segment bitmaps */
> > >   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
> > >   	unsigned long *victim_secmap;		/* background GC victims */
> > > +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> > > +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> > >   };
> > >   /* victim selection function for cleaning and SSR */
> > > -- 
> > > 2.32.0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-12 16:21       ` Jaegeuk Kim
  0 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2022-04-12 16:21 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 04/12, Chao Yu wrote:
> On 2022/4/12 4:20, Jaegeuk Kim wrote:
> > On 04/06, Chao Yu wrote:
> > > Previously, during foreground GC, if victims contain data of pinned file,
> > > it will fail migration of the data, and meanwhile i_gc_failures of that
> > > pinned file may increase, and when it exceeds threshold, GC will unpin
> > > the file, result in breaking pinfile's semantics.
> > > 
> > > In order to mitigate such condition, let's record and skip section which
> > > has pinned file's data and give priority to select unpinned one.
> > > 
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > > v3:
> > > - check pin status before pinning section in pin_section().
> > >   fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
> > >   fs/f2fs/segment.c |  7 ++++++
> > >   fs/f2fs/segment.h |  2 ++
> > >   3 files changed, 62 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > index 6a7e4148ff9d..df23824ae3c2 100644
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
> > >   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
> > >   }
> > > +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> > 
> > Need f2fs_...?
> 
> Sure, I can add prefix...
> 
> It's a local function, it won't pollute global namespace w/o f2fs_ prefix
> though.
> 
> > 
> > > +{
> > > +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > > +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> > > +
> > > +	if (test_bit(secno, dirty_i->pinned_secmap))
> > > +		return;
> > > +	set_bit(secno, dirty_i->pinned_secmap);
> > > +	dirty_i->pinned_secmap_cnt++;
> > > +}
> > > +
> > > +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
> > > +{
> > > +	return dirty_i->pinned_secmap_cnt;
> > > +}
> > > +
> > > +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
> > > +						unsigned int secno)
> > > +{
> > > +	return pinned_section_exists(dirty_i) &&
> > > +			test_bit(secno, dirty_i->pinned_secmap);
> > > +}
> > > +
> > > +static void unpin_all_sections(struct f2fs_sb_info *sbi)
> > > +{
> > > +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> > > +
> > > +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> > > +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> > > +}
> > > +
> > >   /*
> > >    * This function is called from two paths.
> > >    * One is garbage collection and the other is SSR segment selection.
> > > @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
> > >   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
> > >   			goto next;
> > > +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
> > > +			goto next;
> > > +
> > >   		if (is_atgc) {
> > >   			add_victim_entry(sbi, &p, segno);
> > >   			goto next;
> > > @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
> > >   	}
> > >   	if (f2fs_is_pinned_file(inode)) {
> > > -		if (gc_type == FG_GC)
> > > +		if (gc_type == FG_GC) {
> > >   			f2fs_pin_file_control(inode, true);
> > > +			pin_section(F2FS_I_SB(inode), segno);
> > 
> > Do we need to check unpinning the inode?
> > 			if (!f2fs_pin_file_control())
> > 				f2fs_set_pin_section();
> 
> I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
> so could you please check below logic:
> 
> From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao@kernel.org>
> Date: Wed, 6 Apr 2022 23:26:51 +0800
> Subject: [PATCH v4] f2fs: give priority to select unpinned section for
>  foreground GC
> 
> Previously, during foreground GC, if victims contain data of pinned file,
> it will fail migration of the data, and meanwhile i_gc_failures of that
> pinned file may increase, and when it exceeds threshold, GC will unpin
> the file, result in breaking pinfile's semantics.
> 
> In order to mitigate such condition, let's record and skip section which
> has pinned file's data and give priority to select unpinned one.
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
> v4:
> - add f2fs_ prefix for newly introduced functions
> - add bool type variable for functionality switch
> - increase GC_FAILURE_PIN only if it disable pinning section
>  fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
>  fs/f2fs/segment.c |  8 ++++++
>  fs/f2fs/segment.h |  3 +++
>  3 files changed, 71 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 6a7e4148ff9d..296b31e28d3d 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>  	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>  }
> 
> +static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
> +{
> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
> +
> +	if (!dirty_i->enable_pin_section)
> +		return false;
> +	if (test_bit(secno, dirty_i->pinned_secmap))
> +		return true;
> +	set_bit(secno, dirty_i->pinned_secmap);
> +	dirty_i->pinned_secmap_cnt++;

	if (!test_and_set_bit())
		dirty_i->pinned_secmap_cnt++;

> +	return true;
> +}
> +
> +static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
> +{
> +	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
> +}
> +
> +static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
> +						unsigned int secno)
> +{
> +	return f2fs_pinned_section_exists(dirty_i) &&
> +			test_bit(secno, dirty_i->pinned_secmap);
> +}
> +
> +static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
> +{
> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
> +
> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
> +	DIRTY_I(sbi)->enable_pin_section = enable;
> +}
> +
>  /*
>   * This function is called from two paths.
>   * One is garbage collection and the other is SSR segment selection.
> @@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>  		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>  			goto next;
> 
> +		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
> +			goto next;
> +
>  		if (is_atgc) {
>  			add_victim_entry(sbi, &p, segno);
>  			goto next;
> @@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>  	}
> 

Can we make a generic function?

f2fs_gc_pinned_control()
{
	if (!f2fs_is_pinned_file(inode))
		return 0;
	if (gc_type != FG_GC)
		return 0;
	if (!f2fs_pin_section())
		f2fs_pin_file_control();
	return -EAGAIN;
}

>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> -			f2fs_pin_file_control(inode, true);
> +		if (gc_type == FG_GC) {
> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))

> +				f2fs_pin_file_control(inode, true);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>  		goto out;
>  	}
>  	if (f2fs_is_pinned_file(inode)) {
> -		if (gc_type == FG_GC)
> -			f2fs_pin_file_control(inode, true);
> +		if (gc_type == FG_GC) {
> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
> +				f2fs_pin_file_control(inode, true);
> +		}
>  		err = -EAGAIN;
>  		goto out;
>  	}
> @@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> 
>  			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
>  							gc_type == FG_GC) {
> -				f2fs_pin_file_control(inode, true);
> +				if (!f2fs_pin_section(sbi, segno))
> +					f2fs_pin_file_control(inode, true);
>  				iput(inode);
>  				return submitted;
>  			}
> @@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  		ret = -EINVAL;
>  		goto stop;
>  	}
> +retry:
>  	ret = __get_victim(sbi, &segno, gc_type);
> -	if (ret)
> +	if (ret) {
> +		/* allow to search victim from sections has pinned data */
> +		if (ret == -ENODATA && gc_type == FG_GC &&
> +				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
> +			f2fs_unpin_all_sections(sbi, false);
> +			goto retry;
> +		}
>  		goto stop;
> +	}
> 
>  	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>  	if (gc_type == FG_GC &&
> @@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>  	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> 
> +	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
> +		f2fs_unpin_all_sections(sbi, true);
> +
>  	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>  				get_pages(sbi, F2FS_DIRTY_NODES),
>  				get_pages(sbi, F2FS_DIRTY_DENTS),
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 22dfeb991529..93c7bae57a25 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>  	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>  	if (!dirty_i->victim_secmap)
>  		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> +	if (!dirty_i->pinned_secmap)
> +		return -ENOMEM;
> +
> +	dirty_i->pinned_secmap_cnt = 0;
> +	dirty_i->enable_pin_section = true;
>  	return 0;
>  }
> 
> @@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>  {
>  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> 
> +	kvfree(dirty_i->pinned_secmap);
>  	kvfree(dirty_i->victim_secmap);
>  }
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 5c94caf0c0a1..8a591455d796 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -294,6 +294,9 @@ struct dirty_seglist_info {
>  	struct mutex seglist_lock;		/* lock for segment bitmaps */
>  	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>  	unsigned long *victim_secmap;		/* background GC victims */
> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> +	bool enable_pin_section;		/* enable pinning section */
>  };
> 
>  /* victim selection function for cleaning and SSR */
> -- 
> 2.25.1
> 
> Thanks,
> 
> > 
> > > +		}
> > >   		err = -EAGAIN;
> > >   		goto out;
> > >   	}
> > > @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
> > >   		goto out;
> > >   	}
> > >   	if (f2fs_is_pinned_file(inode)) {
> > > -		if (gc_type == FG_GC)
> > > +		if (gc_type == FG_GC) {
> > >   			f2fs_pin_file_control(inode, true);
> > > +			pin_section(F2FS_I_SB(inode), segno);
> > > +		}
> > >   		err = -EAGAIN;
> > >   		goto out;
> > >   	}
> > > @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > >   							gc_type == FG_GC) {
> > >   				f2fs_pin_file_control(inode, true);
> > >   				iput(inode);
> > > +				pin_section(sbi, segno);
> > 
> > We don't have this code.
> > 
> > >   				return submitted;
> > >   			}
> > > @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> > >   		ret = -EINVAL;
> > >   		goto stop;
> > >   	}
> > > +retry:
> > >   	ret = __get_victim(sbi, &segno, gc_type);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		/* allow to search victim from sections has pinned data */
> > > +		if (ret == -ENODATA && gc_type == FG_GC &&
> > > +				pinned_section_exists(DIRTY_I(sbi))) {
> > > +			unpin_all_sections(sbi);
> > > +			goto retry;
> > > +		}
> > >   		goto stop;
> > > +	}
> > >   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
> > >   	if (gc_type == FG_GC &&
> > > @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> > >   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
> > >   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
> > > +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
> > > +		unpin_all_sections(sbi);
> > > +
> > >   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
> > >   				get_pages(sbi, F2FS_DIRTY_NODES),
> > >   				get_pages(sbi, F2FS_DIRTY_DENTS),
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 012524db7437..1c20d7c9eca3 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
> > >   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > >   	if (!dirty_i->victim_secmap)
> > >   		return -ENOMEM;
> > > +
> > > +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
> > > +	if (!dirty_i->pinned_secmap)
> > > +		return -ENOMEM;
> > > +
> > > +	dirty_i->pinned_secmap_cnt = 0;
> > >   	return 0;
> > >   }
> > > @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
> > >   {
> > >   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > > +	kvfree(dirty_i->pinned_secmap);
> > >   	kvfree(dirty_i->victim_secmap);
> > >   }
> > > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > > index 5c94caf0c0a1..fd6f246e649c 100644
> > > --- a/fs/f2fs/segment.h
> > > +++ b/fs/f2fs/segment.h
> > > @@ -294,6 +294,8 @@ struct dirty_seglist_info {
> > >   	struct mutex seglist_lock;		/* lock for segment bitmaps */
> > >   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
> > >   	unsigned long *victim_secmap;		/* background GC victims */
> > > +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
> > > +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
> > >   };
> > >   /* victim selection function for cleaning and SSR */
> > > -- 
> > > 2.32.0


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
  2022-04-12 16:21       ` [f2fs-dev] " Jaegeuk Kim
@ 2022-04-13  1:26         ` Chao Yu
  -1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-13  1:26 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu

On 2022/4/13 0:21, Jaegeuk Kim wrote:
> On 04/12, Chao Yu wrote:
>> On 2022/4/12 4:20, Jaegeuk Kim wrote:
>>> On 04/06, Chao Yu wrote:
>>>> Previously, during foreground GC, if victims contain data of pinned file,
>>>> it will fail migration of the data, and meanwhile i_gc_failures of that
>>>> pinned file may increase, and when it exceeds threshold, GC will unpin
>>>> the file, result in breaking pinfile's semantics.
>>>>
>>>> In order to mitigate such condition, let's record and skip section which
>>>> has pinned file's data and give priority to select unpinned one.
>>>>
>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>> ---
>>>> v3:
>>>> - check pin status before pinning section in pin_section().
>>>>    fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>>>>    fs/f2fs/segment.c |  7 ++++++
>>>>    fs/f2fs/segment.h |  2 ++
>>>>    3 files changed, 62 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index 6a7e4148ff9d..df23824ae3c2 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>>>    	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>>>    }
>>>> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>>>
>>> Need f2fs_...?
>>
>> Sure, I can add prefix...
>>
>> It's a local function, it won't pollute global namespace w/o f2fs_ prefix
>> though.
>>
>>>
>>>> +{
>>>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>>> +
>>>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>>>> +		return;
>>>> +	set_bit(secno, dirty_i->pinned_secmap);
>>>> +	dirty_i->pinned_secmap_cnt++;
>>>> +}
>>>> +
>>>> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
>>>> +{
>>>> +	return dirty_i->pinned_secmap_cnt;
>>>> +}
>>>> +
>>>> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
>>>> +						unsigned int secno)
>>>> +{
>>>> +	return pinned_section_exists(dirty_i) &&
>>>> +			test_bit(secno, dirty_i->pinned_secmap);
>>>> +}
>>>> +
>>>> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
>>>> +{
>>>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>>>> +
>>>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>>>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>>>> +}
>>>> +
>>>>    /*
>>>>     * This function is called from two paths.
>>>>     * One is garbage collection and the other is SSR segment selection.
>>>> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>>>    		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>>>    			goto next;
>>>> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
>>>> +			goto next;
>>>> +
>>>>    		if (is_atgc) {
>>>>    			add_victim_entry(sbi, &p, segno);
>>>>    			goto next;
>>>> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>>>    	}
>>>>    	if (f2fs_is_pinned_file(inode)) {
>>>> -		if (gc_type == FG_GC)
>>>> +		if (gc_type == FG_GC) {
>>>>    			f2fs_pin_file_control(inode, true);
>>>> +			pin_section(F2FS_I_SB(inode), segno);
>>>
>>> Do we need to check unpinning the inode?
>>> 			if (!f2fs_pin_file_control())
>>> 				f2fs_set_pin_section();
>>
>> I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
>> so could you please check below logic:
>>
>>  From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
>> From: Chao Yu <chao@kernel.org>
>> Date: Wed, 6 Apr 2022 23:26:51 +0800
>> Subject: [PATCH v4] f2fs: give priority to select unpinned section for
>>   foreground GC
>>
>> Previously, during foreground GC, if victims contain data of pinned file,
>> it will fail migration of the data, and meanwhile i_gc_failures of that
>> pinned file may increase, and when it exceeds threshold, GC will unpin
>> the file, result in breaking pinfile's semantics.
>>
>> In order to mitigate such condition, let's record and skip section which
>> has pinned file's data and give priority to select unpinned one.
>>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>> v4:
>> - add f2fs_ prefix for newly introduced functions
>> - add bool type variable for functionality switch
>> - increase GC_FAILURE_PIN only if it disable pinning section
>>   fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
>>   fs/f2fs/segment.c |  8 ++++++
>>   fs/f2fs/segment.h |  3 +++
>>   3 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6a7e4148ff9d..296b31e28d3d 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>   }
>>
>> +static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>> +{
>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>> +
>> +	if (!dirty_i->enable_pin_section)
>> +		return false;
>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>> +		return true;
>> +	set_bit(secno, dirty_i->pinned_secmap);
>> +	dirty_i->pinned_secmap_cnt++;
> 
> 	if (!test_and_set_bit())
> 		dirty_i->pinned_secmap_cnt++;

More clean.

> 
>> +	return true;
>> +}
>> +
>> +static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
>> +{
>> +	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
>> +}
>> +
>> +static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
>> +						unsigned int secno)
>> +{
>> +	return f2fs_pinned_section_exists(dirty_i) &&
>> +			test_bit(secno, dirty_i->pinned_secmap);
>> +}
>> +
>> +static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
>> +{
>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>> +
>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>> +	DIRTY_I(sbi)->enable_pin_section = enable;
>> +}
>> +
>>   /*
>>    * This function is called from two paths.
>>    * One is garbage collection and the other is SSR segment selection.
>> @@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>   			goto next;
>>
>> +		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
>> +			goto next;
>> +
>>   		if (is_atgc) {
>>   			add_victim_entry(sbi, &p, segno);
>>   			goto next;
>> @@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>   	}
>>
> 
> Can we make a generic function?
> 
> f2fs_gc_pinned_control()
> {
> 	if (!f2fs_is_pinned_file(inode))
> 		return 0;
> 	if (gc_type != FG_GC)
> 		return 0;
> 	if (!f2fs_pin_section())
> 		f2fs_pin_file_control();
> 	return -EAGAIN;
> }

Better. :)

Thanks,

> 
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> -			f2fs_pin_file_control(inode, true);
>> +		if (gc_type == FG_GC) {
>> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
> 
>> +				f2fs_pin_file_control(inode, true);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>   		goto out;
>>   	}
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> -			f2fs_pin_file_control(inode, true);
>> +		if (gc_type == FG_GC) {
>> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
>> +				f2fs_pin_file_control(inode, true);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>
>>   			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
>>   							gc_type == FG_GC) {
>> -				f2fs_pin_file_control(inode, true);
>> +				if (!f2fs_pin_section(sbi, segno))
>> +					f2fs_pin_file_control(inode, true);
>>   				iput(inode);
>>   				return submitted;
>>   			}
>> @@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   		ret = -EINVAL;
>>   		goto stop;
>>   	}
>> +retry:
>>   	ret = __get_victim(sbi, &segno, gc_type);
>> -	if (ret)
>> +	if (ret) {
>> +		/* allow to search victim from sections has pinned data */
>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>> +				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
>> +			f2fs_unpin_all_sections(sbi, false);
>> +			goto retry;
>> +		}
>>   		goto stop;
>> +	}
>>
>>   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>   	if (gc_type == FG_GC &&
>> @@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>
>> +	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
>> +		f2fs_unpin_all_sections(sbi, true);
>> +
>>   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>   				get_pages(sbi, F2FS_DIRTY_NODES),
>>   				get_pages(sbi, F2FS_DIRTY_DENTS),
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 22dfeb991529..93c7bae57a25 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>   	if (!dirty_i->victim_secmap)
>>   		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> +	if (!dirty_i->pinned_secmap)
>> +		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap_cnt = 0;
>> +	dirty_i->enable_pin_section = true;
>>   	return 0;
>>   }
>>
>> @@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>   {
>>   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>
>> +	kvfree(dirty_i->pinned_secmap);
>>   	kvfree(dirty_i->victim_secmap);
>>   }
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 5c94caf0c0a1..8a591455d796 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -294,6 +294,9 @@ struct dirty_seglist_info {
>>   	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>   	unsigned long *victim_secmap;		/* background GC victims */
>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>> +	bool enable_pin_section;		/* enable pinning section */
>>   };
>>
>>   /* victim selection function for cleaning and SSR */
>> -- 
>> 2.25.1
>>
>> Thanks,
>>
>>>
>>>> +		}
>>>>    		err = -EAGAIN;
>>>>    		goto out;
>>>>    	}
>>>> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>    		goto out;
>>>>    	}
>>>>    	if (f2fs_is_pinned_file(inode)) {
>>>> -		if (gc_type == FG_GC)
>>>> +		if (gc_type == FG_GC) {
>>>>    			f2fs_pin_file_control(inode, true);
>>>> +			pin_section(F2FS_I_SB(inode), segno);
>>>> +		}
>>>>    		err = -EAGAIN;
>>>>    		goto out;
>>>>    	}
>>>> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>>    							gc_type == FG_GC) {
>>>>    				f2fs_pin_file_control(inode, true);
>>>>    				iput(inode);
>>>> +				pin_section(sbi, segno);
>>>
>>> We don't have this code.
>>>
>>>>    				return submitted;
>>>>    			}
>>>> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>>    		ret = -EINVAL;
>>>>    		goto stop;
>>>>    	}
>>>> +retry:
>>>>    	ret = __get_victim(sbi, &segno, gc_type);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>> +		/* allow to search victim from sections has pinned data */
>>>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>>>> +				pinned_section_exists(DIRTY_I(sbi))) {
>>>> +			unpin_all_sections(sbi);
>>>> +			goto retry;
>>>> +		}
>>>>    		goto stop;
>>>> +	}
>>>>    	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>>>    	if (gc_type == FG_GC &&
>>>> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>>    	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>>>    	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>>> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
>>>> +		unpin_all_sections(sbi);
>>>> +
>>>>    	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>>>    				get_pages(sbi, F2FS_DIRTY_NODES),
>>>>    				get_pages(sbi, F2FS_DIRTY_DENTS),
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 012524db7437..1c20d7c9eca3 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>>>    	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>>    	if (!dirty_i->victim_secmap)
>>>>    		return -ENOMEM;
>>>> +
>>>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>> +	if (!dirty_i->pinned_secmap)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	dirty_i->pinned_secmap_cnt = 0;
>>>>    	return 0;
>>>>    }
>>>> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>>>    {
>>>>    	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> +	kvfree(dirty_i->pinned_secmap);
>>>>    	kvfree(dirty_i->victim_secmap);
>>>>    }
>>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>>> index 5c94caf0c0a1..fd6f246e649c 100644
>>>> --- a/fs/f2fs/segment.h
>>>> +++ b/fs/f2fs/segment.h
>>>> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>>>>    	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>>>    	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>>>    	unsigned long *victim_secmap;		/* background GC victims */
>>>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>>>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>>>>    };
>>>>    /* victim selection function for cleaning and SSR */
>>>> -- 
>>>> 2.32.0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC
@ 2022-04-13  1:26         ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-04-13  1:26 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2022/4/13 0:21, Jaegeuk Kim wrote:
> On 04/12, Chao Yu wrote:
>> On 2022/4/12 4:20, Jaegeuk Kim wrote:
>>> On 04/06, Chao Yu wrote:
>>>> Previously, during foreground GC, if victims contain data of pinned file,
>>>> it will fail migration of the data, and meanwhile i_gc_failures of that
>>>> pinned file may increase, and when it exceeds threshold, GC will unpin
>>>> the file, result in breaking pinfile's semantics.
>>>>
>>>> In order to mitigate such condition, let's record and skip section which
>>>> has pinned file's data and give priority to select unpinned one.
>>>>
>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>> ---
>>>> v3:
>>>> - check pin status before pinning section in pin_section().
>>>>    fs/f2fs/gc.c      | 56 ++++++++++++++++++++++++++++++++++++++++++++---
>>>>    fs/f2fs/segment.c |  7 ++++++
>>>>    fs/f2fs/segment.h |  2 ++
>>>>    3 files changed, 62 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index 6a7e4148ff9d..df23824ae3c2 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -646,6 +646,37 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>>>    	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>>>    }
>>>> +static void pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>>>
>>> Need f2fs_...?
>>
>> Sure, I can add prefix...
>>
>> It's a local function, it won't pollute global namespace w/o f2fs_ prefix
>> though.
>>
>>>
>>>> +{
>>>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>>> +
>>>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>>>> +		return;
>>>> +	set_bit(secno, dirty_i->pinned_secmap);
>>>> +	dirty_i->pinned_secmap_cnt++;
>>>> +}
>>>> +
>>>> +static bool pinned_section_exists(struct dirty_seglist_info *dirty_i)
>>>> +{
>>>> +	return dirty_i->pinned_secmap_cnt;
>>>> +}
>>>> +
>>>> +static bool section_is_pinned(struct dirty_seglist_info *dirty_i,
>>>> +						unsigned int secno)
>>>> +{
>>>> +	return pinned_section_exists(dirty_i) &&
>>>> +			test_bit(secno, dirty_i->pinned_secmap);
>>>> +}
>>>> +
>>>> +static void unpin_all_sections(struct f2fs_sb_info *sbi)
>>>> +{
>>>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>>>> +
>>>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>>>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>>>> +}
>>>> +
>>>>    /*
>>>>     * This function is called from two paths.
>>>>     * One is garbage collection and the other is SSR segment selection.
>>>> @@ -787,6 +818,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>>>    		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>>>    			goto next;
>>>> +		if (gc_type == FG_GC && section_is_pinned(dirty_i, secno))
>>>> +			goto next;
>>>> +
>>>>    		if (is_atgc) {
>>>>    			add_victim_entry(sbi, &p, segno);
>>>>    			goto next;
>>>> @@ -1202,8 +1236,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>>>    	}
>>>>    	if (f2fs_is_pinned_file(inode)) {
>>>> -		if (gc_type == FG_GC)
>>>> +		if (gc_type == FG_GC) {
>>>>    			f2fs_pin_file_control(inode, true);
>>>> +			pin_section(F2FS_I_SB(inode), segno);
>>>
>>> Do we need to check unpinning the inode?
>>> 			if (!f2fs_pin_file_control())
>>> 				f2fs_set_pin_section();
>>
>> I'm thinking that it needs to avoid increasing GC_FAILURE_PIN AMAP,
>> so could you please check below logic:
>>
>>  From 7cb1ee0df32ede44b17c503b81930dae25d287eb Mon Sep 17 00:00:00 2001
>> From: Chao Yu <chao@kernel.org>
>> Date: Wed, 6 Apr 2022 23:26:51 +0800
>> Subject: [PATCH v4] f2fs: give priority to select unpinned section for
>>   foreground GC
>>
>> Previously, during foreground GC, if victims contain data of pinned file,
>> it will fail migration of the data, and meanwhile i_gc_failures of that
>> pinned file may increase, and when it exceeds threshold, GC will unpin
>> the file, result in breaking pinfile's semantics.
>>
>> In order to mitigate such condition, let's record and skip section which
>> has pinned file's data and give priority to select unpinned one.
>>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>> v4:
>> - add f2fs_ prefix for newly introduced functions
>> - add bool type variable for functionality switch
>> - increase GC_FAILURE_PIN only if it disable pinning section
>>   fs/f2fs/gc.c      | 66 ++++++++++++++++++++++++++++++++++++++++++-----
>>   fs/f2fs/segment.c |  8 ++++++
>>   fs/f2fs/segment.h |  3 +++
>>   3 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 6a7e4148ff9d..296b31e28d3d 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -646,6 +646,41 @@ static void release_victim_entry(struct f2fs_sb_info *sbi)
>>   	f2fs_bug_on(sbi, !list_empty(&am->victim_list));
>>   }
>>
>> +static bool f2fs_pin_section(struct f2fs_sb_info *sbi, unsigned int segno)
>> +{
>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>> +	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>> +
>> +	if (!dirty_i->enable_pin_section)
>> +		return false;
>> +	if (test_bit(secno, dirty_i->pinned_secmap))
>> +		return true;
>> +	set_bit(secno, dirty_i->pinned_secmap);
>> +	dirty_i->pinned_secmap_cnt++;
> 
> 	if (!test_and_set_bit())
> 		dirty_i->pinned_secmap_cnt++;

More clean.

> 
>> +	return true;
>> +}
>> +
>> +static bool f2fs_pinned_section_exists(struct dirty_seglist_info *dirty_i)
>> +{
>> +	return dirty_i->enable_pin_section && dirty_i->pinned_secmap_cnt;
>> +}
>> +
>> +static bool f2fs_section_is_pinned(struct dirty_seglist_info *dirty_i,
>> +						unsigned int secno)
>> +{
>> +	return f2fs_pinned_section_exists(dirty_i) &&
>> +			test_bit(secno, dirty_i->pinned_secmap);
>> +}
>> +
>> +static void f2fs_unpin_all_sections(struct f2fs_sb_info *sbi, bool enable)
>> +{
>> +	unsigned int bitmap_size = f2fs_bitmap_size(MAIN_SECS(sbi));
>> +
>> +	memset(DIRTY_I(sbi)->pinned_secmap, 0, bitmap_size);
>> +	DIRTY_I(sbi)->pinned_secmap_cnt = 0;
>> +	DIRTY_I(sbi)->enable_pin_section = enable;
>> +}
>> +
>>   /*
>>    * This function is called from two paths.
>>    * One is garbage collection and the other is SSR segment selection.
>> @@ -787,6 +822,9 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
>>   		if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
>>   			goto next;
>>
>> +		if (gc_type == FG_GC && f2fs_section_is_pinned(dirty_i, secno))
>> +			goto next;
>> +
>>   		if (is_atgc) {
>>   			add_victim_entry(sbi, &p, segno);
>>   			goto next;
>> @@ -1202,8 +1240,10 @@ static int move_data_block(struct inode *inode, block_t bidx,
>>   	}
>>
> 
> Can we make a generic function?
> 
> f2fs_gc_pinned_control()
> {
> 	if (!f2fs_is_pinned_file(inode))
> 		return 0;
> 	if (gc_type != FG_GC)
> 		return 0;
> 	if (!f2fs_pin_section())
> 		f2fs_pin_file_control();
> 	return -EAGAIN;
> }

Better. :)

Thanks,

> 
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> -			f2fs_pin_file_control(inode, true);
>> +		if (gc_type == FG_GC) {
>> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
> 
>> +				f2fs_pin_file_control(inode, true);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1352,8 +1392,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>   		goto out;
>>   	}
>>   	if (f2fs_is_pinned_file(inode)) {
>> -		if (gc_type == FG_GC)
>> -			f2fs_pin_file_control(inode, true);
>> +		if (gc_type == FG_GC) {
>> +			if (!f2fs_pin_section(F2FS_I_SB(inode), segno))
>> +				f2fs_pin_file_control(inode, true);
>> +		}
>>   		err = -EAGAIN;
>>   		goto out;
>>   	}
>> @@ -1483,7 +1525,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>
>>   			if (is_inode_flag_set(inode, FI_PIN_FILE) &&
>>   							gc_type == FG_GC) {
>> -				f2fs_pin_file_control(inode, true);
>> +				if (!f2fs_pin_section(sbi, segno))
>> +					f2fs_pin_file_control(inode, true);
>>   				iput(inode);
>>   				return submitted;
>>   			}
>> @@ -1766,9 +1809,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   		ret = -EINVAL;
>>   		goto stop;
>>   	}
>> +retry:
>>   	ret = __get_victim(sbi, &segno, gc_type);
>> -	if (ret)
>> +	if (ret) {
>> +		/* allow to search victim from sections has pinned data */
>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>> +				f2fs_pinned_section_exists(DIRTY_I(sbi))) {
>> +			f2fs_unpin_all_sections(sbi, false);
>> +			goto retry;
>> +		}
>>   		goto stop;
>> +	}
>>
>>   	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>   	if (gc_type == FG_GC &&
>> @@ -1811,6 +1862,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>
>> +	if (gc_type == FG_GC && f2fs_pinned_section_exists(DIRTY_I(sbi)))
>> +		f2fs_unpin_all_sections(sbi, true);
>> +
>>   	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>   				get_pages(sbi, F2FS_DIRTY_NODES),
>>   				get_pages(sbi, F2FS_DIRTY_DENTS),
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 22dfeb991529..93c7bae57a25 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4734,6 +4734,13 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>   	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>   	if (!dirty_i->victim_secmap)
>>   		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>> +	if (!dirty_i->pinned_secmap)
>> +		return -ENOMEM;
>> +
>> +	dirty_i->pinned_secmap_cnt = 0;
>> +	dirty_i->enable_pin_section = true;
>>   	return 0;
>>   }
>>
>> @@ -5322,6 +5329,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>   {
>>   	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>
>> +	kvfree(dirty_i->pinned_secmap);
>>   	kvfree(dirty_i->victim_secmap);
>>   }
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 5c94caf0c0a1..8a591455d796 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -294,6 +294,9 @@ struct dirty_seglist_info {
>>   	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>   	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>   	unsigned long *victim_secmap;		/* background GC victims */
>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>> +	bool enable_pin_section;		/* enable pinning section */
>>   };
>>
>>   /* victim selection function for cleaning and SSR */
>> -- 
>> 2.25.1
>>
>> Thanks,
>>
>>>
>>>> +		}
>>>>    		err = -EAGAIN;
>>>>    		goto out;
>>>>    	}
>>>> @@ -1352,8 +1388,10 @@ static int move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>    		goto out;
>>>>    	}
>>>>    	if (f2fs_is_pinned_file(inode)) {
>>>> -		if (gc_type == FG_GC)
>>>> +		if (gc_type == FG_GC) {
>>>>    			f2fs_pin_file_control(inode, true);
>>>> +			pin_section(F2FS_I_SB(inode), segno);
>>>> +		}
>>>>    		err = -EAGAIN;
>>>>    		goto out;
>>>>    	}
>>>> @@ -1485,6 +1523,7 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>>    							gc_type == FG_GC) {
>>>>    				f2fs_pin_file_control(inode, true);
>>>>    				iput(inode);
>>>> +				pin_section(sbi, segno);
>>>
>>> We don't have this code.
>>>
>>>>    				return submitted;
>>>>    			}
>>>> @@ -1766,9 +1805,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>>    		ret = -EINVAL;
>>>>    		goto stop;
>>>>    	}
>>>> +retry:
>>>>    	ret = __get_victim(sbi, &segno, gc_type);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>> +		/* allow to search victim from sections has pinned data */
>>>> +		if (ret == -ENODATA && gc_type == FG_GC &&
>>>> +				pinned_section_exists(DIRTY_I(sbi))) {
>>>> +			unpin_all_sections(sbi);
>>>> +			goto retry;
>>>> +		}
>>>>    		goto stop;
>>>> +	}
>>>>    	seg_freed = do_garbage_collect(sbi, segno, &gc_list, gc_type, force);
>>>>    	if (gc_type == FG_GC &&
>>>> @@ -1811,6 +1858,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>>>>    	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>>>>    	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
>>>> +	if (gc_type == FG_GC && pinned_section_exists(DIRTY_I(sbi)))
>>>> +		unpin_all_sections(sbi);
>>>> +
>>>>    	trace_f2fs_gc_end(sbi->sb, ret, total_freed, sec_freed,
>>>>    				get_pages(sbi, F2FS_DIRTY_NODES),
>>>>    				get_pages(sbi, F2FS_DIRTY_DENTS),
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 012524db7437..1c20d7c9eca3 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -4736,6 +4736,12 @@ static int init_victim_secmap(struct f2fs_sb_info *sbi)
>>>>    	dirty_i->victim_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>>    	if (!dirty_i->victim_secmap)
>>>>    		return -ENOMEM;
>>>> +
>>>> +	dirty_i->pinned_secmap = f2fs_kvzalloc(sbi, bitmap_size, GFP_KERNEL);
>>>> +	if (!dirty_i->pinned_secmap)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	dirty_i->pinned_secmap_cnt = 0;
>>>>    	return 0;
>>>>    }
>>>> @@ -5324,6 +5330,7 @@ static void destroy_victim_secmap(struct f2fs_sb_info *sbi)
>>>>    {
>>>>    	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>>> +	kvfree(dirty_i->pinned_secmap);
>>>>    	kvfree(dirty_i->victim_secmap);
>>>>    }
>>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>>> index 5c94caf0c0a1..fd6f246e649c 100644
>>>> --- a/fs/f2fs/segment.h
>>>> +++ b/fs/f2fs/segment.h
>>>> @@ -294,6 +294,8 @@ struct dirty_seglist_info {
>>>>    	struct mutex seglist_lock;		/* lock for segment bitmaps */
>>>>    	int nr_dirty[NR_DIRTY_TYPE];		/* # of dirty segments */
>>>>    	unsigned long *victim_secmap;		/* background GC victims */
>>>> +	unsigned long *pinned_secmap;		/* pinned victims from foreground GC */
>>>> +	unsigned int pinned_secmap_cnt;		/* count of victims which has pinned data */
>>>>    };
>>>>    /* victim selection function for cleaning and SSR */
>>>> -- 
>>>> 2.32.0


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-04-13  1:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 15:26 [f2fs-dev] [PATCH v3] f2fs: give priority to select unpinned section for foreground GC Chao Yu
2022-04-06 15:26 ` Chao Yu
2022-04-11 20:20 ` Jaegeuk Kim
2022-04-11 20:20   ` [f2fs-dev] " Jaegeuk Kim
2022-04-11 21:25   ` Jaegeuk Kim
2022-04-11 21:25     ` Jaegeuk Kim
2022-04-12  3:54   ` Chao Yu
2022-04-12  3:54     ` [f2fs-dev] " Chao Yu
2022-04-12 16:21     ` Jaegeuk Kim
2022-04-12 16:21       ` [f2fs-dev] " Jaegeuk Kim
2022-04-13  1:26       ` Chao Yu
2022-04-13  1:26         ` [f2fs-dev] " Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.