[PATCH v4] PM: Support aborting sleep during filesystem sync

[PATCH v4] PM: Support aborting sleep during filesystem sync

[Samuel Wu]

At the start of suspend and hibernate, filesystems will sync to save the
current state of the device. However, the long tail of the filesystem
sync can take upwards of 25 seconds. If during this filesystem sync
there is some wakeup or abort signal, it will not be processed until the
sync is complete; from a user's perspective, this looks like the device
is unresponsive to any form of input.

This patch adds functionality to handle a sleep abort signal when in
the filesystem sync phase of suspend or hibernate. This topic was first
discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
where the general consensus was to allow filesystem sync on a parallel
thread. The same logic applies to both suspend and hibernate code paths.

There is extra care needed to account for back-to-back sleeps while
still maintaining functionality to immediately abort during the
filesystem sync stage.

This patch handles this by serializing the filesystem sync sequence with
an invariant; a subsequent sleep's filesystem sync operation will only
start when the previous sleep's filesystem sync has finished. While
waiting for the previous sleep's filesystem sync to finish, the
subsequent sleep will still abort early if a wakeup event is triggered,
solving the original issue of filesystem sync blocking abort.

[1]: https://lpc.events/event/18/contributions/1845/

Suggested-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Samuel Wu <wusamuel@google.com>
---
Changes in v4:
- Removed patch 1/3 of v3 as it is already picked up on linux-pm
- Squashed patches 2/3 and 3/3 from v3 into this single patch
- Added abort during fs_sync functionality to hibernate in addition to suspend
- Moved variables and functions for abort from power/suspend.c to power/main.c
- Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
- Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
- v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/

Changes in v3:
- Split v2 patch into 3 patches
- Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
- Updated documentation and comments within kernel/power/suspend.c
- v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/

Changes in v2:
- Added documentation for suspend_abort_fs_sync()
- Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
- v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com

 drivers/base/power/wakeup.c |  8 +++++
 include/linux/suspend.h     |  4 +++
 kernel/power/hibernate.c    |  5 ++-
 kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
 kernel/power/suspend.c      |  7 ++--
 5 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index d1283ff1080b..daf07ab7ac3f 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 
 	/* Increment the counter of events in progress. */
 	cec = atomic_inc_return(&combined_event_count);
+	/*
+	 * wakeup_source_activate() aborts sleep only if events_check_enabled
+	 * is set (see pm_wakeup_pending()). Similarly, abort sleep during
+	 * fs_sync only if events_check_enabled is set.
+	 */
+	if (events_check_enabled)
+		abort_sleep_during_fs_sync();
 
 	trace_wakeup_source_activate(ws->name, cec);
 }
@@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
 void pm_system_wakeup(void)
 {
 	atomic_inc(&pm_abort_suspend);
+	abort_sleep_during_fs_sync();
 	s2idle_wake();
 }
 EXPORT_SYMBOL_GPL(pm_system_wakeup);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 317ae31e89b3..c961bdb00bb6 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -444,6 +444,8 @@ void restore_processor_state(void);
 extern int register_pm_notifier(struct notifier_block *nb);
 extern int unregister_pm_notifier(struct notifier_block *nb);
 extern void ksys_sync_helper(void);
+extern void abort_sleep_during_fs_sync(void);
+extern int pm_sleep_fs_sync(void);
 extern void pm_report_hw_sleep_time(u64 t);
 extern void pm_report_max_hw_sleep(u64 t);
 void pm_restrict_gfp_mask(void);
@@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
 static inline void pm_restore_gfp_mask(void) {}
 
 static inline void ksys_sync_helper(void) {}
+static inline abort_sleep_during_fs_sync(void) {}
+static inline int pm_sleep_fs_sync(void) {}
 
 #define pm_notifier(fn, pri)	do { (void)(fn); } while (0)
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2f66ab453823..651dcd768644 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -811,7 +811,10 @@ int hibernate(void)
 	if (error)
 		goto Restore;
 
-	ksys_sync_helper();
+	error = pm_sleep_fs_sync();
+	if (error)
+		goto Restore;
+
 	if (filesystem_freeze_enabled)
 		filesystems_freeze();
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 3cf2d7e72567..38b1de295cfe 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
 {
 	return pm_suspend_in_progress() || hibernation_in_progress();
 }
+
+static bool pm_sleep_fs_sync_queued;
+static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
+static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
+
+/**
+ * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
+ *
+ * This function aborts the fs_sync stage of suspend/hibernate so that
+ * suspend/hibernate itself can be aborted early.
+ */
+void abort_sleep_during_fs_sync(void)
+{
+	spin_lock(&pm_sleep_fs_sync_lock);
+	complete(&pm_sleep_fs_sync_complete);
+	spin_unlock(&pm_sleep_fs_sync_lock);
+}
+
+static void sync_filesystems_fn(struct work_struct *work)
+{
+	ksys_sync_helper();
+
+	spin_lock(&pm_sleep_fs_sync_lock);
+	pm_sleep_fs_sync_queued = false;
+	complete(&pm_sleep_fs_sync_complete);
+	spin_unlock(&pm_sleep_fs_sync_lock);
+}
+static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
+
+/**
+ * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
+ *
+ * Return 0 on successful file system sync, otherwise returns -EBUSY if file
+ * system sync was aborted.
+ */
+int pm_sleep_fs_sync(void)
+{
+	bool need_pm_sleep_fs_sync_requeue;
+
+Start_fs_sync:
+	spin_lock(&pm_sleep_fs_sync_lock);
+	reinit_completion(&pm_sleep_fs_sync_complete);
+	/*
+	 * Handle the case where a sleep immediately follows a previous sleep
+	 * that was aborted during fs_sync. In this case, wait for the previous
+	 * filesystem sync to finish. Then do another filesystem sync so any
+	 * subsequent filesystem changes are synced before sleeping.
+	 */
+	if (pm_sleep_fs_sync_queued) {
+		need_pm_sleep_fs_sync_requeue = true;
+	} else {
+		need_pm_sleep_fs_sync_requeue = false;
+		pm_sleep_fs_sync_queued = true;
+		schedule_work(&sync_filesystems);
+	}
+	spin_unlock(&pm_sleep_fs_sync_lock);
+
+	/*
+	 * Completion is triggered by fs_sync finishing or an abort sleep
+	 * signal, whichever comes first
+	 */
+	wait_for_completion(&pm_sleep_fs_sync_complete);
+	if (pm_wakeup_pending())
+		return -EBUSY;
+	if (need_pm_sleep_fs_sync_requeue)
+		goto Start_fs_sync;
+
+	return 0;
+}
+
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_PM_SLEEP_DEBUG
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 4bb4686c1c08..c019a4396c1f 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -31,6 +31,7 @@
 #include <linux/compiler.h>
 #include <linux/moduleparam.h>
 #include <linux/fs.h>
+#include <linux/workqueue.h>
 
 #include "power.h"
 
@@ -588,14 +589,16 @@ static int enter_state(suspend_state_t state)
 	if (state == PM_SUSPEND_TO_IDLE)
 		s2idle_begin();
 
+	pm_wakeup_clear(0);
 	if (sync_on_suspend_enabled) {
 		trace_suspend_resume(TPS("sync_filesystems"), 0, true);
-		ksys_sync_helper();
+		error = pm_sleep_fs_sync();
 		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
+		if (error)
+			goto Unlock;
 	}
 
 	pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
-	pm_wakeup_clear(0);
 	pm_suspend_clear_flags();
 	error = suspend_prepare(state);
 	if (error)
-- 
2.51.0.384.g4c02a37b29-goog

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Samuel Wu]

Hi Rafael,

Just a friendly ping on this patch. Please let me know if there's any
feedback or if you'd like me to make any changes.

Thanks,
Sam

On Thu, Sep 11, 2025 at 11:53 AM Samuel Wu <wusamuel@google.com> wrote:
>
> At the start of suspend and hibernate, filesystems will sync to save the
> current state of the device. However, the long tail of the filesystem
> sync can take upwards of 25 seconds. If during this filesystem sync
> there is some wakeup or abort signal, it will not be processed until the
> sync is complete; from a user's perspective, this looks like the device
> is unresponsive to any form of input.
>
> This patch adds functionality to handle a sleep abort signal when in
> the filesystem sync phase of suspend or hibernate. This topic was first
> discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> where the general consensus was to allow filesystem sync on a parallel
> thread. The same logic applies to both suspend and hibernate code paths.
>
> There is extra care needed to account for back-to-back sleeps while
> still maintaining functionality to immediately abort during the
> filesystem sync stage.
>
> This patch handles this by serializing the filesystem sync sequence with
> an invariant; a subsequent sleep's filesystem sync operation will only
> start when the previous sleep's filesystem sync has finished. While
> waiting for the previous sleep's filesystem sync to finish, the
> subsequent sleep will still abort early if a wakeup event is triggered,
> solving the original issue of filesystem sync blocking abort.
>
> [1]: https://lpc.events/event/18/contributions/1845/
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Samuel Wu <wusamuel@google.com>
> ---
> Changes in v4:
> - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> - Squashed patches 2/3 and 3/3 from v3 into this single patch
> - Added abort during fs_sync functionality to hibernate in addition to suspend
> - Moved variables and functions for abort from power/suspend.c to power/main.c
> - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
>
> Changes in v3:
> - Split v2 patch into 3 patches
> - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> - Updated documentation and comments within kernel/power/suspend.c
> - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
>
> Changes in v2:
> - Added documentation for suspend_abort_fs_sync()
> - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
>
>  drivers/base/power/wakeup.c |  8 +++++
>  include/linux/suspend.h     |  4 +++
>  kernel/power/hibernate.c    |  5 ++-
>  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
>  kernel/power/suspend.c      |  7 ++--
>  5 files changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..daf07ab7ac3f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>
>         /* Increment the counter of events in progress. */
>         cec = atomic_inc_return(&combined_event_count);
> +       /*
> +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> +        * fs_sync only if events_check_enabled is set.
> +        */
> +       if (events_check_enabled)
> +               abort_sleep_during_fs_sync();
>
>         trace_wakeup_source_activate(ws->name, cec);
>  }
> @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
>  void pm_system_wakeup(void)
>  {
>         atomic_inc(&pm_abort_suspend);
> +       abort_sleep_during_fs_sync();
>         s2idle_wake();
>  }
>  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 317ae31e89b3..c961bdb00bb6 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -444,6 +444,8 @@ void restore_processor_state(void);
>  extern int register_pm_notifier(struct notifier_block *nb);
>  extern int unregister_pm_notifier(struct notifier_block *nb);
>  extern void ksys_sync_helper(void);
> +extern void abort_sleep_during_fs_sync(void);
> +extern int pm_sleep_fs_sync(void);
>  extern void pm_report_hw_sleep_time(u64 t);
>  extern void pm_report_max_hw_sleep(u64 t);
>  void pm_restrict_gfp_mask(void);
> @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
>  static inline void pm_restore_gfp_mask(void) {}
>
>  static inline void ksys_sync_helper(void) {}
> +static inline abort_sleep_during_fs_sync(void) {}
> +static inline int pm_sleep_fs_sync(void) {}
>
>  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2f66ab453823..651dcd768644 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -811,7 +811,10 @@ int hibernate(void)
>         if (error)
>                 goto Restore;
>
> -       ksys_sync_helper();
> +       error = pm_sleep_fs_sync();
> +       if (error)
> +               goto Restore;
> +
>         if (filesystem_freeze_enabled)
>                 filesystems_freeze();
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 3cf2d7e72567..38b1de295cfe 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
>  {
>         return pm_suspend_in_progress() || hibernation_in_progress();
>  }
> +
> +static bool pm_sleep_fs_sync_queued;
> +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> +
> +/**
> + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> + *
> + * This function aborts the fs_sync stage of suspend/hibernate so that
> + * suspend/hibernate itself can be aborted early.
> + */
> +void abort_sleep_during_fs_sync(void)
> +{
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       complete(&pm_sleep_fs_sync_complete);
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +}
> +
> +static void sync_filesystems_fn(struct work_struct *work)
> +{
> +       ksys_sync_helper();
> +
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       pm_sleep_fs_sync_queued = false;
> +       complete(&pm_sleep_fs_sync_complete);
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +}
> +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> +
> +/**
> + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> + *
> + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> + * system sync was aborted.
> + */
> +int pm_sleep_fs_sync(void)
> +{
> +       bool need_pm_sleep_fs_sync_requeue;
> +
> +Start_fs_sync:
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       reinit_completion(&pm_sleep_fs_sync_complete);
> +       /*
> +        * Handle the case where a sleep immediately follows a previous sleep
> +        * that was aborted during fs_sync. In this case, wait for the previous
> +        * filesystem sync to finish. Then do another filesystem sync so any
> +        * subsequent filesystem changes are synced before sleeping.
> +        */
> +       if (pm_sleep_fs_sync_queued) {
> +               need_pm_sleep_fs_sync_requeue = true;
> +       } else {
> +               need_pm_sleep_fs_sync_requeue = false;
> +               pm_sleep_fs_sync_queued = true;
> +               schedule_work(&sync_filesystems);
> +       }
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +
> +       /*
> +        * Completion is triggered by fs_sync finishing or an abort sleep
> +        * signal, whichever comes first
> +        */
> +       wait_for_completion(&pm_sleep_fs_sync_complete);
> +       if (pm_wakeup_pending())
> +               return -EBUSY;
> +       if (need_pm_sleep_fs_sync_requeue)
> +               goto Start_fs_sync;
> +
> +       return 0;
> +}
> +
>  #endif /* CONFIG_PM_SLEEP */
>
>  #ifdef CONFIG_PM_SLEEP_DEBUG
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4bb4686c1c08..c019a4396c1f 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -31,6 +31,7 @@
>  #include <linux/compiler.h>
>  #include <linux/moduleparam.h>
>  #include <linux/fs.h>
> +#include <linux/workqueue.h>
>
>  #include "power.h"
>
> @@ -588,14 +589,16 @@ static int enter_state(suspend_state_t state)
>         if (state == PM_SUSPEND_TO_IDLE)
>                 s2idle_begin();
>
> +       pm_wakeup_clear(0);
>         if (sync_on_suspend_enabled) {
>                 trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> -               ksys_sync_helper();
> +               error = pm_sleep_fs_sync();
>                 trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> +               if (error)
> +                       goto Unlock;
>         }
>
>         pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> -       pm_wakeup_clear(0);
>         pm_suspend_clear_flags();
>         error = suspend_prepare(state);
>         if (error)
> --
> 2.51.0.384.g4c02a37b29-goog
>

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

Hi,

On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
>
> Hi Rafael,
>
> Just a friendly ping on this patch. Please let me know if there's any
> feedback or if you'd like me to make any changes.

Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
?

If so, what do you think about it?

If not, please see it.

Thanks!

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Samuel Wu]

On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi,
>
> On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> >
> > Hi Rafael,
> >
> > Just a friendly ping on this patch. Please let me know if there's any
> > feedback or if you'd like me to make any changes.
>
> Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> ?
>
> If so, what do you think about it?

I was following this chain
(https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
where there is some ongoing discussion on converging the solution.

Our changes aren't mutually exclusive, and tuhaowen can build changes
on top of ours, even indicating:
> I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.

These patchsets don't negate each other, so could we decouple these
two patchsets since they address different issues?

Thanks,
Sam
>
> If not, please see it.
>
> Thanks!

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Wed, Oct 1, 2025 at 12:37 AM Samuel Wu <wusamuel@google.com> wrote:
>
> On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > Just a friendly ping on this patch. Please let me know if there's any
> > > feedback or if you'd like me to make any changes.
> >
> > Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> > ?
> >
> > If so, what do you think about it?
>
> I was following this chain
> (https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
> where there is some ongoing discussion on converging the solution.
>
> Our changes aren't mutually exclusive, and tuhaowen can build changes
> on top of ours, even indicating:
> > I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.

That's fair.

> These patchsets don't negate each other, so could we decouple these
> two patchsets since they address different issues?

Well, I'm not sure if they are really different.  After all, this is
all about avoiding having to wait on an excessively long filesystem
sync during suspend.

I'm also not sure why it is being pursued to be honest.  Is setting
/sys/power/sync_on_suspend to 0 not regarded as a viable option?

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Saravana Kannan]

On Wed, Oct 1, 2025 at 11:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Oct 1, 2025 at 12:37 AM Samuel Wu <wusamuel@google.com> wrote:
> >
> > On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > Just a friendly ping on this patch. Please let me know if there's any
> > > > feedback or if you'd like me to make any changes.
> > >
> > > Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> > > ?
> > >
> > > If so, what do you think about it?
> >
> > I was following this chain
> > (https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
> > where there is some ongoing discussion on converging the solution.
> >
> > Our changes aren't mutually exclusive, and tuhaowen can build changes
> > on top of ours, even indicating:
> > > I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.
>
> That's fair.
>
> > These patchsets don't negate each other, so could we decouple these
> > two patchsets since they address different issues?
>
> Well, I'm not sure if they are really different.  After all, this is
> all about avoiding having to wait on an excessively long filesystem
> sync during suspend.

No, it's different. We don't mind a long filesystem sync if we don't
have a need to abort a suspend. If it takes 25 seconds to sync the
filesystem but there's no need to abort it, that's totally fine. So,
this patch is just about allowing abort to happen without waiting for
file system sync to finish.

The other patch's requirement is to always abort if suspend takes 25
seconds (or whatever the timeout is). IIRC, in his case, it's because
of a bad disk or say a USB disk getting unplugged. I'm not convinced a
suspend timeout is the right thing to do, but I'm not going to nack
it. But to implement his requirement, he can put a patch on top of
ours where he sets a timer and then aborts suspends if it fires.

> I'm also not sure why it is being pursued to be honest.  Is setting
> /sys/power/sync_on_suspend to 0 not regarded as a viable option?

We do want to sync on every suspend. So, we don't want to turn it off
completely.

Thanks,
Saravana

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Thu, Oct 2, 2025 at 12:13 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Oct 1, 2025 at 11:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Oct 1, 2025 at 12:37 AM Samuel Wu <wusamuel@google.com> wrote:
> > >
> > > On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> > > > >
> > > > > Hi Rafael,
> > > > >
> > > > > Just a friendly ping on this patch. Please let me know if there's any
> > > > > feedback or if you'd like me to make any changes.
> > > >
> > > > Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> > > > ?
> > > >
> > > > If so, what do you think about it?
> > >
> > > I was following this chain
> > > (https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
> > > where there is some ongoing discussion on converging the solution.
> > >
> > > Our changes aren't mutually exclusive, and tuhaowen can build changes
> > > on top of ours, even indicating:
> > > > I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.
> >
> > That's fair.
> >
> > > These patchsets don't negate each other, so could we decouple these
> > > two patchsets since they address different issues?
> >
> > Well, I'm not sure if they are really different.  After all, this is
> > all about avoiding having to wait on an excessively long filesystem
> > sync during suspend.
>
> No, it's different. We don't mind a long filesystem sync if we don't
> have a need to abort a suspend. If it takes 25 seconds to sync the
> filesystem but there's no need to abort it, that's totally fine. So,
> this patch is just about allowing abort to happen without waiting for
> file system sync to finish.

OK, thanks for clarification.

> The other patch's requirement is to always abort if suspend takes 25
> seconds (or whatever the timeout is). IIRC, in his case, it's because
> of a bad disk or say a USB disk getting unplugged. I'm not convinced a
> suspend timeout is the right thing to do, but I'm not going to nack
> it. But to implement his requirement, he can put a patch on top of
> ours where he sets a timer and then aborts suspends if it fires.
>
> > I'm also not sure why it is being pursued to be honest.  Is setting
> > /sys/power/sync_on_suspend to 0 not regarded as a viable option?
>
> We do want to sync on every suspend. So, we don't want to turn it off
> completely.

I wonder why though.

If suspend/resume works reliably, syncing filesystems on every attempt
to suspend the system isn't particularly useful AFAICS.

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Saravana Kannan]

On Mon, Oct 13, 2025 at 11:02 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Oct 2, 2025 at 12:13 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Oct 1, 2025 at 11:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Oct 1, 2025 at 12:37 AM Samuel Wu <wusamuel@google.com> wrote:
> > > >
> > > > On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> > > > > >
> > > > > > Hi Rafael,
> > > > > >
> > > > > > Just a friendly ping on this patch. Please let me know if there's any
> > > > > > feedback or if you'd like me to make any changes.
> > > > >
> > > > > Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> > > > > ?
> > > > >
> > > > > If so, what do you think about it?
> > > >
> > > > I was following this chain
> > > > (https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
> > > > where there is some ongoing discussion on converging the solution.
> > > >
> > > > Our changes aren't mutually exclusive, and tuhaowen can build changes
> > > > on top of ours, even indicating:
> > > > > I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.
> > >
> > > That's fair.
> > >
> > > > These patchsets don't negate each other, so could we decouple these
> > > > two patchsets since they address different issues?
> > >
> > > Well, I'm not sure if they are really different.  After all, this is
> > > all about avoiding having to wait on an excessively long filesystem
> > > sync during suspend.
> >
> > No, it's different. We don't mind a long filesystem sync if we don't
> > have a need to abort a suspend. If it takes 25 seconds to sync the
> > filesystem but there's no need to abort it, that's totally fine. So,
> > this patch is just about allowing abort to happen without waiting for
> > file system sync to finish.
>
> OK, thanks for clarification.
>
> > The other patch's requirement is to always abort if suspend takes 25
> > seconds (or whatever the timeout is). IIRC, in his case, it's because
> > of a bad disk or say a USB disk getting unplugged. I'm not convinced a
> > suspend timeout is the right thing to do, but I'm not going to nack
> > it. But to implement his requirement, he can put a patch on top of
> > ours where he sets a timer and then aborts suspends if it fires.
> >
> > > I'm also not sure why it is being pursued to be honest.  Is setting
> > > /sys/power/sync_on_suspend to 0 not regarded as a viable option?
> >
> > We do want to sync on every suspend. So, we don't want to turn it off
> > completely.
>
> I wonder why though.
>
> If suspend/resume works reliably, syncing filesystems on every attempt
> to suspend the system isn't particularly useful AFAICS.

A lot of people's entire digital life is on their phone. Even if
there's just 1% crash, when you look at that across billions of users
it adds up to serious impact.

Also, there are classes of devices where we do disable sync on suspend
(I can't share specifics). So, this is not an arbitrary stance. For
phones, we want to sync on suspend.

Also, your question could be posed equally to laptops too. If suspend
is reliable, then the laptop can always wake up at 3% battery, sync,
and then shutdown. But that's a leap of faith none of us want to make.
So, as long as we support sync on suspend, I think it makes sense to
allow suspend aborts to happen more gracefully.

Thanks,
Saravana

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@google.com> wrote:
>
> At the start of suspend and hibernate, filesystems will sync to save the
> current state of the device. However, the long tail of the filesystem
> sync can take upwards of 25 seconds. If during this filesystem sync
> there is some wakeup or abort signal, it will not be processed until the
> sync is complete; from a user's perspective, this looks like the device
> is unresponsive to any form of input.
>
> This patch adds functionality to handle a sleep abort signal when in
> the filesystem sync phase of suspend or hibernate. This topic was first
> discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> where the general consensus was to allow filesystem sync on a parallel
> thread. The same logic applies to both suspend and hibernate code paths.
>
> There is extra care needed to account for back-to-back sleeps while
> still maintaining functionality to immediately abort during the
> filesystem sync stage.
>
> This patch handles this by serializing the filesystem sync sequence with
> an invariant; a subsequent sleep's filesystem sync operation will only
> start when the previous sleep's filesystem sync has finished. While
> waiting for the previous sleep's filesystem sync to finish, the
> subsequent sleep will still abort early if a wakeup event is triggered,
> solving the original issue of filesystem sync blocking abort.
>
> [1]: https://lpc.events/event/18/contributions/1845/
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Samuel Wu <wusamuel@google.com>
> ---
> Changes in v4:
> - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> - Squashed patches 2/3 and 3/3 from v3 into this single patch
> - Added abort during fs_sync functionality to hibernate in addition to suspend
> - Moved variables and functions for abort from power/suspend.c to power/main.c
> - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
>
> Changes in v3:
> - Split v2 patch into 3 patches
> - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> - Updated documentation and comments within kernel/power/suspend.c
> - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
>
> Changes in v2:
> - Added documentation for suspend_abort_fs_sync()
> - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
>
>  drivers/base/power/wakeup.c |  8 +++++
>  include/linux/suspend.h     |  4 +++
>  kernel/power/hibernate.c    |  5 ++-
>  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
>  kernel/power/suspend.c      |  7 ++--
>  5 files changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..daf07ab7ac3f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>
>         /* Increment the counter of events in progress. */
>         cec = atomic_inc_return(&combined_event_count);
> +       /*
> +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> +        * fs_sync only if events_check_enabled is set.
> +        */
> +       if (events_check_enabled)
> +               abort_sleep_during_fs_sync();
>
>         trace_wakeup_source_activate(ws->name, cec);
>  }
> @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
>  void pm_system_wakeup(void)
>  {
>         atomic_inc(&pm_abort_suspend);
> +       abort_sleep_during_fs_sync();
>         s2idle_wake();
>  }
>  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 317ae31e89b3..c961bdb00bb6 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -444,6 +444,8 @@ void restore_processor_state(void);
>  extern int register_pm_notifier(struct notifier_block *nb);
>  extern int unregister_pm_notifier(struct notifier_block *nb);
>  extern void ksys_sync_helper(void);
> +extern void abort_sleep_during_fs_sync(void);
> +extern int pm_sleep_fs_sync(void);
>  extern void pm_report_hw_sleep_time(u64 t);
>  extern void pm_report_max_hw_sleep(u64 t);
>  void pm_restrict_gfp_mask(void);
> @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
>  static inline void pm_restore_gfp_mask(void) {}
>
>  static inline void ksys_sync_helper(void) {}
> +static inline abort_sleep_during_fs_sync(void) {}
> +static inline int pm_sleep_fs_sync(void) {}
>
>  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2f66ab453823..651dcd768644 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -811,7 +811,10 @@ int hibernate(void)
>         if (error)
>                 goto Restore;
>
> -       ksys_sync_helper();
> +       error = pm_sleep_fs_sync();
> +       if (error)
> +               goto Restore;
> +
>         if (filesystem_freeze_enabled)
>                 filesystems_freeze();
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 3cf2d7e72567..38b1de295cfe 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
>  {
>         return pm_suspend_in_progress() || hibernation_in_progress();
>  }
> +
> +static bool pm_sleep_fs_sync_queued;
> +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> +
> +/**
> + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> + *
> + * This function aborts the fs_sync stage of suspend/hibernate so that
> + * suspend/hibernate itself can be aborted early.

This changelog needs to be more precise IMV.

I'd actually call the function something like
pm_stop_waiting_for_fs_sync() and I'd say in the changelog that the
functions causes a suspend process to stop waiting on an fs sync in
progress and continue so that it can be aborted before the fs sync is
complete.

> + */
> +void abort_sleep_during_fs_sync(void)
> +{
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       complete(&pm_sleep_fs_sync_complete);
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +}
> +
> +static void sync_filesystems_fn(struct work_struct *work)
> +{
> +       ksys_sync_helper();
> +
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       pm_sleep_fs_sync_queued = false;
> +       complete(&pm_sleep_fs_sync_complete);
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +}
> +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> +
> +/**
> + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> + *
> + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> + * system sync was aborted.
> + */
> +int pm_sleep_fs_sync(void)
> +{
> +       bool need_pm_sleep_fs_sync_requeue;
> +
> +Start_fs_sync:
> +       spin_lock(&pm_sleep_fs_sync_lock);
> +       reinit_completion(&pm_sleep_fs_sync_complete);
> +       /*
> +        * Handle the case where a sleep immediately follows a previous sleep
> +        * that was aborted during fs_sync. In this case, wait for the previous
> +        * filesystem sync to finish. Then do another filesystem sync so any
> +        * subsequent filesystem changes are synced before sleeping.

Is the extra sync really necessary?

Some files may still be updated after it is complete and before all
tasks are frozen.

> +        */
> +       if (pm_sleep_fs_sync_queued) {
> +               need_pm_sleep_fs_sync_requeue = true;
> +       } else {
> +               need_pm_sleep_fs_sync_requeue = false;
> +               pm_sleep_fs_sync_queued = true;
> +               schedule_work(&sync_filesystems);
> +       }
> +       spin_unlock(&pm_sleep_fs_sync_lock);
> +
> +       /*
> +        * Completion is triggered by fs_sync finishing or an abort sleep
> +        * signal, whichever comes first
> +        */
> +       wait_for_completion(&pm_sleep_fs_sync_complete);
> +       if (pm_wakeup_pending())
> +               return -EBUSY;
> +       if (need_pm_sleep_fs_sync_requeue)
> +               goto Start_fs_sync;

Wouldn't a do { .. } while () work here instead of the goto?

> +
> +       return 0;
> +}
> +
>  #endif /* CONFIG_PM_SLEEP */
>
>  #ifdef CONFIG_PM_SLEEP_DEBUG
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4bb4686c1c08..c019a4396c1f 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -31,6 +31,7 @@
>  #include <linux/compiler.h>
>  #include <linux/moduleparam.h>
>  #include <linux/fs.h>
> +#include <linux/workqueue.h>
>
>  #include "power.h"
>
> @@ -588,14 +589,16 @@ static int enter_state(suspend_state_t state)
>         if (state == PM_SUSPEND_TO_IDLE)
>                 s2idle_begin();
>
> +       pm_wakeup_clear(0);
>         if (sync_on_suspend_enabled) {
>                 trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> -               ksys_sync_helper();
> +               error = pm_sleep_fs_sync();
>                 trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> +               if (error)
> +                       goto Unlock;
>         }
>
>         pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> -       pm_wakeup_clear(0);
>         pm_suspend_clear_flags();
>         error = suspend_prepare(state);
>         if (error)
> --

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Mon, Oct 13, 2025 at 8:13 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Oct 13, 2025 at 11:02 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Oct 2, 2025 at 12:13 AM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Wed, Oct 1, 2025 at 11:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Oct 1, 2025 at 12:37 AM Samuel Wu <wusamuel@google.com> wrote:
> > > > >
> > > > > On Tue, Sep 30, 2025 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tue, Sep 30, 2025 at 8:30 PM Samuel Wu <wusamuel@google.com> wrote:
> > > > > > >
> > > > > > > Hi Rafael,
> > > > > > >
> > > > > > > Just a friendly ping on this patch. Please let me know if there's any
> > > > > > > feedback or if you'd like me to make any changes.
> > > > > >
> > > > > > Have you seen https://lore.kernel.org/all/20250909065836.32534-1-tuhaowen@uniontech.com/
> > > > > > ?
> > > > > >
> > > > > > If so, what do you think about it?
> > > > >
> > > > > I was following this chain
> > > > > (https://lore.kernel.org/all/20250908024655.14636-1-tuhaowen@uniontech.com/),
> > > > > where there is some ongoing discussion on converging the solution.
> > > > >
> > > > > Our changes aren't mutually exclusive, and tuhaowen can build changes
> > > > > on top of ours, even indicating:
> > > > > > I'm happy to work on this as a follow-up patch series after your changes land, or we could explore a unified solution that handles both scenarios.
> > > >
> > > > That's fair.
> > > >
> > > > > These patchsets don't negate each other, so could we decouple these
> > > > > two patchsets since they address different issues?
> > > >
> > > > Well, I'm not sure if they are really different.  After all, this is
> > > > all about avoiding having to wait on an excessively long filesystem
> > > > sync during suspend.
> > >
> > > No, it's different. We don't mind a long filesystem sync if we don't
> > > have a need to abort a suspend. If it takes 25 seconds to sync the
> > > filesystem but there's no need to abort it, that's totally fine. So,
> > > this patch is just about allowing abort to happen without waiting for
> > > file system sync to finish.
> >
> > OK, thanks for clarification.
> >
> > > The other patch's requirement is to always abort if suspend takes 25
> > > seconds (or whatever the timeout is). IIRC, in his case, it's because
> > > of a bad disk or say a USB disk getting unplugged. I'm not convinced a
> > > suspend timeout is the right thing to do, but I'm not going to nack
> > > it. But to implement his requirement, he can put a patch on top of
> > > ours where he sets a timer and then aborts suspends if it fires.
> > >
> > > > I'm also not sure why it is being pursued to be honest.  Is setting
> > > > /sys/power/sync_on_suspend to 0 not regarded as a viable option?
> > >
> > > We do want to sync on every suspend. So, we don't want to turn it off
> > > completely.
> >
> > I wonder why though.
> >
> > If suspend/resume works reliably, syncing filesystems on every attempt
> > to suspend the system isn't particularly useful AFAICS.
>
> A lot of people's entire digital life is on their phone. Even if
> there's just 1% crash, when you look at that across billions of users
> it adds up to serious impact.
>
> Also, there are classes of devices where we do disable sync on suspend
> (I can't share specifics). So, this is not an arbitrary stance. For
> phones, we want to sync on suspend.
>
> Also, your question could be posed equally to laptops too. If suspend
> is reliable, then the laptop can always wake up at 3% battery, sync,
> and then shutdown. But that's a leap of faith none of us want to make.
> So, as long as we support sync on suspend, I think it makes sense to
> allow suspend aborts to happen more gracefully.

The sync on suspend may not prevent damage from happening when suspend
or resume crashes though.

On laptops it is mostly a mitigation for letting the battery run dry
while suspended due to the lack of safety there on the majority of
systems, but on phones that shouldn't be a problem AFAICS.

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Saravana Kannan]

On Mon, Oct 13, 2025 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@google.com> wrote:
> >
> > At the start of suspend and hibernate, filesystems will sync to save the
> > current state of the device. However, the long tail of the filesystem
> > sync can take upwards of 25 seconds. If during this filesystem sync
> > there is some wakeup or abort signal, it will not be processed until the
> > sync is complete; from a user's perspective, this looks like the device
> > is unresponsive to any form of input.
> >
> > This patch adds functionality to handle a sleep abort signal when in
> > the filesystem sync phase of suspend or hibernate. This topic was first
> > discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> > where the general consensus was to allow filesystem sync on a parallel
> > thread. The same logic applies to both suspend and hibernate code paths.
> >
> > There is extra care needed to account for back-to-back sleeps while
> > still maintaining functionality to immediately abort during the
> > filesystem sync stage.
> >
> > This patch handles this by serializing the filesystem sync sequence with
> > an invariant; a subsequent sleep's filesystem sync operation will only
> > start when the previous sleep's filesystem sync has finished. While
> > waiting for the previous sleep's filesystem sync to finish, the
> > subsequent sleep will still abort early if a wakeup event is triggered,
> > solving the original issue of filesystem sync blocking abort.
> >
> > [1]: https://lpc.events/event/18/contributions/1845/
> >
> > Suggested-by: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Samuel Wu <wusamuel@google.com>
> > ---
> > Changes in v4:
> > - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> > - Squashed patches 2/3 and 3/3 from v3 into this single patch
> > - Added abort during fs_sync functionality to hibernate in addition to suspend
> > - Moved variables and functions for abort from power/suspend.c to power/main.c
> > - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> > - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> > - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
> >
> > Changes in v3:
> > - Split v2 patch into 3 patches
> > - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> > - Updated documentation and comments within kernel/power/suspend.c
> > - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
> >
> > Changes in v2:
> > - Added documentation for suspend_abort_fs_sync()
> > - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> > - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
> >
> >  drivers/base/power/wakeup.c |  8 +++++
> >  include/linux/suspend.h     |  4 +++
> >  kernel/power/hibernate.c    |  5 ++-
> >  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
> >  kernel/power/suspend.c      |  7 ++--
> >  5 files changed, 91 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index d1283ff1080b..daf07ab7ac3f 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> >
> >         /* Increment the counter of events in progress. */
> >         cec = atomic_inc_return(&combined_event_count);
> > +       /*
> > +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> > +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> > +        * fs_sync only if events_check_enabled is set.
> > +        */
> > +       if (events_check_enabled)
> > +               abort_sleep_during_fs_sync();
> >
> >         trace_wakeup_source_activate(ws->name, cec);
> >  }
> > @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
> >  void pm_system_wakeup(void)
> >  {
> >         atomic_inc(&pm_abort_suspend);
> > +       abort_sleep_during_fs_sync();
> >         s2idle_wake();
> >  }
> >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 317ae31e89b3..c961bdb00bb6 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -444,6 +444,8 @@ void restore_processor_state(void);
> >  extern int register_pm_notifier(struct notifier_block *nb);
> >  extern int unregister_pm_notifier(struct notifier_block *nb);
> >  extern void ksys_sync_helper(void);
> > +extern void abort_sleep_during_fs_sync(void);
> > +extern int pm_sleep_fs_sync(void);
> >  extern void pm_report_hw_sleep_time(u64 t);
> >  extern void pm_report_max_hw_sleep(u64 t);
> >  void pm_restrict_gfp_mask(void);
> > @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
> >  static inline void pm_restore_gfp_mask(void) {}
> >
> >  static inline void ksys_sync_helper(void) {}
> > +static inline abort_sleep_during_fs_sync(void) {}
> > +static inline int pm_sleep_fs_sync(void) {}
> >
> >  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
> >
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index 2f66ab453823..651dcd768644 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -811,7 +811,10 @@ int hibernate(void)
> >         if (error)
> >                 goto Restore;
> >
> > -       ksys_sync_helper();
> > +       error = pm_sleep_fs_sync();
> > +       if (error)
> > +               goto Restore;
> > +
> >         if (filesystem_freeze_enabled)
> >                 filesystems_freeze();
> >
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 3cf2d7e72567..38b1de295cfe 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
> >  {
> >         return pm_suspend_in_progress() || hibernation_in_progress();
> >  }
> > +
> > +static bool pm_sleep_fs_sync_queued;
> > +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> > +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> > +
> > +/**
> > + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> > + *
> > + * This function aborts the fs_sync stage of suspend/hibernate so that
> > + * suspend/hibernate itself can be aborted early.
>
> This changelog needs to be more precise IMV.
>
> I'd actually call the function something like
> pm_stop_waiting_for_fs_sync() and I'd say in the changelog that the
> functions causes a suspend process to stop waiting on an fs sync in
> progress and continue so that it can be aborted before the fs sync is
> complete.
>
> > + */
> > +void abort_sleep_during_fs_sync(void)
> > +{
> > +       spin_lock(&pm_sleep_fs_sync_lock);
> > +       complete(&pm_sleep_fs_sync_complete);
> > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > +}
> > +
> > +static void sync_filesystems_fn(struct work_struct *work)
> > +{
> > +       ksys_sync_helper();
> > +
> > +       spin_lock(&pm_sleep_fs_sync_lock);
> > +       pm_sleep_fs_sync_queued = false;
> > +       complete(&pm_sleep_fs_sync_complete);
> > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > +}
> > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > +
> > +/**
> > + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> > + *
> > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > + * system sync was aborted.
> > + */
> > +int pm_sleep_fs_sync(void)
> > +{
> > +       bool need_pm_sleep_fs_sync_requeue;
> > +
> > +Start_fs_sync:
> > +       spin_lock(&pm_sleep_fs_sync_lock);
> > +       reinit_completion(&pm_sleep_fs_sync_complete);
> > +       /*
> > +        * Handle the case where a sleep immediately follows a previous sleep
> > +        * that was aborted during fs_sync. In this case, wait for the previous
> > +        * filesystem sync to finish. Then do another filesystem sync so any
> > +        * subsequent filesystem changes are synced before sleeping.
>
> Is the extra sync really necessary?

Yeah, since the fs syncs can take up to 25 seconds in some cases,
there's enough time to create new dirty data that needs to be written
to disk. So, we want to sync again to write all of that out as if the
previous attempt/abort hadn't happened. And to do that correctly, we
have to let the existing sync finish and then kick off the new one
once the "work" and "completion" are in a good state.

>
> Some files may still be updated after it is complete and before all
> tasks are frozen.

The time window can be very large here. The one you are referring to
is just a few milliseconds.

-Saravana


>
> > +        */
> > +       if (pm_sleep_fs_sync_queued) {
> > +               need_pm_sleep_fs_sync_requeue = true;
> > +       } else {
> > +               need_pm_sleep_fs_sync_requeue = false;
> > +               pm_sleep_fs_sync_queued = true;
> > +               schedule_work(&sync_filesystems);
> > +       }
> > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > +
> > +       /*
> > +        * Completion is triggered by fs_sync finishing or an abort sleep
> > +        * signal, whichever comes first
> > +        */
> > +       wait_for_completion(&pm_sleep_fs_sync_complete);
> > +       if (pm_wakeup_pending())
> > +               return -EBUSY;
> > +       if (need_pm_sleep_fs_sync_requeue)
> > +               goto Start_fs_sync;
>
> Wouldn't a do { .. } while () work here instead of the goto?
>
> > +
> > +       return 0;
> > +}
> > +
> >  #endif /* CONFIG_PM_SLEEP */
> >
> >  #ifdef CONFIG_PM_SLEEP_DEBUG
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 4bb4686c1c08..c019a4396c1f 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/compiler.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/fs.h>
> > +#include <linux/workqueue.h>
> >
> >  #include "power.h"
> >
> > @@ -588,14 +589,16 @@ static int enter_state(suspend_state_t state)
> >         if (state == PM_SUSPEND_TO_IDLE)
> >                 s2idle_begin();
> >
> > +       pm_wakeup_clear(0);
> >         if (sync_on_suspend_enabled) {
> >                 trace_suspend_resume(TPS("sync_filesystems"), 0, true);
> > -               ksys_sync_helper();
> > +               error = pm_sleep_fs_sync();
> >                 trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> > +               if (error)
> > +                       goto Unlock;
> >         }
> >
> >         pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> > -       pm_wakeup_clear(0);
> >         pm_suspend_clear_flags();
> >         error = suspend_prepare(state);
> >         if (error)
> > --

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Mon, Oct 13, 2025 at 8:34 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Oct 13, 2025 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@google.com> wrote:
> > >
> > > At the start of suspend and hibernate, filesystems will sync to save the
> > > current state of the device. However, the long tail of the filesystem
> > > sync can take upwards of 25 seconds. If during this filesystem sync
> > > there is some wakeup or abort signal, it will not be processed until the
> > > sync is complete; from a user's perspective, this looks like the device
> > > is unresponsive to any form of input.
> > >
> > > This patch adds functionality to handle a sleep abort signal when in
> > > the filesystem sync phase of suspend or hibernate. This topic was first
> > > discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> > > where the general consensus was to allow filesystem sync on a parallel
> > > thread. The same logic applies to both suspend and hibernate code paths.
> > >
> > > There is extra care needed to account for back-to-back sleeps while
> > > still maintaining functionality to immediately abort during the
> > > filesystem sync stage.
> > >
> > > This patch handles this by serializing the filesystem sync sequence with
> > > an invariant; a subsequent sleep's filesystem sync operation will only
> > > start when the previous sleep's filesystem sync has finished. While
> > > waiting for the previous sleep's filesystem sync to finish, the
> > > subsequent sleep will still abort early if a wakeup event is triggered,
> > > solving the original issue of filesystem sync blocking abort.
> > >
> > > [1]: https://lpc.events/event/18/contributions/1845/
> > >
> > > Suggested-by: Saravana Kannan <saravanak@google.com>
> > > Signed-off-by: Samuel Wu <wusamuel@google.com>
> > > ---
> > > Changes in v4:
> > > - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> > > - Squashed patches 2/3 and 3/3 from v3 into this single patch
> > > - Added abort during fs_sync functionality to hibernate in addition to suspend
> > > - Moved variables and functions for abort from power/suspend.c to power/main.c
> > > - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> > > - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> > > - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
> > >
> > > Changes in v3:
> > > - Split v2 patch into 3 patches
> > > - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> > > - Updated documentation and comments within kernel/power/suspend.c
> > > - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
> > >
> > > Changes in v2:
> > > - Added documentation for suspend_abort_fs_sync()
> > > - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> > > - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
> > >
> > >  drivers/base/power/wakeup.c |  8 +++++
> > >  include/linux/suspend.h     |  4 +++
> > >  kernel/power/hibernate.c    |  5 ++-
> > >  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
> > >  kernel/power/suspend.c      |  7 ++--
> > >  5 files changed, 91 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index d1283ff1080b..daf07ab7ac3f 100644
> > > --- a/drivers/base/power/wakeup.c
> > > +++ b/drivers/base/power/wakeup.c
> > > @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> > >
> > >         /* Increment the counter of events in progress. */
> > >         cec = atomic_inc_return(&combined_event_count);
> > > +       /*
> > > +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> > > +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> > > +        * fs_sync only if events_check_enabled is set.
> > > +        */
> > > +       if (events_check_enabled)
> > > +               abort_sleep_during_fs_sync();
> > >
> > >         trace_wakeup_source_activate(ws->name, cec);
> > >  }
> > > @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
> > >  void pm_system_wakeup(void)
> > >  {
> > >         atomic_inc(&pm_abort_suspend);
> > > +       abort_sleep_during_fs_sync();
> > >         s2idle_wake();
> > >  }
> > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > index 317ae31e89b3..c961bdb00bb6 100644
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -444,6 +444,8 @@ void restore_processor_state(void);
> > >  extern int register_pm_notifier(struct notifier_block *nb);
> > >  extern int unregister_pm_notifier(struct notifier_block *nb);
> > >  extern void ksys_sync_helper(void);
> > > +extern void abort_sleep_during_fs_sync(void);
> > > +extern int pm_sleep_fs_sync(void);
> > >  extern void pm_report_hw_sleep_time(u64 t);
> > >  extern void pm_report_max_hw_sleep(u64 t);
> > >  void pm_restrict_gfp_mask(void);
> > > @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
> > >  static inline void pm_restore_gfp_mask(void) {}
> > >
> > >  static inline void ksys_sync_helper(void) {}
> > > +static inline abort_sleep_during_fs_sync(void) {}
> > > +static inline int pm_sleep_fs_sync(void) {}
> > >
> > >  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
> > >
> > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > index 2f66ab453823..651dcd768644 100644
> > > --- a/kernel/power/hibernate.c
> > > +++ b/kernel/power/hibernate.c
> > > @@ -811,7 +811,10 @@ int hibernate(void)
> > >         if (error)
> > >                 goto Restore;
> > >
> > > -       ksys_sync_helper();
> > > +       error = pm_sleep_fs_sync();
> > > +       if (error)
> > > +               goto Restore;
> > > +
> > >         if (filesystem_freeze_enabled)
> > >                 filesystems_freeze();
> > >
> > > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > > index 3cf2d7e72567..38b1de295cfe 100644
> > > --- a/kernel/power/main.c
> > > +++ b/kernel/power/main.c
> > > @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
> > >  {
> > >         return pm_suspend_in_progress() || hibernation_in_progress();
> > >  }
> > > +
> > > +static bool pm_sleep_fs_sync_queued;
> > > +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> > > +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> > > +
> > > +/**
> > > + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> > > + *
> > > + * This function aborts the fs_sync stage of suspend/hibernate so that
> > > + * suspend/hibernate itself can be aborted early.
> >
> > This changelog needs to be more precise IMV.
> >
> > I'd actually call the function something like
> > pm_stop_waiting_for_fs_sync() and I'd say in the changelog that the
> > functions causes a suspend process to stop waiting on an fs sync in
> > progress and continue so that it can be aborted before the fs sync is
> > complete.
> >
> > > + */
> > > +void abort_sleep_during_fs_sync(void)
> > > +{
> > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > +       complete(&pm_sleep_fs_sync_complete);
> > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > +}
> > > +
> > > +static void sync_filesystems_fn(struct work_struct *work)
> > > +{
> > > +       ksys_sync_helper();
> > > +
> > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > +       pm_sleep_fs_sync_queued = false;
> > > +       complete(&pm_sleep_fs_sync_complete);
> > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > +}
> > > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > > +
> > > +/**
> > > + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> > > + *
> > > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > > + * system sync was aborted.
> > > + */
> > > +int pm_sleep_fs_sync(void)
> > > +{
> > > +       bool need_pm_sleep_fs_sync_requeue;
> > > +
> > > +Start_fs_sync:
> > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > +       reinit_completion(&pm_sleep_fs_sync_complete);
> > > +       /*
> > > +        * Handle the case where a sleep immediately follows a previous sleep
> > > +        * that was aborted during fs_sync. In this case, wait for the previous
> > > +        * filesystem sync to finish. Then do another filesystem sync so any
> > > +        * subsequent filesystem changes are synced before sleeping.
> >
> > Is the extra sync really necessary?
>
> Yeah, since the fs syncs can take up to 25 seconds in some cases,
> there's enough time to create new dirty data that needs to be written
> to disk. So, we want to sync again to write all of that out as if the
> previous attempt/abort hadn't happened. And to do that correctly, we
> have to let the existing sync finish and then kick off the new one
> once the "work" and "completion" are in a good state.

And if the new suspend is aborted while the new fs sync is in
progress, then you'll repeat the exercise and so on, possibly ad
infinitum?

> >
> > Some files may still be updated after it is complete and before all
> > tasks are frozen.
>
> The time window can be very large here. The one you are referring to
> is just a few milliseconds.

It can take up to 20 sec.

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Mon, Oct 13, 2025 at 8:39 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Oct 13, 2025 at 8:34 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Mon, Oct 13, 2025 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@google.com> wrote:
> > > >
> > > > At the start of suspend and hibernate, filesystems will sync to save the
> > > > current state of the device. However, the long tail of the filesystem
> > > > sync can take upwards of 25 seconds. If during this filesystem sync
> > > > there is some wakeup or abort signal, it will not be processed until the
> > > > sync is complete; from a user's perspective, this looks like the device
> > > > is unresponsive to any form of input.
> > > >
> > > > This patch adds functionality to handle a sleep abort signal when in
> > > > the filesystem sync phase of suspend or hibernate. This topic was first
> > > > discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> > > > where the general consensus was to allow filesystem sync on a parallel
> > > > thread. The same logic applies to both suspend and hibernate code paths.
> > > >
> > > > There is extra care needed to account for back-to-back sleeps while
> > > > still maintaining functionality to immediately abort during the
> > > > filesystem sync stage.
> > > >
> > > > This patch handles this by serializing the filesystem sync sequence with
> > > > an invariant; a subsequent sleep's filesystem sync operation will only
> > > > start when the previous sleep's filesystem sync has finished. While
> > > > waiting for the previous sleep's filesystem sync to finish, the
> > > > subsequent sleep will still abort early if a wakeup event is triggered,
> > > > solving the original issue of filesystem sync blocking abort.
> > > >
> > > > [1]: https://lpc.events/event/18/contributions/1845/
> > > >
> > > > Suggested-by: Saravana Kannan <saravanak@google.com>
> > > > Signed-off-by: Samuel Wu <wusamuel@google.com>
> > > > ---
> > > > Changes in v4:
> > > > - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> > > > - Squashed patches 2/3 and 3/3 from v3 into this single patch
> > > > - Added abort during fs_sync functionality to hibernate in addition to suspend
> > > > - Moved variables and functions for abort from power/suspend.c to power/main.c
> > > > - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> > > > - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> > > > - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
> > > >
> > > > Changes in v3:
> > > > - Split v2 patch into 3 patches
> > > > - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> > > > - Updated documentation and comments within kernel/power/suspend.c
> > > > - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
> > > >
> > > > Changes in v2:
> > > > - Added documentation for suspend_abort_fs_sync()
> > > > - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> > > > - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
> > > >
> > > >  drivers/base/power/wakeup.c |  8 +++++
> > > >  include/linux/suspend.h     |  4 +++
> > > >  kernel/power/hibernate.c    |  5 ++-
> > > >  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
> > > >  kernel/power/suspend.c      |  7 ++--
> > > >  5 files changed, 91 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > > index d1283ff1080b..daf07ab7ac3f 100644
> > > > --- a/drivers/base/power/wakeup.c
> > > > +++ b/drivers/base/power/wakeup.c
> > > > @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> > > >
> > > >         /* Increment the counter of events in progress. */
> > > >         cec = atomic_inc_return(&combined_event_count);
> > > > +       /*
> > > > +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> > > > +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> > > > +        * fs_sync only if events_check_enabled is set.
> > > > +        */
> > > > +       if (events_check_enabled)
> > > > +               abort_sleep_during_fs_sync();
> > > >
> > > >         trace_wakeup_source_activate(ws->name, cec);
> > > >  }
> > > > @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
> > > >  void pm_system_wakeup(void)
> > > >  {
> > > >         atomic_inc(&pm_abort_suspend);
> > > > +       abort_sleep_during_fs_sync();
> > > >         s2idle_wake();
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > > index 317ae31e89b3..c961bdb00bb6 100644
> > > > --- a/include/linux/suspend.h
> > > > +++ b/include/linux/suspend.h
> > > > @@ -444,6 +444,8 @@ void restore_processor_state(void);
> > > >  extern int register_pm_notifier(struct notifier_block *nb);
> > > >  extern int unregister_pm_notifier(struct notifier_block *nb);
> > > >  extern void ksys_sync_helper(void);
> > > > +extern void abort_sleep_during_fs_sync(void);
> > > > +extern int pm_sleep_fs_sync(void);
> > > >  extern void pm_report_hw_sleep_time(u64 t);
> > > >  extern void pm_report_max_hw_sleep(u64 t);
> > > >  void pm_restrict_gfp_mask(void);
> > > > @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
> > > >  static inline void pm_restore_gfp_mask(void) {}
> > > >
> > > >  static inline void ksys_sync_helper(void) {}
> > > > +static inline abort_sleep_during_fs_sync(void) {}
> > > > +static inline int pm_sleep_fs_sync(void) {}
> > > >
> > > >  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
> > > >
> > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > index 2f66ab453823..651dcd768644 100644
> > > > --- a/kernel/power/hibernate.c
> > > > +++ b/kernel/power/hibernate.c
> > > > @@ -811,7 +811,10 @@ int hibernate(void)
> > > >         if (error)
> > > >                 goto Restore;
> > > >
> > > > -       ksys_sync_helper();
> > > > +       error = pm_sleep_fs_sync();
> > > > +       if (error)
> > > > +               goto Restore;
> > > > +
> > > >         if (filesystem_freeze_enabled)
> > > >                 filesystems_freeze();
> > > >
> > > > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > > > index 3cf2d7e72567..38b1de295cfe 100644
> > > > --- a/kernel/power/main.c
> > > > +++ b/kernel/power/main.c
> > > > @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
> > > >  {
> > > >         return pm_suspend_in_progress() || hibernation_in_progress();
> > > >  }
> > > > +
> > > > +static bool pm_sleep_fs_sync_queued;
> > > > +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> > > > +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> > > > +
> > > > +/**
> > > > + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> > > > + *
> > > > + * This function aborts the fs_sync stage of suspend/hibernate so that
> > > > + * suspend/hibernate itself can be aborted early.
> > >
> > > This changelog needs to be more precise IMV.
> > >
> > > I'd actually call the function something like
> > > pm_stop_waiting_for_fs_sync() and I'd say in the changelog that the
> > > functions causes a suspend process to stop waiting on an fs sync in
> > > progress and continue so that it can be aborted before the fs sync is
> > > complete.
> > >
> > > > + */
> > > > +void abort_sleep_during_fs_sync(void)
> > > > +{
> > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > +       complete(&pm_sleep_fs_sync_complete);
> > > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > > +}
> > > > +
> > > > +static void sync_filesystems_fn(struct work_struct *work)
> > > > +{
> > > > +       ksys_sync_helper();
> > > > +
> > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > +       pm_sleep_fs_sync_queued = false;
> > > > +       complete(&pm_sleep_fs_sync_complete);
> > > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > > +}
> > > > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > > > +
> > > > +/**
> > > > + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> > > > + *
> > > > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > > > + * system sync was aborted.
> > > > + */
> > > > +int pm_sleep_fs_sync(void)
> > > > +{
> > > > +       bool need_pm_sleep_fs_sync_requeue;
> > > > +
> > > > +Start_fs_sync:
> > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > +       reinit_completion(&pm_sleep_fs_sync_complete);
> > > > +       /*
> > > > +        * Handle the case where a sleep immediately follows a previous sleep
> > > > +        * that was aborted during fs_sync. In this case, wait for the previous
> > > > +        * filesystem sync to finish. Then do another filesystem sync so any
> > > > +        * subsequent filesystem changes are synced before sleeping.
> > >
> > > Is the extra sync really necessary?
> >
> > Yeah, since the fs syncs can take up to 25 seconds in some cases,
> > there's enough time to create new dirty data that needs to be written
> > to disk. So, we want to sync again to write all of that out as if the
> > previous attempt/abort hadn't happened. And to do that correctly, we
> > have to let the existing sync finish and then kick off the new one
> > once the "work" and "completion" are in a good state.
>
> And if the new suspend is aborted while the new fs sync is in
> progress, then you'll repeat the exercise and so on, possibly ad
> infinitum?

Also, by the above argument, you should do an extra fs sync after
every fs sync taking "too much" time, not just when the previous
suspend has been aborted during an fs sync.

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[Saravana Kannan]

On Mon, Oct 13, 2025 at 11:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Oct 13, 2025 at 8:39 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Oct 13, 2025 at 8:34 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Mon, Oct 13, 2025 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@google.com> wrote:
> > > > >
> > > > > At the start of suspend and hibernate, filesystems will sync to save the
> > > > > current state of the device. However, the long tail of the filesystem
> > > > > sync can take upwards of 25 seconds. If during this filesystem sync
> > > > > there is some wakeup or abort signal, it will not be processed until the
> > > > > sync is complete; from a user's perspective, this looks like the device
> > > > > is unresponsive to any form of input.
> > > > >
> > > > > This patch adds functionality to handle a sleep abort signal when in
> > > > > the filesystem sync phase of suspend or hibernate. This topic was first
> > > > > discussed specifically for suspend by Saravana Kannan at LPC 2024 [1],
> > > > > where the general consensus was to allow filesystem sync on a parallel
> > > > > thread. The same logic applies to both suspend and hibernate code paths.
> > > > >
> > > > > There is extra care needed to account for back-to-back sleeps while
> > > > > still maintaining functionality to immediately abort during the
> > > > > filesystem sync stage.
> > > > >
> > > > > This patch handles this by serializing the filesystem sync sequence with
> > > > > an invariant; a subsequent sleep's filesystem sync operation will only
> > > > > start when the previous sleep's filesystem sync has finished. While
> > > > > waiting for the previous sleep's filesystem sync to finish, the
> > > > > subsequent sleep will still abort early if a wakeup event is triggered,
> > > > > solving the original issue of filesystem sync blocking abort.
> > > > >
> > > > > [1]: https://lpc.events/event/18/contributions/1845/
> > > > >
> > > > > Suggested-by: Saravana Kannan <saravanak@google.com>
> > > > > Signed-off-by: Samuel Wu <wusamuel@google.com>
> > > > > ---
> > > > > Changes in v4:
> > > > > - Removed patch 1/3 of v3 as it is already picked up on linux-pm
> > > > > - Squashed patches 2/3 and 3/3 from v3 into this single patch
> > > > > - Added abort during fs_sync functionality to hibernate in addition to suspend
> > > > > - Moved variables and functions for abort from power/suspend.c to power/main.c
> > > > > - Renamed suspend_fs_sync_with_abort() to pm_sleep_fs_sync()
> > > > > - Renamed suspend_abort_fs_sync() to abort_sleep_during_fs_sync()
> > > > > - v3 link: https://lore.kernel.org/all/20250821004237.2712312-1-wusamuel@google.com/
> > > > >
> > > > > Changes in v3:
> > > > > - Split v2 patch into 3 patches
> > > > > - Moved pm_wakeup_clear() outside of if(sync_on_suspend_enabled) condition
> > > > > - Updated documentation and comments within kernel/power/suspend.c
> > > > > - v2 link: https://lore.kernel.org/all/20250812232126.1814253-1-wusamuel@google.com/
> > > > >
> > > > > Changes in v2:
> > > > > - Added documentation for suspend_abort_fs_sync()
> > > > > - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> > > > > - v1 link: https://lore.kernel.org/all/20250815004635.3684650-1-wusamuel@google.com
> > > > >
> > > > >  drivers/base/power/wakeup.c |  8 +++++
> > > > >  include/linux/suspend.h     |  4 +++
> > > > >  kernel/power/hibernate.c    |  5 ++-
> > > > >  kernel/power/main.c         | 70 +++++++++++++++++++++++++++++++++++++
> > > > >  kernel/power/suspend.c      |  7 ++--
> > > > >  5 files changed, 91 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > > > index d1283ff1080b..daf07ab7ac3f 100644
> > > > > --- a/drivers/base/power/wakeup.c
> > > > > +++ b/drivers/base/power/wakeup.c
> > > > > @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> > > > >
> > > > >         /* Increment the counter of events in progress. */
> > > > >         cec = atomic_inc_return(&combined_event_count);
> > > > > +       /*
> > > > > +        * wakeup_source_activate() aborts sleep only if events_check_enabled
> > > > > +        * is set (see pm_wakeup_pending()). Similarly, abort sleep during
> > > > > +        * fs_sync only if events_check_enabled is set.
> > > > > +        */
> > > > > +       if (events_check_enabled)
> > > > > +               abort_sleep_during_fs_sync();
> > > > >
> > > > >         trace_wakeup_source_activate(ws->name, cec);
> > > > >  }
> > > > > @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
> > > > >  void pm_system_wakeup(void)
> > > > >  {
> > > > >         atomic_inc(&pm_abort_suspend);
> > > > > +       abort_sleep_during_fs_sync();
> > > > >         s2idle_wake();
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > > > index 317ae31e89b3..c961bdb00bb6 100644
> > > > > --- a/include/linux/suspend.h
> > > > > +++ b/include/linux/suspend.h
> > > > > @@ -444,6 +444,8 @@ void restore_processor_state(void);
> > > > >  extern int register_pm_notifier(struct notifier_block *nb);
> > > > >  extern int unregister_pm_notifier(struct notifier_block *nb);
> > > > >  extern void ksys_sync_helper(void);
> > > > > +extern void abort_sleep_during_fs_sync(void);
> > > > > +extern int pm_sleep_fs_sync(void);
> > > > >  extern void pm_report_hw_sleep_time(u64 t);
> > > > >  extern void pm_report_max_hw_sleep(u64 t);
> > > > >  void pm_restrict_gfp_mask(void);
> > > > > @@ -499,6 +501,8 @@ static inline void pm_restrict_gfp_mask(void) {}
> > > > >  static inline void pm_restore_gfp_mask(void) {}
> > > > >
> > > > >  static inline void ksys_sync_helper(void) {}
> > > > > +static inline abort_sleep_during_fs_sync(void) {}
> > > > > +static inline int pm_sleep_fs_sync(void) {}
> > > > >
> > > > >  #define pm_notifier(fn, pri)   do { (void)(fn); } while (0)
> > > > >
> > > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > > index 2f66ab453823..651dcd768644 100644
> > > > > --- a/kernel/power/hibernate.c
> > > > > +++ b/kernel/power/hibernate.c
> > > > > @@ -811,7 +811,10 @@ int hibernate(void)
> > > > >         if (error)
> > > > >                 goto Restore;
> > > > >
> > > > > -       ksys_sync_helper();
> > > > > +       error = pm_sleep_fs_sync();
> > > > > +       if (error)
> > > > > +               goto Restore;
> > > > > +
> > > > >         if (filesystem_freeze_enabled)
> > > > >                 filesystems_freeze();
> > > > >
> > > > > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > > > > index 3cf2d7e72567..38b1de295cfe 100644
> > > > > --- a/kernel/power/main.c
> > > > > +++ b/kernel/power/main.c
> > > > > @@ -570,6 +570,76 @@ bool pm_sleep_transition_in_progress(void)
> > > > >  {
> > > > >         return pm_suspend_in_progress() || hibernation_in_progress();
> > > > >  }
> > > > > +
> > > > > +static bool pm_sleep_fs_sync_queued;
> > > > > +static DEFINE_SPINLOCK(pm_sleep_fs_sync_lock);
> > > > > +static DECLARE_COMPLETION(pm_sleep_fs_sync_complete);
> > > > > +
> > > > > +/**
> > > > > + * abort_sleep_during_fs_sync - Abort fs_sync to abort sleep early
> > > > > + *
> > > > > + * This function aborts the fs_sync stage of suspend/hibernate so that
> > > > > + * suspend/hibernate itself can be aborted early.
> > > >
> > > > This changelog needs to be more precise IMV.
> > > >
> > > > I'd actually call the function something like
> > > > pm_stop_waiting_for_fs_sync() and I'd say in the changelog that the
> > > > functions causes a suspend process to stop waiting on an fs sync in
> > > > progress and continue so that it can be aborted before the fs sync is
> > > > complete.
> > > >
> > > > > + */
> > > > > +void abort_sleep_during_fs_sync(void)
> > > > > +{
> > > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > > +       complete(&pm_sleep_fs_sync_complete);
> > > > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > > > +}
> > > > > +
> > > > > +static void sync_filesystems_fn(struct work_struct *work)
> > > > > +{
> > > > > +       ksys_sync_helper();
> > > > > +
> > > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > > +       pm_sleep_fs_sync_queued = false;
> > > > > +       complete(&pm_sleep_fs_sync_complete);
> > > > > +       spin_unlock(&pm_sleep_fs_sync_lock);
> > > > > +}
> > > > > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > > > > +
> > > > > +/**
> > > > > + * pm_sleep_fs_sync - Trigger fs_sync with ability to abort
> > > > > + *
> > > > > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > > > > + * system sync was aborted.
> > > > > + */
> > > > > +int pm_sleep_fs_sync(void)
> > > > > +{
> > > > > +       bool need_pm_sleep_fs_sync_requeue;
> > > > > +
> > > > > +Start_fs_sync:
> > > > > +       spin_lock(&pm_sleep_fs_sync_lock);
> > > > > +       reinit_completion(&pm_sleep_fs_sync_complete);
> > > > > +       /*
> > > > > +        * Handle the case where a sleep immediately follows a previous sleep
> > > > > +        * that was aborted during fs_sync. In this case, wait for the previous
> > > > > +        * filesystem sync to finish. Then do another filesystem sync so any
> > > > > +        * subsequent filesystem changes are synced before sleeping.
> > > >
> > > > Is the extra sync really necessary?
> > >
> > > Yeah, since the fs syncs can take up to 25 seconds in some cases,
> > > there's enough time to create new dirty data that needs to be written
> > > to disk. So, we want to sync again to write all of that out as if the
> > > previous attempt/abort hadn't happened. And to do that correctly, we
> > > have to let the existing sync finish and then kick off the new one
> > > once the "work" and "completion" are in a good state.
> >
> > And if the new suspend is aborted while the new fs sync is in
> > progress, then you'll repeat the exercise and so on, possibly ad
> > infinitum?
>
> Also, by the above argument, you should do an extra fs sync after
> every fs sync taking "too much" time, not just when the previous
> suspend has been aborted during an fs sync.

The goal is to NOT regress what gets synced when we add the ability to
abort suspend while fs sync is in progress. I'm open to other ideas if
you can ensure we aren't regressing it.

But even if we ignore that, we clearly have to make sure the previous
sync/work isn't "in use" before we try to requeue the work. So, this
needs to be done anyway?

We also can't cancel/flush sync the work because that'll take us back
to square one where the abort can get hung up behind a fs sync
completing.

-Saravana

Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

[tuhaowen]

Hi Rafael,

Thank you for your attention to this matter. I'd like to clarify the 
difference between our approach and the Google team's solution, as they 
address fundamentally different use cases and environments.

On Mon, Oct 13, 2025 at 8:02 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > No, it's different. We don't mind a long filesystem sync if we don't
> > have a need to abort a suspend. If it takes 25 seconds to sync the
> > filesystem but there's no need to abort it, that's totally fine. So,
> > this patch is just about allowing abort to happen without waiting for
> > file system sync to finish.

Saravana is correct that our problems are different. Let me explain the 
key distinction:

**Google's approach (Mobile/Android focus)**:
- Problem: Unnecessary wake-ups during sync operations waste battery
- Solution: Abort sync only when wakeup events occur (user interaction)
- Philosophy: Wait indefinitely for sync if no user action required
- Use case: Mobile devices where users expect to press power button to wake

**Our approach (Desktop/PC focus)**:
- Problem: Indefinite sync hangs leave users with unresponsive black screen
- Solution: Proactive timeout to prevent system appearing frozen
- Philosophy: Provide user feedback and system recovery within reasonable time
- Use case: Desktop/laptop where users expect immediate system response

> > The other patch's requirement is to always abort if suspend takes 25
> > seconds (or whatever the timeout is). IIRC, in his case, it's because
> > of a bad disk or say a USB disk getting unplugged. I'm not convinced a
> > suspend timeout is the right thing to do, but I'm not going to nack
> > it. But to implement his requirement, he can put a patch on top of
> > ours where he sets a timer and then aborts suspends if it fires.

The key difference is **when** we need to abort:
- Google: Abort when user wants to wake up (reactive)
- UnionTech: Abort when sync becomes pathologically slow (proactive)

For desktop users, a 25-second black screen with no feedback creates the 
impression of a system freeze, especially when caused by removed USB 
devices or failing storage. Users cannot distinguish between "system is 
syncing" and "system has crashed" without feedback.

**Question about integration**:

Since both approaches serve legitimate but different needs, could we 
implement a unified solution that supports both mechanisms? For example:

1. **Combined approach**: Implement both wakeup-based abort (Google's patch) 
   and timeout-based abort (our patch) in the same framework

2. **Configuration via sysfs**: Add a node to control the behavior:
   - `/sys/power/sync_abort_mode`:
     - "wakeup-only": Use Google's approach (abort only on wakeup events)
     - "timeout": Use our approach (abort on timeout)
     - "both": Use both mechanisms (abort on either condition)

3. **Default behavior**: Could default to "wakeup-only" for mobile/embedded 
   systems and "timeout" for desktop systems, or let distributions choose

This would allow different systems to choose appropriate behavior based 
on their needs.

Would this unified approach be acceptable? We're happy to work on 
implementation details with the Google team to ensure both use cases 
are properly addressed.

**Additional concern about integration timing**:

I noticed that Samuel Wu previously mentioned in his response to you 
(Sep 30, 2025) that our approaches could be "decoupled" and that I could 
"build changes on top of theirs." However, after reviewing their v4 patch 
implementation, I'm concerned that if their approach lands first, it may 
make our timeout-based solution significantly more difficult to integrate.

Their current implementation:
- Uses workqueue + completion for sync operations
- Introduces pm_sleep_fs_sync() as the main interface
- Adds complex state management for back-to-back sleep attempts

This architecture makes it challenging to integrate our timeout approach 
and add mode switching functionality. If their patch lands first, adding 
our timeout mechanism would require:
- Modifying their workqueue-based sync mechanism to support timeout
- Adding logic to coordinate between workqueue completion and timeout
- Implementing mode switching between wakeup-abort and timeout-abort
- Ensuring proper interaction between the two abort mechanisms

The main challenge is: how do we add timeout functionality to their 
workqueue + completion design? And how do we implement clean switching 
between "abort on wakeup events" mode versus "abort on timeout" mode? 
Their current design focuses solely on wakeup-based abort, so retrofitting 
timeout support and mode selection would require significant changes to 
their implementation.

Would it be possible to consider both approaches simultaneously to ensure 
a clean integration path? This might result in a better unified solution 
than trying to retrofit timeout functionality into their workqueue-based 
implementation.

Best regards,
Haowen Tu