Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 11 Apr 2022 13:20:30 -0700
From:   Jaegeuk Kim <jaegeuk@kernel.org>
To:     Chao Yu <chao@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Chao Yu <chao.yu@oppo.com>
Subject: Re: [PATCH v3] f2fs: give priority to select unpinned section for
 foreground GC
Message-ID: <YlSNjgQwoiENx5EK@google.com>
References: <20220406152651.5142-1-chao@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220406152651.5142-1-chao@kernel.org>
Precedence: bulk

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