Message-ID: <CAGETcx8ZL3jAwFRxO1B8SFSWmC2jCitc9_61hBG-N2AvzRQv7w@mail.gmail.com>
Date: Thu, 23 Oct 2025 15:46:16 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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 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?

-Saravana

Powered by blists - more mailing lists