Re: [PATCH] f2fs: make gc_wake test-and-clear atomic

[Chao Yu <chao@kernel.org>]

On 5/16/2026 11:50 AM, Ziyu Zhang wrote:
> gc_thread_func() tests gc_th->gc_wake and then clears it with
> separate plain accesses. sysfs gc_urgent writes set the same flag and
> wake the GC thread. If a sysfs writer stores true between the GC
> thread's load and store, the later store false can clear the new wake
> request.

I can accept calling "echo 1 > gc_urgent" multiple times, but f2fs only
trigger one time, because it's rare to change to urgent mode, and there
should be no multiple users of this mode in Android.

> 
> Store gc_wake as an atomic_t. Use atomic_read() for the wait
> condition, atomic_xchg(..., 0) in the GC thread, and atomic_set(..., 1)
> in the sysfs trigger paths. This makes the consume-and-clear operation
> atomic with respect to new wake requests: a set before the exchange is
> consumed by the current iteration, while a set after the exchange stays
> pending for the next wait.
> 
> Fixes: d9872a698c39 ("f2fs: introduce gc_urgent mode for background GC")
> Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
> ---
>   fs/f2fs/gc.c    | 7 +++----
>   fs/f2fs/gc.h    | 2 +-
>   fs/f2fs/sysfs.c | 4 ++--
>   3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 098e9f71421e..71e40e4083ad 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -49,15 +49,14 @@ static int gc_thread_func(void *data)
>   		wait_event_freezable_timeout(*wq,
>   				kthread_should_stop() ||
>   				waitqueue_active(fggc_wq) ||
> -				gc_th->gc_wake,
> +				atomic_read(&gc_th->gc_wake),
>   				msecs_to_jiffies(wait_ms));
>   
>   		if (test_opt(sbi, GC_MERGE) && waitqueue_active(fggc_wq))
>   			foreground = true;

If we trigger gc_urgent before, and then trigger again here, atomic_xchg()
will clear the new wakeup request as well? unless we record the total request
count into atomic variable.

Thanks,

>   
>   		/* give it a try one time */
> -		if (gc_th->gc_wake)
> -			gc_th->gc_wake = false;
> +		atomic_xchg(&gc_th->gc_wake, 0);
>   
>   		if (f2fs_readonly(sbi->sb)) {
>   			stat_other_skip_bggc_count(sbi);
> @@ -214,7 +213,7 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi)
>   		gc_th->boost_zoned_gc_percent = 0;
>   	}
>   
> -	gc_th->gc_wake = false;
> +	atomic_set(&gc_th->gc_wake, 0);
>   
>   	sbi->gc_thread = gc_th;
>   	init_waitqueue_head(&sbi->gc_thread->gc_wait_queue_head);
> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
> index 24e8b1c27acc..65e5b062a0d3 100644
> --- a/fs/f2fs/gc.h
> +++ b/fs/f2fs/gc.h
> @@ -56,7 +56,7 @@ struct f2fs_gc_kthread {
>   	unsigned int no_gc_sleep_time;
>   
>   	/* for changing gc mode */
> -	bool gc_wake;
> +	atomic_t gc_wake;
>   
>   	/* for GC_MERGE mount option */
>   	wait_queue_head_t fggc_wq;		/*
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index f736052dea50..6ca5943450d5 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -586,7 +586,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   		} else if (t == 1) {
>   			sbi->gc_mode = GC_URGENT_HIGH;
>   			if (sbi->gc_thread) {
> -				sbi->gc_thread->gc_wake = true;
> +				atomic_set(&sbi->gc_thread->gc_wake, 1);
>   				wake_up_interruptible_all(
>   					&sbi->gc_thread->gc_wait_queue_head);
>   				wake_up_discard_thread(sbi, true);
> @@ -596,7 +596,7 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   		} else if (t == 3) {
>   			sbi->gc_mode = GC_URGENT_MID;
>   			if (sbi->gc_thread) {
> -				sbi->gc_thread->gc_wake = true;
> +				atomic_set(&sbi->gc_thread->gc_wake, 1);
>   				wake_up_interruptible_all(
>   					&sbi->gc_thread->gc_wait_queue_head);
>   			}