[PATCH v3 0/3] PM: Support aborting suspend during filesystem sync

[PATCH v3 0/3] PM: Support aborting suspend during filesystem sync

[Samuel Wu]

This series adds functionality to handle a wakeup event in the filesystem
sync phase of suspend. This topic was first discussed by Saravana Kannan at
LPC 2024 [1], where the general consensus was to allow filesystem sync on a
parallel thread.

1/3: Moves pm_wakeup_clear() to be with similar functions that do the
     prerequisite bookkeeping prior to suspend_prepare(), making the intent
     of the function clearer. Additionally, this sets up patch2 in the
     series, which requires pm_wakeup_clear() to be called before abort
     functionality during suspend can be enabled.
2/3: Add support for abort during filesystem sync phase of suspend. This
     requires detecting a wakeup event or a filesystem sync finishing,
     whichever comes first.
3/3: Adding onto abort during filesystem sync, handle cases of back-to-back
     suspends and their respective filesystem syncs. Extra care is needed
     to ensure filesystem syncs happen serially while still honoring
     wakeup events that lead to early aborts.

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

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

Samuel Wu (3):
  PM: Make pm_wakeup_clear() call more clear
  PM: Support aborting suspend during filesystem sync
  PM: Support abort during fs_sync of back-to-back suspends

 drivers/base/power/wakeup.c |  8 ++++
 include/linux/suspend.h     |  3 ++
 kernel/power/process.c      |  1 -
 kernel/power/suspend.c      | 75 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 85 insertions(+), 2 deletions(-)

-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH v3 1/3] PM: Make pm_wakeup_clear() call more clear

[Samuel Wu]

Move pm_wakeup_clear() to the same location as other functions that do
bookkeeping prior to suspend_prepare(). Since calling pm_wakeup_clear()
is a prerequisite to setting up for suspend and enabling functionalities
of suspend (like aborting during suspend), moving pm_wakeup_clear()
higher up the call stack makes its intent more clear and obvious that it
is called prior to suspend_prepare().

With this patch, there is a slightly larger window when abort events
can be registered, but otherwise suspend functionality is the same.

Suggested-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Samuel Wu <wusamuel@google.com>
---
 kernel/power/process.c | 1 -
 kernel/power/suspend.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index dc0dfc349f22..8ff68ebaa1e0 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -132,7 +132,6 @@ int freeze_processes(void)
 	if (!pm_freezing)
 		static_branch_inc(&freezer_active);
 
-	pm_wakeup_clear(0);
 	pm_freezing = true;
 	error = try_to_freeze_tasks(true);
 	if (!error)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index b4ca17c2fecf..4bb4686c1c08 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -595,6 +595,7 @@ static int enter_state(suspend_state_t state)
 	}
 
 	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.261.g7ce5a0a67e-goog

Re: [PATCH v3 1/3] PM: Make pm_wakeup_clear() call more clear

[Rafael J. Wysocki]

On Thu, Aug 21, 2025 at 2:42 AM Samuel Wu <wusamuel@google.com> wrote:
>
> Move pm_wakeup_clear() to the same location as other functions that do
> bookkeeping prior to suspend_prepare(). Since calling pm_wakeup_clear()
> is a prerequisite to setting up for suspend and enabling functionalities
> of suspend (like aborting during suspend), moving pm_wakeup_clear()
> higher up the call stack makes its intent more clear and obvious that it
> is called prior to suspend_prepare().
>
> With this patch, there is a slightly larger window when abort events
> can be registered, but otherwise suspend functionality is the same.
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Samuel Wu <wusamuel@google.com>
> ---
>  kernel/power/process.c | 1 -
>  kernel/power/suspend.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index dc0dfc349f22..8ff68ebaa1e0 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -132,7 +132,6 @@ int freeze_processes(void)
>         if (!pm_freezing)
>                 static_branch_inc(&freezer_active);
>
> -       pm_wakeup_clear(0);
>         pm_freezing = true;
>         error = try_to_freeze_tasks(true);
>         if (!error)
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b4ca17c2fecf..4bb4686c1c08 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -595,6 +595,7 @@ static int enter_state(suspend_state_t state)
>         }
>
>         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)
> --

Applied as 6.18 material with Saravana's R-by and minor edits in the
subject and changelog.

Thanks!

[PATCH v3 2/3] PM: Support aborting suspend during filesystem sync

[Samuel Wu]

At the start of suspend, 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 suspend abort signal when in
the filesystem sync phase of suspend. This topic was first discussed by
Saravana Kannan at LPC 2024 [1], where the general consensus was to
allow filesystem sync on a parallel thread.

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

Suggested-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Samuel Wu <wusamuel@google.com>
---
 drivers/base/power/wakeup.c |  8 +++++++
 include/linux/suspend.h     |  2 ++
 kernel/power/suspend.c      | 48 +++++++++++++++++++++++++++++++++++--
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index d1283ff1080b..af4cf3e6ba44 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 suspend only if events_check_enabled
+	 * is set (see pm_wakeup_pending()). Similarly, abort suspend during
+	 * fs_sync only if events_check_enabled is set.
+	 */
+	if (events_check_enabled)
+		suspend_abort_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);
+	suspend_abort_fs_sync();
 	s2idle_wake();
 }
 EXPORT_SYMBOL_GPL(pm_system_wakeup);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 317ae31e89b3..68d2e8a7eeb1 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -276,6 +276,7 @@ extern void arch_suspend_enable_irqs(void);
 
 extern int pm_suspend(suspend_state_t state);
 extern bool sync_on_suspend_enabled;
+extern void suspend_abort_fs_sync(void);
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
@@ -296,6 +297,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
 static inline void s2idle_wake(void) {}
+static inline void suspend_abort_fs_sync(void) {}
 #endif /* !CONFIG_SUSPEND */
 
 static inline bool pm_suspend_in_progress(void)
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 4bb4686c1c08..edacd2a4143b 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"
 
@@ -74,6 +75,19 @@ bool pm_suspend_default_s2idle(void)
 }
 EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
 
+static DECLARE_COMPLETION(suspend_fs_sync_complete);
+
+/**
+ * suspend_abort_fs_sync - Abort fs_sync to abort suspend early
+ *
+ * This function aborts the fs_sync stage of suspend so that suspend itself can
+ * be aborted early.
+ */
+void suspend_abort_fs_sync(void)
+{
+	complete(&suspend_fs_sync_complete);
+}
+
 void s2idle_set_ops(const struct platform_s2idle_ops *ops)
 {
 	unsigned int sleep_flags;
@@ -403,6 +417,34 @@ void __weak arch_suspend_enable_irqs(void)
 	local_irq_enable();
 }
 
+static void sync_filesystems_fn(struct work_struct *work)
+{
+	ksys_sync_helper();
+	complete(&suspend_fs_sync_complete);
+}
+static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
+
+/**
+ * suspend_fs_sync_with_abort - Trigger fs_sync with ability to abort
+ *
+ * Return 0 on successful file system sync, otherwise returns -EBUSY if file
+ * system sync was aborted.
+ */
+static int suspend_fs_sync_with_abort(void)
+{
+	reinit_completion(&suspend_fs_sync_complete);
+	schedule_work(&sync_filesystems);
+	/*
+	 * Completion is triggered by fs_sync finishing or a suspend abort
+	 * signal, whichever comes first
+	 */
+	wait_for_completion(&suspend_fs_sync_complete);
+	if (pm_wakeup_pending())
+		return -EBUSY;
+
+	return 0;
+}
+
 /**
  * suspend_enter - Make the system enter the given sleep state.
  * @state: System sleep state to enter.
@@ -588,14 +630,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 = suspend_fs_sync_with_abort();
 		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.261.g7ce5a0a67e-goog

Re: [PATCH v3 2/3] PM: Support aborting suspend during filesystem sync

[Rafael J. Wysocki]

On Thu, Aug 21, 2025 at 2:43 AM Samuel Wu <wusamuel@google.com> wrote:
>
> At the start of suspend, 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 suspend abort signal when in
> the filesystem sync phase of suspend. This topic was first discussed by
> Saravana Kannan at LPC 2024 [1], where the general consensus was to
> allow filesystem sync on a parallel thread.
>
> [1]: https://lpc.events/event/18/contributions/1845/
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Samuel Wu <wusamuel@google.com>

The idea is fine with me, but I have comments on the implementation.

First off, I'm not sure how the split of patch [3/3] from this one
helps because that patch adds mandatory functionality.  Patch [2/3]
cannot be applied without patch [3/3], so as far as I'm concerned, it
is not applicable at all.

> ---
>  drivers/base/power/wakeup.c |  8 +++++++
>  include/linux/suspend.h     |  2 ++
>  kernel/power/suspend.c      | 48 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..af4cf3e6ba44 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 suspend only if events_check_enabled
> +        * is set (see pm_wakeup_pending()). Similarly, abort suspend during
> +        * fs_sync only if events_check_enabled is set.
> +        */
> +       if (events_check_enabled)
> +               suspend_abort_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);
> +       suspend_abort_fs_sync();
>         s2idle_wake();
>  }
>  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 317ae31e89b3..68d2e8a7eeb1 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -276,6 +276,7 @@ extern void arch_suspend_enable_irqs(void);
>
>  extern int pm_suspend(suspend_state_t state);
>  extern bool sync_on_suspend_enabled;
> +extern void suspend_abort_fs_sync(void);
>  #else /* !CONFIG_SUSPEND */
>  #define suspend_valid_only_mem NULL
>
> @@ -296,6 +297,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
>  static inline void __init pm_states_init(void) {}
>  static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
>  static inline void s2idle_wake(void) {}
> +static inline void suspend_abort_fs_sync(void) {}
>  #endif /* !CONFIG_SUSPEND */
>
>  static inline bool pm_suspend_in_progress(void)
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4bb4686c1c08..edacd2a4143b 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"
>
> @@ -74,6 +75,19 @@ bool pm_suspend_default_s2idle(void)
>  }
>  EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
>
> +static DECLARE_COMPLETION(suspend_fs_sync_complete);
> +
> +/**
> + * suspend_abort_fs_sync - Abort fs_sync to abort suspend early

This name is kind of misleading because the function doesn't abort
fs_sync.  It aborts suspend while fs_sync is in progress.

The fs_sync is continuing and needs to be waited for to complete, at
least in subsequent suspend/hibernate cycles.

Does system shutdown need to wait for it too?

> + *
> + * This function aborts the fs_sync stage of suspend so that suspend itself can
> + * be aborted early.
> + */
> +void suspend_abort_fs_sync(void)
> +{
> +       complete(&suspend_fs_sync_complete);
> +}
> +
>  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
>  {
>         unsigned int sleep_flags;
> @@ -403,6 +417,34 @@ void __weak arch_suspend_enable_irqs(void)
>         local_irq_enable();
>  }
>
> +static void sync_filesystems_fn(struct work_struct *work)
> +{
> +       ksys_sync_helper();
> +       complete(&suspend_fs_sync_complete);
> +}
> +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> +
> +/**
> + * suspend_fs_sync_with_abort - Trigger fs_sync with ability to abort
> + *
> + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> + * system sync was aborted.
> + */
> +static int suspend_fs_sync_with_abort(void)

The "with_abort" part is not needed in this name and I'd prefer
pm_sleep_ to be used instead of suspend_

> +{
> +       reinit_completion(&suspend_fs_sync_complete);
> +       schedule_work(&sync_filesystems);

If you used a dedicated workqueue for this, waiting for the previously
scheduled filesystems sync would be as simple as calling
flush_workqueue(pm_fs_sync_wq);

> +       /*
> +        * Completion is triggered by fs_sync finishing or a suspend abort
> +        * signal, whichever comes first
> +        */
> +       wait_for_completion(&suspend_fs_sync_complete);
> +       if (pm_wakeup_pending())
> +               return -EBUSY;
> +
> +       return 0;
> +}
> +
>  /**
>   * suspend_enter - Make the system enter the given sleep state.
>   * @state: System sleep state to enter.
> @@ -588,14 +630,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 = suspend_fs_sync_with_abort();

The same thing would also be useful for hibernation.  Is there any
reason for not doing it there?

>                 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 v3 2/3] PM: Support aborting suspend during filesystem sync

[Saravana Kannan]

On Fri, Sep 5, 2025 at 2:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Aug 21, 2025 at 2:43 AM Samuel Wu <wusamuel@google.com> wrote:
> >
> > At the start of suspend, 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 suspend abort signal when in
> > the filesystem sync phase of suspend. This topic was first discussed by
> > Saravana Kannan at LPC 2024 [1], where the general consensus was to
> > allow filesystem sync on a parallel thread.
> >
> > [1]: https://lpc.events/event/18/contributions/1845/
> >
> > Suggested-by: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Samuel Wu <wusamuel@google.com>
>
> The idea is fine with me, but I have comments on the implementation.
>
> First off, I'm not sure how the split of patch [3/3] from this one
> helps because that patch adds mandatory functionality.

It makes the reviewing a lot easier -- at least for me. Especially if
you want to come back and look at the change history. But we are happy
to squash it if you feel strongly.

>  Patch [2/3]
> cannot be applied without patch [3/3], so as far as I'm concerned, it
> is not applicable at all.
>
> > ---
> >  drivers/base/power/wakeup.c |  8 +++++++
> >  include/linux/suspend.h     |  2 ++
> >  kernel/power/suspend.c      | 48 +++++++++++++++++++++++++++++++++++--
> >  3 files changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index d1283ff1080b..af4cf3e6ba44 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 suspend only if events_check_enabled
> > +        * is set (see pm_wakeup_pending()). Similarly, abort suspend during
> > +        * fs_sync only if events_check_enabled is set.
> > +        */
> > +       if (events_check_enabled)
> > +               suspend_abort_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);
> > +       suspend_abort_fs_sync();
> >         s2idle_wake();
> >  }
> >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 317ae31e89b3..68d2e8a7eeb1 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -276,6 +276,7 @@ extern void arch_suspend_enable_irqs(void);
> >
> >  extern int pm_suspend(suspend_state_t state);
> >  extern bool sync_on_suspend_enabled;
> > +extern void suspend_abort_fs_sync(void);
> >  #else /* !CONFIG_SUSPEND */
> >  #define suspend_valid_only_mem NULL
> >
> > @@ -296,6 +297,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
> >  static inline void __init pm_states_init(void) {}
> >  static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
> >  static inline void s2idle_wake(void) {}
> > +static inline void suspend_abort_fs_sync(void) {}
> >  #endif /* !CONFIG_SUSPEND */
> >
> >  static inline bool pm_suspend_in_progress(void)
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 4bb4686c1c08..edacd2a4143b 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"
> >
> > @@ -74,6 +75,19 @@ bool pm_suspend_default_s2idle(void)
> >  }
> >  EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
> >
> > +static DECLARE_COMPLETION(suspend_fs_sync_complete);
> > +
> > +/**
> > + * suspend_abort_fs_sync - Abort fs_sync to abort suspend early
>
> This name is kind of misleading because the function doesn't abort
> fs_sync.  It aborts suspend while fs_sync is in progress.
>
> The fs_sync is continuing and needs to be waited for to complete, at
> least in subsequent suspend/hibernate cycles.
>
> Does system shutdown need to wait for it too?

If the system shutdown does a sync in parallel, does it matter this
one is going on in parallel too? Doesn't seem any different then
shutting down right after userspace calls fsync.

Also, we allow skip sync on suspend today. So I doubt shutdown depends
on sync on suspend for correctness of shutdown.

> > + *
> > + * This function aborts the fs_sync stage of suspend so that suspend itself can
> > + * be aborted early.
> > + */
> > +void suspend_abort_fs_sync(void)
> > +{
> > +       complete(&suspend_fs_sync_complete);
> > +}
> > +
> >  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
> >  {
> >         unsigned int sleep_flags;
> > @@ -403,6 +417,34 @@ void __weak arch_suspend_enable_irqs(void)
> >         local_irq_enable();
> >  }
> >
> > +static void sync_filesystems_fn(struct work_struct *work)
> > +{
> > +       ksys_sync_helper();
> > +       complete(&suspend_fs_sync_complete);
> > +}
> > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > +
> > +/**
> > + * suspend_fs_sync_with_abort - Trigger fs_sync with ability to abort
> > + *
> > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > + * system sync was aborted.
> > + */
> > +static int suspend_fs_sync_with_abort(void)
>
> The "with_abort" part is not needed in this name and I'd prefer
> pm_sleep_ to be used instead of suspend_
>
> > +{
> > +       reinit_completion(&suspend_fs_sync_complete);
> > +       schedule_work(&sync_filesystems);
>
> If you used a dedicated workqueue for this, waiting for the previously
> scheduled filesystems sync would be as simple as calling
> flush_workqueue(pm_fs_sync_wq);

Yeah, that was our first thought too. But unfortunately, we might want
to abort the suspend while the flush is happening (patch 3/3). Which
we can't do with flush_workqueue() and we go back to square one where
a suspend can't be aborted for a long time. That's why we need to
write it this way.

>
> > +       /*
> > +        * Completion is triggered by fs_sync finishing or a suspend abort
> > +        * signal, whichever comes first
> > +        */
> > +       wait_for_completion(&suspend_fs_sync_complete);
> > +       if (pm_wakeup_pending())
> > +               return -EBUSY;
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * suspend_enter - Make the system enter the given sleep state.
> >   * @state: System sleep state to enter.
> > @@ -588,14 +630,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 = suspend_fs_sync_with_abort();
>
> The same thing would also be useful for hibernation.  Is there any
> reason for not doing it there?

No reason not to. We could add support for that in a different patch
series? Hibernate abort is not the top priority for us right now.

Thanks,
Saravana

>
> >                 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 v3 2/3] PM: Support aborting suspend during filesystem sync

[Rafael J. Wysocki]

On Fri, Sep 5, 2025 at 7:07 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Sep 5, 2025 at 2:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Aug 21, 2025 at 2:43 AM Samuel Wu <wusamuel@google.com> wrote:
> > >
> > > At the start of suspend, 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 suspend abort signal when in
> > > the filesystem sync phase of suspend. This topic was first discussed by
> > > Saravana Kannan at LPC 2024 [1], where the general consensus was to
> > > allow filesystem sync on a parallel thread.
> > >
> > > [1]: https://lpc.events/event/18/contributions/1845/
> > >
> > > Suggested-by: Saravana Kannan <saravanak@google.com>
> > > Signed-off-by: Samuel Wu <wusamuel@google.com>
> >
> > The idea is fine with me, but I have comments on the implementation.
> >
> > First off, I'm not sure how the split of patch [3/3] from this one
> > helps because that patch adds mandatory functionality.
>
> It makes the reviewing a lot easier -- at least for me. Especially if
> you want to come back and look at the change history. But we are happy
> to squash it if you feel strongly.
>
> >  Patch [2/3]
> > cannot be applied without patch [3/3], so as far as I'm concerned, it
> > is not applicable at all.
> >
> > > ---
> > >  drivers/base/power/wakeup.c |  8 +++++++
> > >  include/linux/suspend.h     |  2 ++
> > >  kernel/power/suspend.c      | 48 +++++++++++++++++++++++++++++++++++--
> > >  3 files changed, 56 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index d1283ff1080b..af4cf3e6ba44 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 suspend only if events_check_enabled
> > > +        * is set (see pm_wakeup_pending()). Similarly, abort suspend during
> > > +        * fs_sync only if events_check_enabled is set.
> > > +        */
> > > +       if (events_check_enabled)
> > > +               suspend_abort_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);
> > > +       suspend_abort_fs_sync();
> > >         s2idle_wake();
> > >  }
> > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > index 317ae31e89b3..68d2e8a7eeb1 100644
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -276,6 +276,7 @@ extern void arch_suspend_enable_irqs(void);
> > >
> > >  extern int pm_suspend(suspend_state_t state);
> > >  extern bool sync_on_suspend_enabled;
> > > +extern void suspend_abort_fs_sync(void);
> > >  #else /* !CONFIG_SUSPEND */
> > >  #define suspend_valid_only_mem NULL
> > >
> > > @@ -296,6 +297,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
> > >  static inline void __init pm_states_init(void) {}
> > >  static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
> > >  static inline void s2idle_wake(void) {}
> > > +static inline void suspend_abort_fs_sync(void) {}
> > >  #endif /* !CONFIG_SUSPEND */
> > >
> > >  static inline bool pm_suspend_in_progress(void)
> > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > > index 4bb4686c1c08..edacd2a4143b 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"
> > >
> > > @@ -74,6 +75,19 @@ bool pm_suspend_default_s2idle(void)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
> > >
> > > +static DECLARE_COMPLETION(suspend_fs_sync_complete);
> > > +
> > > +/**
> > > + * suspend_abort_fs_sync - Abort fs_sync to abort suspend early
> >
> > This name is kind of misleading because the function doesn't abort
> > fs_sync.  It aborts suspend while fs_sync is in progress.
> >
> > The fs_sync is continuing and needs to be waited for to complete, at
> > least in subsequent suspend/hibernate cycles.
> >
> > Does system shutdown need to wait for it too?
>
> If the system shutdown does a sync in parallel, does it matter this
> one is going on in parallel too? Doesn't seem any different then
> shutting down right after userspace calls fsync.
>
> Also, we allow skip sync on suspend today. So I doubt shutdown depends
> on sync on suspend for correctness of shutdown.

What if the sync is still in progress when shutdown takes place?
Could shutdown somehow break it and cause data corruption, say?

> > > + *
> > > + * This function aborts the fs_sync stage of suspend so that suspend itself can
> > > + * be aborted early.
> > > + */
> > > +void suspend_abort_fs_sync(void)
> > > +{
> > > +       complete(&suspend_fs_sync_complete);
> > > +}
> > > +
> > >  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
> > >  {
> > >         unsigned int sleep_flags;
> > > @@ -403,6 +417,34 @@ void __weak arch_suspend_enable_irqs(void)
> > >         local_irq_enable();
> > >  }
> > >
> > > +static void sync_filesystems_fn(struct work_struct *work)
> > > +{
> > > +       ksys_sync_helper();
> > > +       complete(&suspend_fs_sync_complete);
> > > +}
> > > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > > +
> > > +/**
> > > + * suspend_fs_sync_with_abort - Trigger fs_sync with ability to abort
> > > + *
> > > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > > + * system sync was aborted.
> > > + */
> > > +static int suspend_fs_sync_with_abort(void)
> >
> > The "with_abort" part is not needed in this name and I'd prefer
> > pm_sleep_ to be used instead of suspend_
> >
> > > +{
> > > +       reinit_completion(&suspend_fs_sync_complete);
> > > +       schedule_work(&sync_filesystems);
> >
> > If you used a dedicated workqueue for this, waiting for the previously
> > scheduled filesystems sync would be as simple as calling
> > flush_workqueue(pm_fs_sync_wq);
>
> Yeah, that was our first thought too. But unfortunately, we might want
> to abort the suspend while the flush is happening (patch 3/3). Which
> we can't do with flush_workqueue() and we go back to square one where
> a suspend can't be aborted for a long time. That's why we need to
> write it this way.

So this is an optimization which I'm not sure is really necessary.
Maybe let's start with the flush_workqueue() approach and then
complicate things if it is demonstrated to be insufficient?

> >
> > > +       /*
> > > +        * Completion is triggered by fs_sync finishing or a suspend abort
> > > +        * signal, whichever comes first
> > > +        */
> > > +       wait_for_completion(&suspend_fs_sync_complete);
> > > +       if (pm_wakeup_pending())
> > > +               return -EBUSY;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  /**
> > >   * suspend_enter - Make the system enter the given sleep state.
> > >   * @state: System sleep state to enter.
> > > @@ -588,14 +630,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 = suspend_fs_sync_with_abort();
> >
> > The same thing would also be useful for hibernation.  Is there any
> > reason for not doing it there?
>
> No reason not to. We could add support for that in a different patch
> series?

Not really.

> Hibernate abort is not the top priority for us right now.

With all due respect, please handle them both or none.  Otherwise,
you'd create an arbitrary divergence that would become a maintenance
burden going forward.

Thanks!

Re: [PATCH v3 2/3] PM: Support aborting suspend during filesystem sync

[Saravana Kannan]

On Fri, Sep 5, 2025 at 2:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Sep 5, 2025 at 7:07 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Fri, Sep 5, 2025 at 2:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Aug 21, 2025 at 2:43 AM Samuel Wu <wusamuel@google.com> wrote:
> > > >
> > > > At the start of suspend, 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 suspend abort signal when in
> > > > the filesystem sync phase of suspend. This topic was first discussed by
> > > > Saravana Kannan at LPC 2024 [1], where the general consensus was to
> > > > allow filesystem sync on a parallel thread.
> > > >
> > > > [1]: https://lpc.events/event/18/contributions/1845/
> > > >
> > > > Suggested-by: Saravana Kannan <saravanak@google.com>
> > > > Signed-off-by: Samuel Wu <wusamuel@google.com>
> > >
> > > The idea is fine with me, but I have comments on the implementation.
> > >
> > > First off, I'm not sure how the split of patch [3/3] from this one
> > > helps because that patch adds mandatory functionality.
> >
> > It makes the reviewing a lot easier -- at least for me. Especially if
> > you want to come back and look at the change history. But we are happy
> > to squash it if you feel strongly.
> >
> > >  Patch [2/3]
> > > cannot be applied without patch [3/3], so as far as I'm concerned, it
> > > is not applicable at all.
> > >
> > > > ---
> > > >  drivers/base/power/wakeup.c |  8 +++++++
> > > >  include/linux/suspend.h     |  2 ++
> > > >  kernel/power/suspend.c      | 48 +++++++++++++++++++++++++++++++++++--
> > > >  3 files changed, 56 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > > index d1283ff1080b..af4cf3e6ba44 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 suspend only if events_check_enabled
> > > > +        * is set (see pm_wakeup_pending()). Similarly, abort suspend during
> > > > +        * fs_sync only if events_check_enabled is set.
> > > > +        */
> > > > +       if (events_check_enabled)
> > > > +               suspend_abort_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);
> > > > +       suspend_abort_fs_sync();
> > > >         s2idle_wake();
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > > index 317ae31e89b3..68d2e8a7eeb1 100644
> > > > --- a/include/linux/suspend.h
> > > > +++ b/include/linux/suspend.h
> > > > @@ -276,6 +276,7 @@ extern void arch_suspend_enable_irqs(void);
> > > >
> > > >  extern int pm_suspend(suspend_state_t state);
> > > >  extern bool sync_on_suspend_enabled;
> > > > +extern void suspend_abort_fs_sync(void);
> > > >  #else /* !CONFIG_SUSPEND */
> > > >  #define suspend_valid_only_mem NULL
> > > >
> > > > @@ -296,6 +297,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
> > > >  static inline void __init pm_states_init(void) {}
> > > >  static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
> > > >  static inline void s2idle_wake(void) {}
> > > > +static inline void suspend_abort_fs_sync(void) {}
> > > >  #endif /* !CONFIG_SUSPEND */
> > > >
> > > >  static inline bool pm_suspend_in_progress(void)
> > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > > > index 4bb4686c1c08..edacd2a4143b 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"
> > > >
> > > > @@ -74,6 +75,19 @@ bool pm_suspend_default_s2idle(void)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
> > > >
> > > > +static DECLARE_COMPLETION(suspend_fs_sync_complete);
> > > > +
> > > > +/**
> > > > + * suspend_abort_fs_sync - Abort fs_sync to abort suspend early
> > >
> > > This name is kind of misleading because the function doesn't abort
> > > fs_sync.  It aborts suspend while fs_sync is in progress.
> > >
> > > The fs_sync is continuing and needs to be waited for to complete, at
> > > least in subsequent suspend/hibernate cycles.
> > >
> > > Does system shutdown need to wait for it too?
> >
> > If the system shutdown does a sync in parallel, does it matter this
> > one is going on in parallel too? Doesn't seem any different then
> > shutting down right after userspace calls fsync.
> >
> > Also, we allow skip sync on suspend today. So I doubt shutdown depends
> > on sync on suspend for correctness of shutdown.
>
> What if the sync is still in progress when shutdown takes place?
> Could shutdown somehow break it and cause data corruption, say?

We will take a closer look.

>
> > > > + *
> > > > + * This function aborts the fs_sync stage of suspend so that suspend itself can
> > > > + * be aborted early.
> > > > + */
> > > > +void suspend_abort_fs_sync(void)
> > > > +{
> > > > +       complete(&suspend_fs_sync_complete);
> > > > +}
> > > > +
> > > >  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
> > > >  {
> > > >         unsigned int sleep_flags;
> > > > @@ -403,6 +417,34 @@ void __weak arch_suspend_enable_irqs(void)
> > > >         local_irq_enable();
> > > >  }
> > > >
> > > > +static void sync_filesystems_fn(struct work_struct *work)
> > > > +{
> > > > +       ksys_sync_helper();
> > > > +       complete(&suspend_fs_sync_complete);
> > > > +}
> > > > +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> > > > +
> > > > +/**
> > > > + * suspend_fs_sync_with_abort - Trigger fs_sync with ability to abort
> > > > + *
> > > > + * Return 0 on successful file system sync, otherwise returns -EBUSY if file
> > > > + * system sync was aborted.
> > > > + */
> > > > +static int suspend_fs_sync_with_abort(void)
> > >
> > > The "with_abort" part is not needed in this name and I'd prefer
> > > pm_sleep_ to be used instead of suspend_
> > >
> > > > +{
> > > > +       reinit_completion(&suspend_fs_sync_complete);
> > > > +       schedule_work(&sync_filesystems);
> > >
> > > If you used a dedicated workqueue for this, waiting for the previously
> > > scheduled filesystems sync would be as simple as calling
> > > flush_workqueue(pm_fs_sync_wq);
> >
> > Yeah, that was our first thought too. But unfortunately, we might want
> > to abort the suspend while the flush is happening (patch 3/3). Which
> > we can't do with flush_workqueue() and we go back to square one where
> > a suspend can't be aborted for a long time. That's why we need to
> > write it this way.
>
> So this is an optimization which I'm not sure is really necessary.
> Maybe let's start with the flush_workqueue() approach and then
> complicate things if it is demonstrated to be insufficient?

Unfortunately, back to back suspend attempts are very common in
Android. When we have a lot of dirty pages and approach the long tail
of 25 seconds to sync the file system, this is not a corner case at
all.

Also, even if it's only a 1% chance of happening, that's 10 million
users when the total population is a billion users. And this is very
user perceptible issue that significantly degrades the user experience
(it has happened to me too).

>
> > >
> > > > +       /*
> > > > +        * Completion is triggered by fs_sync finishing or a suspend abort
> > > > +        * signal, whichever comes first
> > > > +        */
> > > > +       wait_for_completion(&suspend_fs_sync_complete);
> > > > +       if (pm_wakeup_pending())
> > > > +               return -EBUSY;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * suspend_enter - Make the system enter the given sleep state.
> > > >   * @state: System sleep state to enter.
> > > > @@ -588,14 +630,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 = suspend_fs_sync_with_abort();
> > >
> > > The same thing would also be useful for hibernation.  Is there any
> > > reason for not doing it there?
> >
> > No reason not to. We could add support for that in a different patch
> > series?
>
> Not really.
>
> > Hibernate abort is not the top priority for us right now.
>
> With all due respect, please handle them both or none.  Otherwise,
> you'd create an arbitrary divergence that would become a maintenance
> burden going forward.

Fair enough. We can take care of that too.

Thanks,
Saravana

[PATCH v3 3/3] PM: Support abort during fs_sync of back-to-back suspends

[Samuel Wu]

There is extra care needed to account for back-to-back suspends 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 suspend's filesystem sync operation will only
start when the previous suspend's filesystem sync has finished. While
waiting for the previous suspend's filesystem sync to finish, the
subsequent suspend will still abort early if a wakeup event is
triggered, solving the original issue of filesystem sync blocking abort.

Suggested-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Samuel Wu <wusamuel@google.com>
Reviewed-by: Saravana Kannan <saravanak@google.com>
---
 kernel/power/suspend.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index edacd2a4143b..514c590ec383 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -75,6 +75,8 @@ bool pm_suspend_default_s2idle(void)
 }
 EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
 
+static bool suspend_fs_sync_queued;
+static DEFINE_SPINLOCK(suspend_fs_sync_lock);
 static DECLARE_COMPLETION(suspend_fs_sync_complete);
 
 /**
@@ -85,7 +87,9 @@ static DECLARE_COMPLETION(suspend_fs_sync_complete);
  */
 void suspend_abort_fs_sync(void)
 {
+	spin_lock(&suspend_fs_sync_lock);
 	complete(&suspend_fs_sync_complete);
+	spin_unlock(&suspend_fs_sync_lock);
 }
 
 void s2idle_set_ops(const struct platform_s2idle_ops *ops)
@@ -420,7 +424,11 @@ void __weak arch_suspend_enable_irqs(void)
 static void sync_filesystems_fn(struct work_struct *work)
 {
 	ksys_sync_helper();
+
+	spin_lock(&suspend_fs_sync_lock);
+	suspend_fs_sync_queued = false;
 	complete(&suspend_fs_sync_complete);
+	spin_unlock(&suspend_fs_sync_lock);
 }
 static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
 
@@ -432,8 +440,26 @@ static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
  */
 static int suspend_fs_sync_with_abort(void)
 {
+	bool need_suspend_fs_sync_requeue;
+
+Start_fs_sync:
+	spin_lock(&suspend_fs_sync_lock);
 	reinit_completion(&suspend_fs_sync_complete);
-	schedule_work(&sync_filesystems);
+	/*
+	 * Handle the case where a suspend immediately follows a previous
+	 * suspend 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 suspending.
+	 */
+	if (suspend_fs_sync_queued) {
+		need_suspend_fs_sync_requeue = true;
+	} else {
+		need_suspend_fs_sync_requeue = false;
+		suspend_fs_sync_queued = true;
+		schedule_work(&sync_filesystems);
+	}
+	spin_unlock(&suspend_fs_sync_lock);
+
 	/*
 	 * Completion is triggered by fs_sync finishing or a suspend abort
 	 * signal, whichever comes first
@@ -441,6 +467,8 @@ static int suspend_fs_sync_with_abort(void)
 	wait_for_completion(&suspend_fs_sync_complete);
 	if (pm_wakeup_pending())
 		return -EBUSY;
+	if (need_suspend_fs_sync_requeue)
+		goto Start_fs_sync;
 
 	return 0;
 }
-- 
2.51.0.261.g7ce5a0a67e-goog

Re: [PATCH v3 3/3] PM: Support abort during fs_sync of back-to-back suspends

[Saravana Kannan]

On Wed, Aug 20, 2025 at 5:43 PM Samuel Wu <wusamuel@google.com> wrote:
>
> There is extra care needed to account for back-to-back suspends 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 suspend's filesystem sync operation will only
> start when the previous suspend's filesystem sync has finished. While
> waiting for the previous suspend's filesystem sync to finish, the
> subsequent suspend will still abort early if a wakeup event is
> triggered, solving the original issue of filesystem sync blocking abort.
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Samuel Wu <wusamuel@google.com>
> Reviewed-by: Saravana Kannan <saravanak@google.com>

I think you meant to add my Reviewed-by: to all 3 patches.

Rafael,

Feel free to add it when you pick up these patches.

-Saravana
> ---
>  kernel/power/suspend.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index edacd2a4143b..514c590ec383 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -75,6 +75,8 @@ bool pm_suspend_default_s2idle(void)
>  }
>  EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
>
> +static bool suspend_fs_sync_queued;
> +static DEFINE_SPINLOCK(suspend_fs_sync_lock);
>  static DECLARE_COMPLETION(suspend_fs_sync_complete);
>
>  /**
> @@ -85,7 +87,9 @@ static DECLARE_COMPLETION(suspend_fs_sync_complete);
>   */
>  void suspend_abort_fs_sync(void)
>  {
> +       spin_lock(&suspend_fs_sync_lock);
>         complete(&suspend_fs_sync_complete);
> +       spin_unlock(&suspend_fs_sync_lock);
>  }
>
>  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
> @@ -420,7 +424,11 @@ void __weak arch_suspend_enable_irqs(void)
>  static void sync_filesystems_fn(struct work_struct *work)
>  {
>         ksys_sync_helper();
> +
> +       spin_lock(&suspend_fs_sync_lock);
> +       suspend_fs_sync_queued = false;
>         complete(&suspend_fs_sync_complete);
> +       spin_unlock(&suspend_fs_sync_lock);
>  }
>  static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
>
> @@ -432,8 +440,26 @@ static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
>   */
>  static int suspend_fs_sync_with_abort(void)
>  {
> +       bool need_suspend_fs_sync_requeue;
> +
> +Start_fs_sync:
> +       spin_lock(&suspend_fs_sync_lock);
>         reinit_completion(&suspend_fs_sync_complete);
> -       schedule_work(&sync_filesystems);
> +       /*
> +        * Handle the case where a suspend immediately follows a previous
> +        * suspend 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 suspending.
> +        */
> +       if (suspend_fs_sync_queued) {
> +               need_suspend_fs_sync_requeue = true;
> +       } else {
> +               need_suspend_fs_sync_requeue = false;
> +               suspend_fs_sync_queued = true;
> +               schedule_work(&sync_filesystems);
> +       }
> +       spin_unlock(&suspend_fs_sync_lock);
> +
>         /*
>          * Completion is triggered by fs_sync finishing or a suspend abort
>          * signal, whichever comes first
> @@ -441,6 +467,8 @@ static int suspend_fs_sync_with_abort(void)
>         wait_for_completion(&suspend_fs_sync_complete);
>         if (pm_wakeup_pending())
>                 return -EBUSY;
> +       if (need_suspend_fs_sync_requeue)
> +               goto Start_fs_sync;
>
>         return 0;
>  }
> --
> 2.51.0.261.g7ce5a0a67e-goog
>