linux-kernel - Re: [PATCH] f2fs: fix to trigger a checkpoint in the end of foreground garbage collection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a4e49177-3959-eb2b-996c-5d07b7390495@kernel.org>
Date:   Wed, 5 Apr 2023 10:02:20 +0800
From:   Chao Yu <chao@...nel.org>
To:     Jaegeuk Kim <jaegeuk@...nel.org>
Cc:     linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] f2fs: fix to trigger a checkpoint in the end of
 foreground garbage collection

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@...nel.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@...nel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@...nel.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;