Re: [PATCH] Revert "xfs: clear PF_NOFREEZE for xfsaild kthread"

Michal Hocko
2016-01-21T10:21:40Z

[CCing Rafael - the thread starts here
http://lkml.kernel.org/r/1452661968-11482-1-git-send-email-david%40fromorbit.com]

On Thu 21-01-16 08:19:44, Dave Chinner wrote:
> On Wed, Jan 20, 2016 at 09:47:50AM +0100, Michal Hocko wrote:
> > On Wed 13-01-16 16:12:48, Dave Chinner wrote:
> > > This reverts commit 24ba16bb3d499c49974669cd8429c3e4138ab102 as it
> > > prevents machines from suspending. This regression occurs when the
> > > xfsaild is idle on entry to suspend, and so there s no activity to
> > > wake it from it's idle sleep and hence see that it is supposed to
> > > freeze. Hence the freezer times out waiting for it and suspend is
> > > cancelled.
> > >
> > > There is no obvious fix for this short of freezing the filesystem
> > > properly, so revert this change for now.
> >
> > We had a similar report opensuse bugzilla just recently. I believe the
> > proper fix should be the following:
> ....
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index aa67339b9537..d6c9c3e9e02b 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -520,14 +520,14 @@ xfsaild(
> > if (!xfs_ail_min(ailp) &&
> > ailp->xa_target == ailp->xa_target_prev) {
> > spin_unlock(&ailp->xa_lock);
> > - schedule();
> > + freezable_schedule();
>
> Oh, wonderful. A completely separate set of undocumented schedule
> functions defined inside the freezer header files that are needed
> just for freezable kthreads.

Yeah, I am not really happy about them either.

> Perhaps there should be some documentation somewhere on how to write
> suspend safe kthreads?

There is something in Documentation/power/freezing-of-tasks.txt but
freezable_schedule* is not mentioned there. Prhaps something like the
following would make it more clear?

diff --git a/Documentation/power/freezing-of-tasks.txt b/Documentation/power/freezing-of-tasks.txt
index 85894d83b352..4df53f0c05a6 100644
--- a/Documentation/power/freezing-of-tasks.txt
+++ b/Documentation/power/freezing-of-tasks.txt
@@ -56,8 +56,11 @@ calling try_to_freeze(). The main loop of a freezable kernel thread may look
If a freezable kernel thread fails to call try_to_freeze() after the freezer has
initiated a freezing operation, the freezing of tasks will fail and the entire
hibernation operation will be cancelled. For this reason, freezable kernel
-threads must call try_to_freeze() somewhere or use one of the
-wait_event_freezable() and wait_event_freezable_timeout() macros.
+threads must call try_to_freeze() somewhere, use wait_event_freezable*()
+and wait_event_freezable_timeout() macros or use freezable_schedule* to hide
+the kernel thread from the suspend if it is guaranteed that a later wake up
+cannot interact with the suspend process (e.g. there is a try_to_freeze call
+or no IO is submitted or any of the suspended devices is not accessed etc..)

After the system memory state has been restored from a hibernation image and
devices have been reinitialized, the function thaw_processes() is called in

> This is too late for the current merge window, but I'll queue it up
> for the next one so we can get some testing on it first.

Sure, no problem and thanks!
--
Michal Hocko