Re: [PATCH v5] PM: Support aborting sleep during filesystem sync

[Rafael J. Wysocki]

On Wed, Oct 29, 2025 at 8:13 PM Samuel Wu <wusamuel@xxxxxxxxxx> wrote:
>
> On Wed, Oct 29, 2025 at 6:23 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> >
> > On Fri, Oct 24, 2025 at 10:37 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 24, 2025 at 12:47 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Oct 23, 2025 at 12:43 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Sat, Oct 18, 2025 at 1:39 AM Samuel Wu <wusamuel@xxxxxxxxxx> wrote:
> > > > > >
> >
> > [cut]
> >
> > > > >
> > > > > 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.
> >
> > Moreover, sync_filesystems_fn() doesn't use its work argument, so the
> > work can be requeued as soon as it is not pending.
> >
> > Now, when it is not pending, it has not been queued yet or the work
> > function is running, which is exactly when you want it to be queued:
> >
> > 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 work is not pending, queue it up and
> > increment the counter.
> > 4. In sync_filesystems_fn(), decrement the counter (after carrying out
> > the fs sync) and complete the completion if the counter is 0.
>
> Thank you for the thoughtful feedback Rafael.
>
> I agree with these points and will incorporate it in v6; this approach
> seems more elegant.
>
> > Of course, the above requires locking, but I don't think that the lock
> > needs to be spinlock.  A mutex would work just as well AFAICS.
> >
> > Thanks!
>
> I'm still thinking this needs to be spin_lock_irqsave(), since
> pm_stop_waiting_for_fs_sync() is called in an interrupt context and
> needs the lock such that the abort signals don't get lost.

OK, I see.  __pm_stay_awake() will call it under a spinlock, for one example.

Yes, you need a spinlock, but in thread context it is sufficient to
use spin_lock_irq() (no need to save the flags).