Re: [f2fs-dev] [PATCH] f2fs: quiesce background threads during system suspend using PM notifier

On 8/5/26 01:35, Daeho Jeong wrote:
> On Tue, Aug 4, 2026 at 4:24 AM Chao Yu <[email protected]> wrote:
>>
>> On 8/4/26 02:42, Daeho Jeong wrote:
>>> On Mon, Aug 3, 2026 at 2:08 AM Chao Yu <[email protected]> wrote:
>>>>
>>>> On 7/30/26 03:23, Daeho Jeong wrote:
>>>>> From: Daeho Jeong <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>>   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 <linux/quotaops.h>
>>>>>   #include <linux/part_stat.h>
>>>>>   #include <linux/rw_hint.h>
>>>>> +#include <linux/suspend.h>
>>>>>
>>>>>   #include <linux/fscrypt.h>
>>>>>   #include <linux/fsverity.h>
>>>>> @@ -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;
>>>>>
>>>>
>>