Message-ID: <CAJZ5v0j-vHA1OC4QMHssN-NxV23EQkZCz4VYCjKaD42_LD_Oeg@mail.gmail.com>
Date: Mon, 13 Oct 2025 20:39:00 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Samuel Wu <wusamuel@...gle.com>, Len Brown <lenb@...nel.org>, 
	Pavel Machek <pavel@...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
Subject: Re: [PATCH v4] PM: Support aborting sleep during filesystem sync

On Mon, Oct 13, 2025 at 8:34 PM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Mon, Oct 13, 2025 at 11:15 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > 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?
>
> Yeah, since the fs syncs can take up to 25 seconds in some cases,
> there's enough time to create new dirty data that needs to be written
> to disk. So, we want to sync again to write all of that out as if the
> previous attempt/abort hadn't happened. And to do that correctly, we
> have to let the existing sync finish and then kick off the new one
> once the "work" and "completion" are in a good state.

And if the new suspend is aborted while the new fs sync is in
progress, then you'll repeat the exercise and so on, possibly ad
infinitum?

> >
> > Some files may still be updated after it is complete and before all
> > tasks are frozen.
>
> The time window can be very large here. The one you are referring to
> is just a few milliseconds.

It can take up to 20 sec.

Powered by blists - more mailing lists