Message-ID: <CAJZ5v0jiLzMvwBfcXKJEOMqa_U=6OeWymnBCBdxYfcgU+7P1Aw@mail.gmail.com>
Date: Fri, 24 Oct 2025 10:37:59 +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>, 
	Pavel Machek <pavel@...nel.org>, Len Brown <lenb@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>, tuhaowen@...ontech.com, 
	kernel-team@...roid.com, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] PM: Support aborting sleep during filesystem sync

On Fri, Oct 24, 2025 at 12:47 AM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Thu, Oct 23, 2025 at 12:43 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > On Sat, Oct 18, 2025 at 1:39 AM 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 by Saravana Kannan at LPC 2024 [1], where the general
> > > consensus was to allow filesystem sync on a parallel thread. In case of
> > > abort, the suspend process will stop waiting on an in-progress
> > > filesystem sync, and continue by aborting suspend before the filesystem
> > > sync is complete.
> > >
> > > Additionally, there is extra care needed to account for back-to-back
> > > sleeps while maintaining functionality to immediately abort during the
> > > filesystem sync stage. This patch handles this by serializing the
> > > 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.
> >
> > It would be good to spell out the rationale for starting another
> > filesystem sync when suspend starts while the previous sync is still
> > in progress.
> >
> > > [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 v5:
> > > - Update spin_lock() to spin_lock_irqsave() since abort can be in IRQ context
> > > - Updated changelog description to be more precise regarding continuing abort
> > >   sleep before fs_sync() is complete
> > > - Rename abort_sleep_during_fs_sync() to pm_stop_waiting_for_fs_sync()
> > > - Simplify from a goto to do-while in pm_sleep_fs_sync()
> > > - v4 link: https://lore.kernel.org/all/20250911185314.2377124-1-wusamuel@google.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         | 75 +++++++++++++++++++++++++++++++++++++
> > >  kernel/power/suspend.c      |  7 +++-
> > >  5 files changed, 96 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index d1283ff1080b..689c16b08b38 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)
> > > +               pm_stop_waiting_for_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);
> > > +       pm_stop_waiting_for_fs_sync();
> > >         s2idle_wake();
> > >  }
> > >  EXPORT_SYMBOL_GPL(pm_system_wakeup);
> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > index b02876f1ae38..dc6829b3836f 100644
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -450,6 +450,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 pm_stop_waiting_for_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);
> > > @@ -505,6 +507,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 pm_stop_waiting_for_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 14e85ff23551..9c8db4b3c114 100644
> > > --- a/kernel/power/hibernate.c
> > > +++ b/kernel/power/hibernate.c
> > > @@ -824,7 +824,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..81a53d833358 100644
> > > --- a/kernel/power/main.c
> > > +++ b/kernel/power/main.c
> > > @@ -570,6 +570,81 @@ 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);
> > > +
> > > +/**
> > > + * pm_stop_waiting_for_fs_sync - Abort fs_sync to abort sleep early
> > > + *
> > > + * This function causes the suspend process to stop waiting on an in-progress
> > > + * filesystem sync, such that the suspend process can be aborted before the
> > > + * filesystem sync is complete.
> > > + */
> > > +void pm_stop_waiting_for_fs_sync(void)
> > > +{
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&pm_sleep_fs_sync_lock, flags);
> > > +       complete(&pm_sleep_fs_sync_complete);
> > > +       spin_unlock_irqrestore(&pm_sleep_fs_sync_lock, flags);
> > > +}
> > > +
> > > +static void sync_filesystems_fn(struct work_struct *work)
> > > +{
> > > +       unsigned long flags;
> > > +
> > > +       ksys_sync_helper();
> > > +       spin_lock_irqsave(&pm_sleep_fs_sync_lock, flags);
> > > +       pm_sleep_fs_sync_queued = false;
> > > +       complete(&pm_sleep_fs_sync_complete);
> > > +       spin_unlock_irqrestore(&pm_sleep_fs_sync_lock, flags);
> > > +}
> > > +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;
> > > +       unsigned long flags;
> > > +
> > > +       do {
> > > +               spin_lock_irqsave(&pm_sleep_fs_sync_lock, flags);
> > > +               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.
> > > +                */
> > > +               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_irqrestore(&pm_sleep_fs_sync_lock, flags);
> > > +
> > > +               /*
> > > +                * 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;
> > > +       } while (need_pm_sleep_fs_sync_requeue);
> > > +
> > > +       return 0;
> > > +}
> >
> > If I'm not mistaken, the mechanism by which one more sync is started
> > right after completing the previous one (that was in progress when
> > suspend started) can be designed differently.
> >
> > 1. Use a dedicated ordered workqueue for the sync work items.
> > 2. Use a counter instead of the two boolean vars for synchronization.
> > 3. In pm_sleep_fs_sync(), if the counter is less than 2, increment the
> > counter and queue up a sync work item.
> > 4. In sync_filesystems_fn(), decrement the counter.
>
> The problem with this is that we can't reuse the same work item. We'll
> have to allocate one each time. Otherwise, we'll be queuing one that's
> already queued. Right?

Of course you can't queue up an already queued work, but there may be
two of them and then in 3 above use work0 when the counter is 0 and
use work1 when the counter is 1.  No big deal.

Powered by blists - more mailing lists