Message-ID: <CAGETcx-3n9Qkiu70Pd2=sjSRsXNMf=CsJQdNLbHUS089qf=vsw@mail.gmail.com>
Date: Fri, 5 Sep 2025 12:07:13 +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>, 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 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.

> > + *
> > + * 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.

>
> > +       /*
> > +        * 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? Hibernate abort is not the top priority for us right now.

Thanks,
Saravana

>
> >                 trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> > +               if (error)
> > +                       goto Unlock;
> >         }
> >
> >         pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> > -       pm_wakeup_clear(0);
> >         pm_suspend_clear_flags();
> >         error = suspend_prepare(state);
> >         if (error)
> > --

Powered by blists - more mailing lists