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 2245D440A02 for ; Tue, 4 Aug 2026 11:24:42 +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=1785842684; cv=none; b=HXYJtwBPqPI4ArfLQj5flvrozyW+Luo5sdd/hLavxptUQN27CGqwzW6RzLIqu+UJb3EHe+3UIrUgZZyQ2RkPcPYFvRSi3wwr7Qww5xQfhuFm0Vrr+NZLQaDDGDEOtfLFlyS50uF0TUoqd1TUONSfzzzAtNhZakLLgfV8VwltAMs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785842684; c=relaxed/simple; bh=52c9k7MZVFSzPJd5s2XbjxP3Bh9P079IZuQiR7pQusM=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=jeHm1Co4ggagnn/xkANLoQ0xe1du4uOixZQpR60BCVMjNcXHDt8eFgo9l8RfOwtiUfdnQEzfGeNmCjwKTPxaM7M4xB1XPj8ORUtXBvC1hzmMMpAtuIFUIVu8BZgj5K/eDUd7YFKmpv1xc7yNCtbQqWWC7RGUK0TxeoGX8qflX5o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=B1j/wEJi; 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="B1j/wEJi" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9829F1F00A3F; Tue, 4 Aug 2026 11:24:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1785842682; bh=Z6Q6n9Lg3CHpwDAr8d++DEUcj9uotVFBZ/V6HFSliYU=; h=Date:Cc:Subject:To:References:From:In-Reply-To; b=B1j/wEJiTEyQXCU46vr4lu29HSSHfBdyycJx6gA65ngHiBezp58OrKegKcfqBcug+ +5BDIqdsLe2Ph74mnvWmLDGLt1hFdX0QUAT6v5pr3wID9cZ8bqvo9qi7PPpA8NrydL flbUSTk8XTOxhlM8KHNn5bFaU1NeOJxYCOTEmTkgV/qcw+clPPTjmnebphTJlJRCEo Du8vLlFJI+9mQVEj3EVuWv9wjEFnm/HxkGD07UWVjoGmQHhu1dHezchvBr3MMEtkag j7XYnsd94178b2JJPiqSAPwg6/ySSMMS3su8ImblSKWsnwg4ez25Q07PfTER771Seb ZtIMVUUnJ7CtQ== Message-ID: Date: Tue, 4 Aug 2026 19:24:39 +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/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. :) > >> >>> 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? 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; >>> >>