drivers/base/power/wakeup.c | 8 ++++ include/linux/suspend.h | 3 ++ kernel/power/process.c | 1 - kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 2 deletions(-)
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 | 3 ++
kernel/power/process.c | 1 -
kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++-
4 files changed, 95 insertions(+), 2 deletions(-)
v1 -> v2:
- Added documentation for suspend_abort_fs_sync()
- Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index d1283ff1080b..304368c3a55f 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);
+ /*
+ * To maintain the same behavior as pm_wakeup_pending(),
+ * aborting suspend will only happen if events_check_enabled. Similarly,
+ * the abort during fs_sync needs the same check.
+ */
+ 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..21b1ea275c79 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -276,6 +276,8 @@ 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 +298,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/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..dc37ab942bcb 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,21 @@ 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);
+
+/**
+ * Triggers the completion that aborts suspend. This completion will only have
+ * an effect if called during filesystems sync step of suspend.
+ */
+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)
{
unsigned int sleep_flags;
@@ -403,6 +419,71 @@ void __weak arch_suspend_enable_irqs(void)
local_irq_enable();
}
+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);
+
+/**
+ * suspend_fs_sync_with_abort- Start filesystem sync and handle potential aborts
+ *
+ * Starts filesystem sync in a workqueue, while the main thread uses a
+ * completion to wait for either the filesystem sync to finish or for a wakeup
+ * event. In the case of filesystem sync finishing and triggering the
+ * completion, the suspend path continues as normal. If the complete is due to a
+ * wakeup or abort signal, the code jumps to the suspend abort path while the
+ * filesystem sync finishes in the background.
+ *
+ * An aborted suspend that is followed by another suspend is a potential
+ * scenario that complicates the sequence. This patch handles this by
+ * serializing any filesystem sync; a subsequent suspend's filesystem sync
+ * operation will only start when the previous suspend's filesystem sync has
+ * finished. Even while waiting for the previous suspend's filesystem sync to
+ * finish, the subsequent suspend will still break early if a wakeup completion
+ * is triggered, solving the original issue of filesystem sync blocking abort.
+ */
+static int suspend_fs_sync_with_abort(void)
+{
+ bool need_suspend_fs_sync_requeue;
+
+ pm_wakeup_clear(0);
+Start_fs_sync:
+ spin_lock(&suspend_fs_sync_lock);
+ reinit_completion(&suspend_fs_sync_complete);
+ /*
+ * Handle the case where a suspend immediately follows a previous
+ * suspend that was aborted during fs_sync. In this case, serialize
+ * fs_sync by only starting fs_sync of the subsequent suspend when the
+ * fs_sync of the previous suspend has finished.
+ */
+ 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
+ */
+ 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;
+}
+
/**
* suspend_enter - Make the system enter the given sleep state.
* @state: System sleep state to enter.
@@ -590,8 +671,10 @@ static int enter_state(suspend_state_t state)
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]);
--
2.51.0.rc1.167.g924127e9c0-googOn Thu, Aug 14, 2025 at 5:46 PM 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>
Minor comments on documentation. The code itself seems good to me.
> ---
> drivers/base/power/wakeup.c | 8 ++++
> include/linux/suspend.h | 3 ++
> kernel/power/process.c | 1 -
> kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++-
> 4 files changed, 95 insertions(+), 2 deletions(-)
>
> v1 -> v2:
> - Added documentation for suspend_abort_fs_sync()
> - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..304368c3a55f 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);
> + /*
> + * To maintain the same behavior as pm_wakeup_pending(),
> + * aborting suspend will only happen if events_check_enabled. Similarly,
> + * the abort during fs_sync needs the same check.
> + */
Maybe something like this would be clearer?
wakeup source active aborts suspend only if events_check_enabled is
set (See pm_wakeup_pending()). Similarly, it should abort 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..21b1ea275c79 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -276,6 +276,8 @@ 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 +298,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/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..dc37ab942bcb 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,21 @@ 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);
> +
> +/**
> + * Triggers the completion that aborts suspend. This completion will only have
> + * an effect if called during filesystems sync step of suspend.
> + */
This is explaining the implementation of the function. It's better to
explain the semantics of the API.
For example (please feel free to rewrite it):
/* 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)
> +{
> + 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)
> {
> unsigned int sleep_flags;
> @@ -403,6 +419,71 @@ void __weak arch_suspend_enable_irqs(void)
> local_irq_enable();
> }
>
> +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);
> +
> +/**
> + * suspend_fs_sync_with_abort- Start filesystem sync and handle potential aborts
> + *
> + * Starts filesystem sync in a workqueue, while the main thread uses a
> + * completion to wait for either the filesystem sync to finish or for a wakeup
> + * event. In the case of filesystem sync finishing and triggering the
> + * completion, the suspend path continues as normal. If the complete is due to a
> + * wakeup or abort signal, the code jumps to the suspend abort path while the
> + * filesystem sync finishes in the background.
This is again talking about implementation. Not great for a function
doc. It's better to have these next to the implementation.
> + *
> + * An aborted suspend that is followed by another suspend is a potential
> + * scenario that complicates the sequence. This patch handles this by
Don't talk about a specific patch in an inline comment. This will be
here long after your patch is merged.
> + * serializing any filesystem sync; a subsequent suspend's filesystem sync
> + * operation will only start when the previous suspend's filesystem sync has
> + * finished. Even while waiting for the previous suspend's filesystem sync to
> + * finish, the subsequent suspend will still break early if a wakeup completion
> + * is triggered, solving the original issue of filesystem sync blocking abort.
Most of the context of this comment is already explained below. I
don't think this function needs a function doc. But if you must, a
simple semantic documentation should be enough:
/* suspend_fs_sync_with_abort -- Tigger fs_sync with ability to abort.
This function initiates a file system sync with the ability to abort.
Return 0 on successful file system sync.
Or -EBUSY if file system sync was aborted
*/
> + */
> +static int suspend_fs_sync_with_abort(void)
> +{
> + bool need_suspend_fs_sync_requeue;
> +
> + pm_wakeup_clear(0);
> +Start_fs_sync:
> + spin_lock(&suspend_fs_sync_lock);
> + reinit_completion(&suspend_fs_sync_complete);
> + /*
> + * Handle the case where a suspend immediately follows a previous
> + * suspend that was aborted during fs_sync. In this case, serialize
In this case, wait for the previous file system sync to finish. Then
do another file system sync so any subsequent changes to the file
systems are synced before suspending.
Assuming you address these non-functional comments:
Reviewed-by: Saravana Kannan <saravanak@google.com>
Thanks,
Saravana
> + * fs_sync by only starting fs_sync of the subsequent suspend when the
> + * fs_sync of the previous suspend has finished.
> + */
> + 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
> + */
> + 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;
> +}
> +
> /**
> * suspend_enter - Make the system enter the given sleep state.
> * @state: System sleep state to enter.
> @@ -590,8 +671,10 @@ static int enter_state(suspend_state_t state)
>
> 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]);
> --
> 2.51.0.rc1.167.g924127e9c0-goog
>
On Thu, Aug 14, 2025 at 05:46:34PM -0700, Samuel Wu 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>
> ---
> drivers/base/power/wakeup.c | 8 ++++
> include/linux/suspend.h | 3 ++
> kernel/power/process.c | 1 -
> kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++-
> 4 files changed, 95 insertions(+), 2 deletions(-)
>
> v1 -> v2:
> - Added documentation for suspend_abort_fs_sync()
> - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..304368c3a55f 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);
> + /*
> + * To maintain the same behavior as pm_wakeup_pending(),
> + * aborting suspend will only happen if events_check_enabled. Similarly,
> + * the abort during fs_sync needs the same check.
> + */
> + 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..21b1ea275c79 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -276,6 +276,8 @@ 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 +298,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/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..dc37ab942bcb 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,21 @@ 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);
> +
> +/**
> + * Triggers the completion that aborts suspend. This completion will only have
> + * an effect if called during filesystems sync step of suspend.
> + */
> +void suspend_abort_fs_sync(void)
This is not kerneldoc format, I think the parser will fail on it, right?
Have you tried building the kernel documentation with this patch
applied?
> +{
> + 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)
> {
> unsigned int sleep_flags;
> @@ -403,6 +419,71 @@ void __weak arch_suspend_enable_irqs(void)
> local_irq_enable();
> }
>
> +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);
> +
> +/**
> + * suspend_fs_sync_with_abort- Start filesystem sync and handle potential aborts
> + *
> + * Starts filesystem sync in a workqueue, while the main thread uses a
> + * completion to wait for either the filesystem sync to finish or for a wakeup
> + * event. In the case of filesystem sync finishing and triggering the
> + * completion, the suspend path continues as normal. If the complete is due to a
> + * wakeup or abort signal, the code jumps to the suspend abort path while the
> + * filesystem sync finishes in the background.
> + *
> + * An aborted suspend that is followed by another suspend is a potential
> + * scenario that complicates the sequence. This patch handles this by
> + * serializing any filesystem sync; a subsequent suspend's filesystem sync
> + * operation will only start when the previous suspend's filesystem sync has
> + * finished. Even while waiting for the previous suspend's filesystem sync to
> + * finish, the subsequent suspend will still break early if a wakeup completion
> + * is triggered, solving the original issue of filesystem sync blocking abort.
> + */
Shouldn't this documentation go up in the public one?
thanks,
greg k-h
On Thu, Aug 14, 2025 at 11:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 14, 2025 at 05:46:34PM -0700, Samuel Wu 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>
> > ---
> > drivers/base/power/wakeup.c | 8 ++++
> > include/linux/suspend.h | 3 ++
> > kernel/power/process.c | 1 -
> > kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++-
> > 4 files changed, 95 insertions(+), 2 deletions(-)
> >
> > v1 -> v2:
> > - Added documentation for suspend_abort_fs_sync()
> > - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index d1283ff1080b..304368c3a55f 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);
> > + /*
> > + * To maintain the same behavior as pm_wakeup_pending(),
> > + * aborting suspend will only happen if events_check_enabled. Similarly,
> > + * the abort during fs_sync needs the same check.
> > + */
> > + 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..21b1ea275c79 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -276,6 +276,8 @@ 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 +298,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/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..dc37ab942bcb 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,21 @@ 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);
> > +
> > +/**
> > + * Triggers the completion that aborts suspend. This completion will only have
> > + * an effect if called during filesystems sync step of suspend.
> > + */
> > +void suspend_abort_fs_sync(void)
>
> This is not kerneldoc format, I think the parser will fail on it, right?
> Have you tried building the kernel documentation with this patch
> applied?
You're right, I will update to the proper format in v3. Moving forward, I'll add
kernel-doc.pl to my validation workflow; I was only using checkpatch.pl and
extra gcc checks.
> > +{
> > + 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)
> > {
> > unsigned int sleep_flags;
> > @@ -403,6 +419,71 @@ void __weak arch_suspend_enable_irqs(void)
> > local_irq_enable();
> > }
> >
> > +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);
> > +
> > +/**
> > + * suspend_fs_sync_with_abort- Start filesystem sync and handle potential aborts
> > + *
> > + * Starts filesystem sync in a workqueue, while the main thread uses a
> > + * completion to wait for either the filesystem sync to finish or for a wakeup
> > + * event. In the case of filesystem sync finishing and triggering the
> > + * completion, the suspend path continues as normal. If the complete is due to a
> > + * wakeup or abort signal, the code jumps to the suspend abort path while the
> > + * filesystem sync finishes in the background.
> > + *
> > + * An aborted suspend that is followed by another suspend is a potential
> > + * scenario that complicates the sequence. This patch handles this by
> > + * serializing any filesystem sync; a subsequent suspend's filesystem sync
> > + * operation will only start when the previous suspend's filesystem sync has
> > + * finished. Even while waiting for the previous suspend's filesystem sync to
> > + * finish, the subsequent suspend will still break early if a wakeup completion
> > + * is triggered, solving the original issue of filesystem sync blocking abort.
> > + */
>
> Shouldn't this documentation go up in the public one?
>
> thanks,
>
> greg k-h
I felt this doc was appropriate here as suspend_fs_sync_with_abort() does the
main lifting of the feature. I'm perfectly fine moving it if that way is more
clear or is the convention.
Appreciate the feedback.
-- Samuel Wu
© 2016 - 2025 Red Hat, Inc.