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

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

[Ziyu Zhang]

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.

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;
 
 		/* 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);
 			}
-- 
2.43.0

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

[Chao Yu]

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);
>   			}

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

[ziyu zhang]

On Sun, 17 May 2026 at 17:22, Chao Yu <chao@kernel.org> wrote:
>
> 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.

Thanks for the clarification. I agree that if gc_urgent is intended to be a
mode switch rather than a counted request, multiple writes to gc_urgent do not
need to translate into multiple GC runs.

In that case, the practical impact of this atomicity violation is very limited:
the repeated wakeups may be coalesced, but gc_mode still records the urgent
mode and the GC thread also has the timeout fallback.

>
> >
> > 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,

You are right. atomic_xchg() does not make gc_wake a counted request.

My patch only makes the existing boolean consume-and-clear operation atomic
with respect to an interleaving store. It still preserves the current boolean
semantics, where multiple gc_urgent writes may be coalesced into one wakeup.
If f2fs wanted to preserve every individual gc_urgent write as a separate
request, then a counter would be needed instead.

Given your explanation that this is not the intended semantic, this patch is
probably not worth changing the code for.

Please feel free to drop it.

Thanks,
Ziyu

>
> >
> >               /* 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);
> >                       }
>