Re: [PATCH v2] PM: Support aborting suspend during filesystem sync

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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