Message-ID: <CAG2KctozO9cVeS5KO8o_S04u7uhbTzheFLE8r9mBXTW7nwnQOw@mail.gmail.com>
Date: Fri, 15 Aug 2025 12:03:49 -0700
From: Samuel Wu <wusamuel@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, Pavel Machek <pavel@...nel.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 v2] PM: Support aborting suspend during filesystem sync

On Thu, Aug 14, 2025 at 11:20 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.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@...gle.com>
> > Signed-off-by: Samuel Wu <wusamuel@...gle.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

Powered by blists - more mailing lists