Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 6 May 2022 11:13:16 -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
Subject: Re: [PATCH v4] f2fs: avoid to select pinned section during
 checkpoint=disable
Message-ID: <YnVlPEWzV6PykN2u@google.com>
References: <20220506115800.2605-1-chao@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220506115800.2605-1-chao@kernel.org>
Precedence: bulk

On 05/06, Chao Yu wrote:
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> 
> The f2fs_gc uses a bitmap to indicate pinned sections, but when disabling
> chckpoint, we call f2fs_gc() with NULL_SEGNO which selects the same dirty
> segment as a victim all the time, resulting in checkpoint=disable failure.
> Let's pick another one, if we fail to collect it.

I'm concerning more like the generic cases where GC will get the same victim
having the pinned space resulting in EAGAIN all the time.

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v4:
> - use sec_freed instead of total_freed
>  fs/f2fs/f2fs.h    |  2 +-
>  fs/f2fs/file.c    |  8 ++++----
>  fs/f2fs/gc.c      | 17 +++++++++++------
>  fs/f2fs/segment.c |  2 +-
>  fs/f2fs/super.c   |  2 +-
>  5 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 56adc3b68e14..0f8c426aed50 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3787,7 +3787,7 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi);
>  void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi);
>  block_t f2fs_start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
>  int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, bool background, bool force,
> -			unsigned int segno);
> +			unsigned int segno, unsigned int nr_section);
>  void f2fs_build_gc_manager(struct f2fs_sb_info *sbi);
>  int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count);
>  int __init f2fs_create_garbage_collection_cache(void);
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index e4cf8b7b23aa..07d2fa6c774b 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1684,7 +1684,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
>  		if (has_not_enough_free_secs(sbi, 0,
>  			GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) {
>  			f2fs_down_write(&sbi->gc_lock);
> -			err = f2fs_gc(sbi, true, false, false, NULL_SEGNO);
> +			err = f2fs_gc(sbi, true, false, false, NULL_SEGNO, 0);
>  			if (err && err != -ENODATA && err != -EAGAIN)
>  				goto out_err;
>  		}
> @@ -2472,7 +2472,7 @@ static int f2fs_ioc_gc(struct file *filp, unsigned long arg)
>  		f2fs_down_write(&sbi->gc_lock);
>  	}
>  
> -	ret = f2fs_gc(sbi, sync, true, false, NULL_SEGNO);
> +	ret = f2fs_gc(sbi, sync, true, false, NULL_SEGNO, 0);
>  out:
>  	mnt_drop_write_file(filp);
>  	return ret;
> @@ -2509,7 +2509,7 @@ static int __f2fs_ioc_gc_range(struct file *filp, struct f2fs_gc_range *range)
>  	}
>  
>  	ret = f2fs_gc(sbi, range->sync, true, false,
> -				GET_SEGNO(sbi, range->start));
> +				GET_SEGNO(sbi, range->start), 0);
>  	if (ret) {
>  		if (ret == -EBUSY)
>  			ret = -EAGAIN;
> @@ -2966,7 +2966,7 @@ static int f2fs_ioc_flush_device(struct file *filp, unsigned long arg)
>  		sm->last_victim[GC_CB] = end_segno + 1;
>  		sm->last_victim[GC_GREEDY] = end_segno + 1;
>  		sm->last_victim[ALLOC_NEXT] = end_segno + 1;
> -		ret = f2fs_gc(sbi, true, true, true, start_segno);
> +		ret = f2fs_gc(sbi, true, true, true, start_segno, 0);
>  		if (ret == -EAGAIN)
>  			ret = 0;
>  		else if (ret < 0)
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 3009c0a97ab4..d13a86272e8b 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -142,7 +142,7 @@ static int gc_thread_func(void *data)
>  			sync_mode = false;
>  
>  		/* if return value is not zero, no victim was selected */
> -		if (f2fs_gc(sbi, sync_mode, !foreground, false, NULL_SEGNO))
> +		if (f2fs_gc(sbi, sync_mode, !foreground, false, NULL_SEGNO, 0))
>  			wait_ms = gc_th->no_gc_sleep_time;
>  
>  		if (foreground)
> @@ -1754,7 +1754,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>  }
>  
>  int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
> -			bool background, bool force, unsigned int segno)
> +			bool background, bool force, unsigned int segno,
> +			unsigned int nr_section)
>  {
>  	int gc_type = sync ? FG_GC : BG_GC;
>  	int sec_freed = 0, seg_freed = 0, total_freed = 0;
> @@ -1841,11 +1842,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  	if (gc_type == FG_GC)
>  		sbi->cur_victim_sec = NULL_SEGNO;
>  
> -	if (sync)
> -		goto stop;
> -
> -	if (!has_not_enough_free_secs(sbi, sec_freed, 0))
> +	if (sync || !has_not_enough_free_secs(sbi, sec_freed, 0)) {
> +		/*
> +		 * f2fs_disable_checkpoint() needs to migrate enough section
> +		 * before f2fs_gc() exits.
> +		 */
> +		if (sec_freed < nr_section)
> +			goto gc_more;
>  		goto stop;
> +	}
>  
>  	if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
>  
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 8c17fed8987e..3a3e2cec2ac4 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -524,7 +524,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
>  			finish_wait(&sbi->gc_thread->fggc_wq, &wait);
>  		} else {
>  			f2fs_down_write(&sbi->gc_lock);
> -			f2fs_gc(sbi, false, false, false, NULL_SEGNO);
> +			f2fs_gc(sbi, false, false, false, NULL_SEGNO, 0);
>  		}
>  	}
>  }
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index d06a577a1208..2f0ad9ffbf17 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2076,7 +2076,7 @@ static int f2fs_disable_checkpoint(struct f2fs_sb_info *sbi)
>  
>  	while (!f2fs_time_over(sbi, DISABLE_TIME)) {
>  		f2fs_down_write(&sbi->gc_lock);
> -		err = f2fs_gc(sbi, true, false, false, NULL_SEGNO);
> +		err = f2fs_gc(sbi, true, false, false, NULL_SEGNO, 1);
>  		if (err == -ENODATA) {
>  			err = 0;
>  			break;
> -- 
> 2.32.0