Message-ID: <CAJZ5v0hwgNt3+stLDpdVbrpDfomEaQ26KikUDFN2QjBpNMgbfQ@mail.gmail.com>
Date: Mon, 13 Oct 2025 20:15:46 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Samuel Wu <wusamuel@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>, 
	Saravana Kannan <saravanak@...gle.com>, kernel-team@...roid.com, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

On Thu, Sep 11, 2025 at 8:53 PM Samuel Wu <wusamuel@...gle.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@...gle.com>
> Signed-off-by: Samuel Wu <wusamuel@...gle.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)
> --

Powered by blists - more mailing lists