From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 046652DCF67 for ; Sun, 17 May 2026 09:22:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779009739; cv=none; b=NABhPdjGQYYrQd2xN6i9UCBe5/eTquXeMqIq2K+XSQVUdgwxcLTpHDlO5fPahbYzWS357m2h0cH9bDcXKgTsibffmfKVG0rAOt+YVCLgGZ3v/pLBoL6SXbIaKBIxVUjXZ50kZ1lcnhB9EJAZd1Jk7W3nC2aEDJ70oz2U2diegOc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779009739; c=relaxed/simple; bh=pFCRIYmewKHusvusZYzm8Yz8hISgDPWeySFQX+oH4To=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=iLUgLo5GT15LXNT8C9zAm9ur4st0PLw4ECkT2SUfY/eU+y+smEpRqtCbKhGarwsqbRku6zm36sD+z7OKzB+auPkPQuXVHlpEiXnCBsnZFyynC6h7FLXo1Qpu+8dqwSjGVrHSqSXhWeDOliyHqrglibfjZrrEb8N1iYMElyAXjFI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=IzaI7Qrc; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="IzaI7Qrc" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 79E47C2BCB0; Sun, 17 May 2026 09:22:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1779009738; bh=pFCRIYmewKHusvusZYzm8Yz8hISgDPWeySFQX+oH4To=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=IzaI7QrciJ4L7EBZbXH+SwuqI+sdme/G+nYpiKhTLfwILxmtNjtarsVa+TlqZOvwi /NMPxOF9Fks6r/9vUooc8IBL+E9f2Hvk/c4uemiImcE2Uk2mubJIKwLC2Avhkt5jcO weTQXrC5utAFSjTrPo1RB3X6DrPhchCLc1/ii4v+Ld1tHqnIt9epQ4rZBPljmcH53O ovj7YP6rqGBwRdBCo3kDIfi7c6EN3azhc5n1SMeVBoqiSHtZQ3cWrybzB17QqSnyuc T5YS+5b10SC4qBz7ugw5Kw2IibmMwO/5BZWY56gSD9l3H8le3VlzotSKQmWkf1VpFB NCfnp0vo/mOIw== Message-ID: Date: Sun, 17 May 2026 17:22:09 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, baijiaju1990@gmail.com, r33s3n6@gmail.com, gality369@gmail.com, zhenghaoran154@gmail.com, hanguidong02@gmail.com, zzzccc427@gmail.com Subject: Re: [PATCH] f2fs: make gc_wake test-and-clear atomic To: Ziyu Zhang , Jaegeuk Kim References: <20260516035001.272335-1-ziyuzhang201@gmail.com> Content-Language: en-US From: Chao Yu In-Reply-To: <20260516035001.272335-1-ziyuzhang201@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 > --- > 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); > }