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

[Michal Hocko <mhocko@suse.cz>]

[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
SUSE Labs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs