Message-ID: <CAJZ5v0gT2h_SgPDh=t-XWxgWnm8diG_rMuDVo-RnQxqvA7chPw@mail.gmail.com>
Date: Thu, 4 Sep 2025 21:22:33 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Samuel Wu <wusamuel@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@...nel.org>, Len Brown <lenb@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>, kernel-team@...roid.com, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Saravana Kannan <saravanak@...gle.com>
Subject: Re: [PATCH v3 2/3] PM: Support aborting suspend during filesystem sync

On Thu, Aug 21, 2025 at 2:43 AM Samuel Wu <wusamuel@...gle.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@...gle.com>
> Signed-off-by: Samuel Wu <wusamuel@...gle.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)
> --

Powered by blists - more mailing lists