Re: [PATCH v2] PM: Add configurable sync timeout for suspend and hibernation

Saravana Kannan
2025-09-09T06:19:05Z

On Mon, Sep 8, 2025 at 7:46 PM tuhaowen <tuhaowen@uniontech.com> wrote:
>
> Hi Saravana,
>
> Thank you for your detailed feedback and suggestions. I appreciate you
> taking the time to review my approach. Let me address your concerns and
> explain the real-world issues I'm trying to solve.
>
> > This doesn't really fix our issue where we want to abort suspend only
> > if we have to stay awake. If there's no need to abort the suspend (to
> > deal with some user/wake source request), then it's okay to take the
> > time to sync and then suspend. If you abort the suspend, it's just
> > going to get attempted again and in fact waste power.
>
> I understand your perspective, but I'm addressing a different class of
> problems. The key issue is user experience and system reliability when
> sync operations become indefinitely blocked.
>
> > I also don't understand how your patch fixes anything. If anything
> > this makes things worse because the user might have expected their
> > device to have hibernated only to come back hours later to see their
> > battery dead. And even if the user is actively monitoring it, you
> > still need to sync the file system fully before you eventually
> > suspend. So, this doesn't really save any time or the need to sync.
>
> This highlights exactly the problem I'm trying to solve. When a user
> initiates suspend/hibernation, the screen goes black immediately, giving
> the impression that the system has successfully entered sleep state.
> However, if ksys_sync() is blocked (which can happen in several scenarios
> I'll describe below), the system appears to be suspended but is actually
> still running at full power consumption in the background.
>
> The user has no way to know the system is stuck in sync - they see a
> black screen and assume suspension succeeded. This leads to exactly the
> scenario you mentioned: coming back hours later to find the battery dead,
> but with the added risk of data corruption since the sync never completed.
>
> > Can you explain the actual real world issue you are trying to fix? If
> > it's the UI hanging and not responding to keypress/mouse move, then to
> > me it looks like those should be marked as wake sources.
>
> Here are the specific real-world scenarios I've encountered:
>
> 1. **Device removal during file operations**: When copying large files
> to USB drives and then initiating suspend, if the USB device is
> removed during the sync process, some filesystems may not properly
> handle the device disappearance. The sync can become indefinitely
> blocked waiting for I/O operations on a device that no longer exists.
>
> 2. **Faulty storage devices**: Slow or failing storage devices can cause
> sync operations to hang for extended periods, making the system appear
> unresponsive.
>
> I've observed stack traces like this when the block device is removed during sync:
>
> ```
> [<0>] __switch_to+0xd0/0x168
> [<0>] iterate_supers+0x88/0x118
> [<0>] ksys_sync+0x48/0xb8
> [<0>] ksys_sync_helper+0x18/0xa0
> [<0>] pm_suspend+0x260/0x3e8
> ```

This seems like a fundamental sync issue you are trying to paper
over/work around. I think the right fix should be in the ksys_sync()
code.

> In this case, ksys_sync() is permanently blocked and will never complete,
> even though the user believes the system has suspended.
>
> My timeout approach provides several benefits:
>
> 1. **User feedback**: If I set a 1-minute timeout and sync doesn't
> complete, the system fails suspend gracefully and returns control to
> the user. This gives them clear indication that something went wrong,
> rather than leaving them with a black screen and unknown system state.
>
> 2. **Prevents false assumptions**: Users won't mistakenly believe their
> system is suspended when it's actually consuming full power.
>
> 3. **Allows recovery**: Users can investigate the issue, safely eject
> devices, or take other corrective actions.

If the user has closed the laptop, none of these will be noticed by
the user. And the laptop will still consume power since it's not
suspended and cause a dead battery hours later.

> > However, if you are really set on proving the need for a timeout and
> > implementing it, you can always add it as a separate patch after our
> > patch lands. You can set up a timer to call pm_system_wakeup(). Or
> > just grab a wake source after a time period.
>
> I appreciate this suggestion, and I'm certainly willing to coordinate our
> approaches. However, I believe the sync timeout addresses a fundamentally
> different problem than wakeup-event-based abortion.
>
> Regarding your concern about sync completion after timeout, I want to
> clarify that in my updated implementation, I've removed kthread_stop() to
> ensure the sync operation continues in the background even after timeout.

I don't think the kthread_stop() is even correct to do. You are just
randomly killing the sync thread where data structures and locks could
be left in a bad state.

> This means:
>
> 1. The suspend operation fails immediately with a timeout error, giving
> the user feedback
> 2. The sync operation continues running in the background to completion
> 3. Data integrity is preserved while providing responsive user experience
>
> This approach ensures that we get both the user experience benefits
> (immediate feedback) and data safety (background sync completion).
>
> I believe both our approaches serve different but complementary purposes:
> - Your approach: Handle cases where wakeup events should abort sync
> - My approach: Handle cases where sync becomes pathologically slow or
> blocked

I'm still not sold on the timeout being the right solution to your
issue, but I'm not going to block the feature.

> Would you be open to discussing how we might integrate both approaches?
> I'm happy to work on this as a follow-up patch series after your changes
> land, or we could explore a unified solution that handles both scenarios.

I think a follow up patch is the best approach. We'll give you the
APIs to abort as needed (timer in your case) and you can try to
convince the community that the timeout is needed.

>
> > In fact, thinking more about this, you are generally having a problem
> > with suspend taking too long to complete. Doesn't matter if it's due
> > to file system sync or anything else. In which case, you should just
> > use a timer to call pm_system_wakeup() and not fix just file system
> > sync (which happens to be the current long pole).
>
> This is an interesting perspective. However, I believe filesystem sync
> deserves special attention because:
>
> 1. It's currently the most common source of indefinite hangs during
> suspend

This is not a reason for special casing for the file system.

> 2. The consequences of interrupted vs. timed-out sync are different
> (data integrity)

Not really. You do realize that even in our patch we let the sync
continue in the background, right?

> 3. Sync has specific requirements for background completion that other
> suspend operations may not have

Not sure what you mean.

> I'd be very interested in your thoughts on this approach and whether we
> can find a path forward that addresses both our use cases. I really
> appreciate your patience in this discussion and look forward to
> continuing our collaboration on this important issue.

I don't want to block your effort, but I also can't support it. So a
unified patch doesn't seem like the right way to go. As I mentioned
above, I'd recommend building on top of what we land.

> Thanks again for taking the time to review and provide such detailed
> feedback.

Same to you!

Thanks,
Saravana