Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a4e49177-3959-eb2b-996c-5d07b7390495@kernel.org>
Date:   Wed, 5 Apr 2023 10:02:20 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Content-Language: en-US
To:     Jaegeuk Kim <jaegeuk@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
References: <20230324071028.336982-1-chao@kernel.org>
 <ZCyZGgf4RSEjyHTF@google.com>
From:   Chao Yu <chao@kernel.org>
Subject: Re: [PATCH] f2fs: fix to trigger a checkpoint in the end of
 foreground garbage collection
In-Reply-To: <ZCyZGgf4RSEjyHTF@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/4/5 5:39, Jaegeuk Kim wrote:
> Can we do like this?
> 
>  From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Tue, 4 Apr 2023 14:36:00 -0700
> Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections
> 
> We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree
> sctions in time.
> 
> Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc")
> Signed-off-by: Chao Yu <chao@kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>   fs/f2fs/gc.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 56c53dbe05c9..f1d0dd9c5a6c 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
>   	};
>   	unsigned int skipped_round = 0, round = 0;
>   	unsigned int upper_secs;
> +	bool stop_gc = false;
>   
>   	trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc,
>   				gc_control->nr_free_secs,
> @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
>   				(gc_type == FG_GC) ? sec_freed : 0, 0)) {
>   		if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs)
>   			goto go_gc_more;
> -		goto stop;
> -	}
> -
> -	/* FG_GC stops GC by skip_count */
> -	if (gc_type == FG_GC) {
> +		stop_gc = true;

I guess below condition is for emergency recycle of prefree segments during
foreground GC, in order to avoid exhausting free sections due to to many
metadata allocation during CP.

	if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS &&
				prefree_segments(sbi)) {

But for common case, free_sections() is close to reserved_segments(), and
upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(),
so checkpoint may not be trggered as expected, IIUC.

So it's fine to just trigger CP in the end of foreground garbage collection?

One other concern is for those path as below:
- disable_checkpoint
- ioc_gc
- ioc_gc_range
- ioc_resize
...

We've passed gc_type as FG_GC, but the demand here is to migrate block in time,
rather than dirtying blocks, and callers don't expect checkpoint in f2fs_gc(),
instead the callers will do the checkpoit as it needs.

That means it's better to decouple FG_GC and write_checkpoint behavior, so I
added another parameter .reclaim_space to just let f2fs_balance_fs() to trigger
checkpoit in the end of f2fs_gc().

Thanks,

> +	} else if (gc_type == FG_GC) {
> +		/* FG_GC stops GC by skip_count */
>   		if (sbi->skipped_gc_rwsem)
>   			skipped_round++;
>   		round++;
>   		if (skipped_round > MAX_SKIP_GC_COUNT &&
> -				skipped_round * 2 >= round) {
> -			ret = f2fs_write_checkpoint(sbi, &cpc);
> -			goto stop;
> -		}
> +				skipped_round * 2 >= round)
> +			stop_gc = true;
>   	}
>   
>   	__get_secs_required(sbi, NULL, &upper_secs, NULL);
> @@ -1901,12 +1898,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
>   				prefree_segments(sbi)) {
>   		ret = f2fs_write_checkpoint(sbi, &cpc);
>   		if (ret)
> -			goto stop;
> +			stop_gc = true;
>   	}
>   go_gc_more:
> -	segno = NULL_SEGNO;
> -	goto gc_more;
> -
> +	if (!stop_gc) {
> +		segno = NULL_SEGNO;
> +		goto gc_more;
> +	}
>   stop:
>   	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
>   	SIT_I(sbi)->last_victim[FLUSH_DEVICE] = gc_control->victim_segno;