Message-ID: <CAJZ5v0gerKe7fmTh==F4HOOAmF60devB2N8pV1fn9QzX45JzAA@mail.gmail.com>
Date: Fri, 5 Sep 2025 11:38:31 +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>, kernel-team@...roid.com, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] PM: Support aborting suspend during filesystem sync

On Fri, Sep 5, 2025 at 7:07 AM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Fri, Sep 5, 2025 at 2:22 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > 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.
>
> It makes the reviewing a lot easier -- at least for me. Especially if
> you want to come back and look at the change history. But we are happy
> to squash it if you feel strongly.
>
> >  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?
>
> If the system shutdown does a sync in parallel, does it matter this
> one is going on in parallel too? Doesn't seem any different then
> shutting down right after userspace calls fsync.
>
> Also, we allow skip sync on suspend today. So I doubt shutdown depends
> on sync on suspend for correctness of shutdown.

What if the sync is still in progress when shutdown takes place?
Could shutdown somehow break it and cause data corruption, say?

> > > + *
> > > + * 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);
>
> Yeah, that was our first thought too. But unfortunately, we might want
> to abort the suspend while the flush is happening (patch 3/3). Which
> we can't do with flush_workqueue() and we go back to square one where
> a suspend can't be aborted for a long time. That's why we need to
> write it this way.

So this is an optimization which I'm not sure is really necessary.
Maybe let's start with the flush_workqueue() approach and then
complicate things if it is demonstrated to be insufficient?

> >
> > > +       /*
> > > +        * 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?
>
> No reason not to. We could add support for that in a different patch
> series?

Not really.

> Hibernate abort is not the top priority for us right now.

With all due respect, please handle them both or none.  Otherwise,
you'd create an arbitrary divergence that would become a maintenance
burden going forward.

Thanks!

Powered by blists - more mailing lists