From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 4F2C436A369 for ; Wed, 5 Aug 2026 01:51:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785894668; cv=none; b=HSUVIr4aBvCfwWDqNufTOHIu2ESOv2IFoXBBfDPHsUKfNlGk9PlPO6/CO3jD4kOr3piaL20LOIzJLKRotc2oVl+3DJcAQQiHVvliTPOov2UBfJGAL3iCVstPylqulsIPYFKqGT6wjoD8zGPnR6KDdBvFd6G3cYxLQXJr9dzGHXE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785894668; c=relaxed/simple; bh=B1gueQo1ra6i8UX1rwEbiudzCexxkfLAQ83asnkfbsI=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=fPgK8HfZhZpJYT0kq3EEuqt3yPvttM0baeJ+kLmTvNModbhFskgtPhFFIkV9rQ0GxkG11gHfyvSuid4WVolEgtqBl8eqAGxD1wx49eZ7a5+t8TedfyO0YlpbtOQQTNQNBPNbzgS/0DP9/cjgL0ArEQGSQ8fNf0dKkJRENkWJHlg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=TqXjz5Gk; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="TqXjz5Gk" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9E64C1F000E9; Wed, 5 Aug 2026 01:51:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1785894666; bh=SnoPLtwIfP2xVQ3yNsAfZxAqq3ADBmzkFWQzNoDcG/I=; h=Date:Cc:Subject:To:References:From:In-Reply-To; b=TqXjz5Gka22jQgSpL2ZVorPzP0yk9/mafFxyy3vDuUXEpAQGVO/SXAgThCzwMNap7 DSvwnss0SHmudmM1xq84lkQS89e3vuObx4XAxI/Fbldw4tmMFXm2uI/LqmG9o1ZVvO d+J1NdkjWDqXUoCuXqcDlHhdsVYG6cEW3IGN5edoiHGaeV9kgIzOviOz4SZYPwCAUB 02rraFftLqk8uGL4NfIzrmDXcghvPeypqtVHFwSxMJGg/Ef/KxJ9SGTrvtALhOXgbo /NGT//pMKopKTqMqE6pJSoxBf3KbAttrqbHH25cqr8ZNHFcE/95mDc6SeaU4TtZPoE EXHddZ22VmNow== Message-ID: Date: Wed, 5 Aug 2026 09:51:03 +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-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com, Daeho Jeong Subject: Re: [f2fs-dev] [PATCH] f2fs: quiesce background threads during system suspend using PM notifier To: Daeho Jeong References: <20260729192319.4051409-1-daeho43@gmail.com> Content-Language: en-US From: Chao Yu In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 8/5/26 01:35, Daeho Jeong wrote: > On Tue, Aug 4, 2026 at 4:24 AM Chao Yu wrote: >> >> On 8/4/26 02:42, Daeho Jeong wrote: >>> On Mon, Aug 3, 2026 at 2:08 AM Chao Yu wrote: >>>> >>>> On 7/30/26 03:23, Daeho Jeong wrote: >>>>> From: Daeho Jeong >>>>> >>>>> During system suspend, a race condition can cause f2fs_gc and f2fs_discard >>>>> threads to call submit_bio() while the underlying block device (e.g., UFS) >>>>> is in Runtime PM suspend. Because Runtime PM worker threads are already >>>>> frozen during task freezing, the threads become trapped in >>>>> __bio_queue_enter() waiting on mq_freeze_wq, leading to a PM freezer >>>>> timeout. >>>>> >>>>> To prevent this deadlock, register a PM notifier to set SBI_IS_SUSPENDING >>>>> during PM_SUSPEND_PREPARE. Background GC and discard threads check this >>>> >>>> Should we cover issue_flush_thread and issue_checkpoint_thread as well in >>>> where we will submit bio? >>> >>> Unlike GC and discard, which are background optimization tasks and can >>> be safely paused, other operations like checkpoint are critical for >>> data consistency. >>> It is better not to interrupt them in the middle of their progress. If >>> they are running and take too long, it is safer to just let the system >>> suspend temporarily fail rather than forcibly breaking their >>> operations. >> >> So why foreground thread like ckpt thread or flush thread won't suffer the >> same issue like gc or discard thread? because foreground thread will prevent >> UFS from running into suspend state? I may missed something here. :) >> > > Foreground threads (ckpt/flush) issue I/O on-demand for dirty data sync. > If suspend aborts due to active I/O, it is legitimate and expected > behavior rather than an issue, as it is a necessary filesystem > operation under a non-idle workload. Okay, so if ckpt/flush are active, that means there are userspace applications are waiting for checkpoint/flush completion, so freeze_processes() form system suspend still didn't completion, and it won't enter phase 2 (device suspend & freezing pm_wq). Let me know if I understand it correctly. > In contrast, f2fs_gc and f2fs_discard are autonomous background > threads waking up even on an idle system with UFS in Runtime PM > suspend, which is the main cause of UFS deadlock when entering the > suspend. > With ZUFS, GC runs much more frequently, causing frequent suspend aborts. > >>> >>>> >>>>> flag and immediately stop issuing new bios, allowing them to enter a >>>>> freezable sleep state cleanly before process freezing begins. >>>>> >>>>> Signed-off-by: Daeho Jeong >>>>> --- >>>>> fs/f2fs/f2fs.h | 3 +++ >>>>> fs/f2fs/gc.c | 13 ++++++++----- >>>>> fs/f2fs/segment.c | 13 +++++++++---- >>>>> fs/f2fs/super.c | 25 +++++++++++++++++++++++++ >>>>> 4 files changed, 45 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>> index f24e30bb5c3d..c46bf4df9412 100644 >>>>> --- a/fs/f2fs/f2fs.h >>>>> +++ b/fs/f2fs/f2fs.h >>>>> @@ -25,6 +25,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include >>>>> #include >>>>> @@ -1494,6 +1495,7 @@ enum { >>>>> SBI_IS_FREEZING, /* freezefs is in process */ >>>>> SBI_IS_WRITABLE, /* remove ro mountoption transiently */ >>>>> SBI_ENABLE_CHECKPOINT, /* indicate it's during f2fs_enable_checkpoint() */ >>>>> + SBI_IS_SUSPENDING, /* system suspend is in progress */ >>>>> MAX_SBI_FLAG, >>>>> }; >>>>> >>>>> @@ -1757,6 +1759,7 @@ struct f2fs_sb_info { >>>>> struct f2fs_rwsem sb_lock; /* lock for raw super block */ >>>>> int valid_super_block; /* valid super block no */ >>>>> unsigned long s_flag; /* flags for sbi */ >>>>> + struct notifier_block pm_nb; /* for PM notifier */ >>>>> struct mutex writepages; /* mutex for writepages() */ >>>>> >>>>> #ifdef CONFIG_BLK_DEV_ZONED >>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>>> index 93bcb35a5b5d..86b2b29402a5 100644 >>>>> --- a/fs/f2fs/gc.c >>>>> +++ b/fs/f2fs/gc.c >>>>> @@ -71,7 +71,8 @@ static int gc_thread_func(void *data) >>>>> if (kthread_should_stop()) >>>>> break; >>>>> >>>>> - if (sbi->sb->s_writers.frozen >= SB_FREEZE_WRITE) { >>>>> + if (sbi->sb->s_writers.frozen >= SB_FREEZE_WRITE || >>>>> + is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) { >>>>> increase_sleep_time(gc_th, &wait_ms); >>>>> stat_other_skip_bggc_count(sbi); >>>>> continue; >>>>> @@ -1064,8 +1065,9 @@ static int gc_node_segment(struct f2fs_sb_info *sbi, >>>>> struct node_info ni; >>>>> int err; >>>>> >>>>> - /* stop BG_GC if there is not enough free sections. */ >>>>> - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) >>>>> + /* stop BG_GC if there is not enough free sections or suspending. */ >>>>> + if (gc_type == BG_GC && (has_not_enough_free_secs(sbi, 0, 0) || >>>>> + is_sbi_flag_set(sbi, SBI_IS_SUSPENDING))) >>>>> return submitted; >>>>> >>>>> if (check_valid_map(sbi, segno, off) == 0) >>>>> @@ -1611,7 +1613,8 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum, >>>>> * Or, stop GC if the segment becomes fully valid caused by >>>>> * race condition along with SSR block allocation. >>>>> */ >>>>> - if ((gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) || >>>>> + if ((gc_type == BG_GC && (has_not_enough_free_secs(sbi, 0, 0) || >>>>> + is_sbi_flag_set(sbi, SBI_IS_SUSPENDING))) || >>>>> (!force_migrate && get_valid_blocks(sbi, segno, true) == >>>>> CAP_BLKS_PER_SEC(sbi))) >>>>> return submitted; >>>>> @@ -2015,7 +2018,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>>>> goto stop; >>>>> } >>>>> retry: >>>>> - if (unlikely(freezing(current))) { >>>> >>>> Shouldn't we keep original freezing logic? in case filesystem are frozen >>>> when low device snapshot is triggered? >>> >>> Since the runtime PM suspend/resume workers are only frozen during >>> system-wide PM transitions (suspend/hibernation), the PM notifier >>> approach with SBI_IS_SUSPENDING sufficiently prevents the deadlock. >> >> What I mean is: >> >> if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING) || unlikely(freezing(current))) { >> >> otherwise, gc thread or discard thread won't detect freeze state, and will >> continue to trigger IO in background even there is a system freeze request >> from device snapshot or cgroup freezing, right? >> > > The hang in system suspend happens because Runtime PM workers (pm_wq) > are frozen, so UFS cannot be resumed from submit_bio(). > During cgroup freeze or snapshot, I believe pm_wq is alive, so UFS > resumes in a few ms and I/O finishes without deadlock. Also, > wait_event_freezable_timeout() will freeze the threads when they > sleep. Here, we need to detect the freezing state and stop issuing any new I/O immediately because non-PM freezing mechanisms (such as dm-snapshot or cgroup freezer) require the underlying filesystem/device to reach a quiescent (static) state. It's not limited strictly to the PM suspend state. > > If you prefer keeping freezing(current) as a fast path for non-PM > freezing, I can change it to: > if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING) || unlikely(freezing(current))) > Do you still think it is required? If so, plz, let me know. Yes, please. Thanks, > > Thanks. > >> Thanks, >> >>> >>> Thanks, >>> >>>> >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) { >>>>> ret = 0; >>>>> goto stop; >>>>> } >>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>>> index d70dc5ef3de4..e27197953356 100644 >>>>> --- a/fs/f2fs/segment.c >>>>> +++ b/fs/f2fs/segment.c >>>>> @@ -1300,7 +1300,8 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi, >>>>> if (dc->state != D_PREP) >>>>> return 0; >>>>> >>>>> - if (is_sbi_flag_set(sbi, SBI_NEED_FSCK)) >>>>> + if (is_sbi_flag_set(sbi, SBI_NEED_FSCK) || >>>>> + is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) >>>>> return 0; >>>>> >>>>> #ifdef CONFIG_BLK_DEV_ZONED >>>>> @@ -1341,6 +1342,9 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi, >>>>> unsigned long flags; >>>>> bool last = true; >>>>> >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) >>>>> + break; >>>>> + >>>>> if (len > max_discard_blocks) { >>>>> len = max_discard_blocks; >>>>> last = false; >>>>> @@ -1615,7 +1619,7 @@ static void __issue_discard_cmd_orderly(struct f2fs_sb_info *sbi, >>>>> if (dc->state != D_PREP) >>>>> goto next; >>>>> >>>>> - if (*issued > 0 && unlikely(freezing(current))) >>>> >>>> Ditto, >>>> >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) >>>>> break; >>>>> >>>>> if (dpolicy->io_aware && !is_idle(sbi, DISCARD_TIME)) { >>>>> @@ -1688,7 +1692,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi, >>>>> list_for_each_entry_safe(dc, tmp, pend_list, list) { >>>>> f2fs_bug_on(sbi, dc->state != D_PREP); >>>>> >>>>> - if (issued > 0 && unlikely(freezing(current))) { >>>> >>>> Ditto, >>>> >>>> Thanks, >>>> >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING)) { >>>>> suspended = true; >>>>> break; >>>>> } >>>>> @@ -1955,7 +1959,8 @@ static int issue_discard_thread(void *data) >>>>> continue; >>>>> if (kthread_should_stop()) >>>>> return 0; >>>>> - if (is_sbi_flag_set(sbi, SBI_NEED_FSCK) || >>>>> + if (is_sbi_flag_set(sbi, SBI_IS_SUSPENDING) || >>>>> + is_sbi_flag_set(sbi, SBI_NEED_FSCK) || >>>>> !atomic_read(&dcc->discard_cmd_cnt)) { >>>>> wait_ms = dpolicy.max_interval; >>>>> continue; >>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>> index d5dc83e613e2..536f3ffe5354 100644 >>>>> --- a/fs/f2fs/super.c >>>>> +++ b/fs/f2fs/super.c >>>>> @@ -1979,6 +1979,26 @@ static void destroy_device_list(struct f2fs_sb_info *sbi) >>>>> kvfree(sbi->devs); >>>>> } >>>>> >>>>> +static int f2fs_pm_notifier(struct notifier_block *nb, >>>>> + unsigned long action, void *ptr) >>>>> +{ >>>>> + struct f2fs_sb_info *sbi = container_of(nb, struct f2fs_sb_info, pm_nb); >>>>> + >>>>> + switch (action) { >>>>> + case PM_HIBERNATION_PREPARE: >>>>> + case PM_SUSPEND_PREPARE: >>>>> + case PM_RESTORE_PREPARE: >>>>> + set_sbi_flag(sbi, SBI_IS_SUSPENDING); >>>>> + break; >>>>> + case PM_POST_SUSPEND: >>>>> + case PM_POST_HIBERNATION: >>>>> + case PM_POST_RESTORE: >>>>> + clear_sbi_flag(sbi, SBI_IS_SUSPENDING); >>>>> + break; >>>>> + } >>>>> + return NOTIFY_OK; >>>>> +} >>>>> + >>>>> static void f2fs_put_super(struct super_block *sb) >>>>> { >>>>> struct f2fs_sb_info *sbi = F2FS_SB(sb); >>>>> @@ -1986,6 +2006,8 @@ static void f2fs_put_super(struct super_block *sb) >>>>> int err = 0; >>>>> bool done; >>>>> >>>>> + unregister_pm_notifier(&sbi->pm_nb); >>>>> + >>>>> /* unregister procfs/sysfs entries in advance to avoid race case */ >>>>> f2fs_unregister_sysfs(sbi); >>>>> >>>>> @@ -5472,6 +5494,9 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc) >>>>> >>>>> f2fs_restore_device_alias(sbi); >>>>> >>>>> + sbi->pm_nb.notifier_call = f2fs_pm_notifier; >>>>> + register_pm_notifier(&sbi->pm_nb); >>>>> + >>>>> sbi->umount_lock_holder = NULL; >>>>> return 0; >>>>> >>>> >>