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

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

[Dave Chinner]

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.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_trans_ail.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index aa67339..4f18fd9 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -497,7 +497,6 @@ xfsaild(
 	long		tout = 0;	/* milliseconds */
 
 	current->flags |= PF_MEMALLOC;
-	set_freezable();
 
 	while (!kthread_should_stop()) {
 		if (tout && tout <= 20)
-- 
2.5.0

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

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

[Jiri Kosina]

On Wed, 13 Jan 2016, 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.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>

I hope I'll find time in the future and make some progress on making the 
fs freezing happen during suspend properly. Unfortunately, before that 
happens

	Acked-by: Jiri Kosina <jkosina@suse.cz>

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

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

[Brian Foster]

On Wed, Jan 13, 2016 at 04:12:48PM +1100, 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.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_trans_ail.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index aa67339..4f18fd9 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -497,7 +497,6 @@ xfsaild(
>  	long		tout = 0;	/* milliseconds */
>  
>  	current->flags |= PF_MEMALLOC;
> -	set_freezable();
>  
>  	while (!kthread_should_stop()) {
>  		if (tout && tout <= 20)
> -- 
> 2.5.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

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

[Michal Hocko]

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:
---
>From ae910a86ada86804c34cc8136afebc9fefa15813 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 19 Jan 2016 20:28:49 +0100
Subject: [PATCH] xfs, xfsaild: Do not block suspend

Hendik has reported suspend failures due to xfsaild blocking the freezer
to settle down.
Jan 17 19:59:56 linux-6380 kernel: PM: Syncing filesystems ... done.
Jan 17 19:59:56 linux-6380 kernel: PM: Preparing system for sleep (mem)
Jan 17 19:59:56 linux-6380 kernel: Freezing user space processes ... (elapsed 0.001 seconds) done.
Jan 17 19:59:56 linux-6380 kernel: Freezing remaining freezable tasks ...
Jan 17 19:59:56 linux-6380 kernel: Freezing of tasks failed after 20.002 seconds (1 tasks refusing to freeze, wq_busy=0):
Jan 17 19:59:56 linux-6380 kernel: xfsaild/dm-5    S 00000000     0  1293      2 0x00000080
Jan 17 19:59:56 linux-6380 kernel:  f0ef5f00 00000046 00000200 00000000 ffff9022 c02d3800 00000000 00000032
Jan 17 19:59:56 linux-6380 kernel:  ee0b2400 00000032 f71e0d00 f36fabc0 f0ef2d00 f0ef6000 f0ef2d00 f12f90c0
Jan 17 19:59:56 linux-6380 kernel:  f0ef5f0c c0844e44 00000000 f0ef5f6c f811e0be 00000000 00000000 f0ef2d00
Jan 17 19:59:56 linux-6380 kernel: Call Trace:
Jan 17 19:59:56 linux-6380 kernel:  [<c0844e44>] schedule+0x34/0x90
Jan 17 19:59:56 linux-6380 kernel:  [<f811e0be>] xfsaild+0x5de/0x600 [xfs]
Jan 17 19:59:56 linux-6380 kernel:  [<c0286cbb>] kthread+0x9b/0xb0
Jan 17 19:59:56 linux-6380 kernel:  [<c0848a79>] ret_from_kernel_thread+0x21/0x38

The issue has been there for quite some time but it has been made
visible by only by 24ba16bb3d49 ("xfs: clear PF_NOFREEZE for xfsaild
kthread") because the suspend started seeing xfsaild.

The above commit has missed that the !xfs_ail_min branch might call
schedule with TASK_INTERRUPTIBLE without calling try_to_freeze so the pm
suspend would wake up the kernel thread over and over again without any
progress. What we want here is to use freezable_schedule instead to hide
the thread from the suspend.

While we are here also change schedule_timeout to freezable variant to
prevent from spurious wakeups by suspend.

Reported-by: Hendrik Woltersdorf <hendrikw@arcor.de>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/xfs/xfs_trans_ail.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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();
 			tout = 0;
 			continue;
 		}
 		spin_unlock(&ailp->xa_lock);
 
 		if (tout)
-			schedule_timeout(msecs_to_jiffies(tout));
+			freezable_schedule_timeout(msecs_to_jiffies(tout));
 
 		__set_current_state(TASK_RUNNING);
 
-- 
2.7.0.rc3

-- 
Michal Hocko
SUSE Labs

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

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

[Dave Chinner]

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. Perhaps there should be some
documentation somewhere on how to write suspend safe kthreads?

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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Michal Hocko]

[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

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

[Dave Chinner]

On Thu, Jan 21, 2016 at 11:21:30AM +0100, Michal Hocko wrote:
> [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?

When I did "git grep freezable_schedule" nothing showed up in the
Documentation directory. I'd suggest that the entire API needs
reference in Documentation/scheduler (i.e. all the
freezable_schedule_timeout variants, the wait_event stuff, etc) as
well as in the freezer documentation so that git grep finds them in
the appropriate places....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Michal Hocko]

On Wed 20-01-16 09:47:50, 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:
> ---
> From ae910a86ada86804c34cc8136afebc9fefa15813 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 19 Jan 2016 20:28:49 +0100
> Subject: [PATCH] xfs, xfsaild: Do not block suspend
> 
> Hendik has reported suspend failures due to xfsaild blocking the freezer
> to settle down.
> Jan 17 19:59:56 linux-6380 kernel: PM: Syncing filesystems ... done.
> Jan 17 19:59:56 linux-6380 kernel: PM: Preparing system for sleep (mem)
> Jan 17 19:59:56 linux-6380 kernel: Freezing user space processes ... (elapsed 0.001 seconds) done.
> Jan 17 19:59:56 linux-6380 kernel: Freezing remaining freezable tasks ...
> Jan 17 19:59:56 linux-6380 kernel: Freezing of tasks failed after 20.002 seconds (1 tasks refusing to freeze, wq_busy=0):
> Jan 17 19:59:56 linux-6380 kernel: xfsaild/dm-5    S 00000000     0  1293      2 0x00000080
> Jan 17 19:59:56 linux-6380 kernel:  f0ef5f00 00000046 00000200 00000000 ffff9022 c02d3800 00000000 00000032
> Jan 17 19:59:56 linux-6380 kernel:  ee0b2400 00000032 f71e0d00 f36fabc0 f0ef2d00 f0ef6000 f0ef2d00 f12f90c0
> Jan 17 19:59:56 linux-6380 kernel:  f0ef5f0c c0844e44 00000000 f0ef5f6c f811e0be 00000000 00000000 f0ef2d00
> Jan 17 19:59:56 linux-6380 kernel: Call Trace:
> Jan 17 19:59:56 linux-6380 kernel:  [<c0844e44>] schedule+0x34/0x90
> Jan 17 19:59:56 linux-6380 kernel:  [<f811e0be>] xfsaild+0x5de/0x600 [xfs]
> Jan 17 19:59:56 linux-6380 kernel:  [<c0286cbb>] kthread+0x9b/0xb0
> Jan 17 19:59:56 linux-6380 kernel:  [<c0848a79>] ret_from_kernel_thread+0x21/0x38
> 
> The issue has been there for quite some time but it has been made
> visible by only by 24ba16bb3d49 ("xfs: clear PF_NOFREEZE for xfsaild
> kthread") because the suspend started seeing xfsaild.
> 
> The above commit has missed that the !xfs_ail_min branch might call
> schedule with TASK_INTERRUPTIBLE without calling try_to_freeze so the pm
> suspend would wake up the kernel thread over and over again without any
> progress. What we want here is to use freezable_schedule instead to hide
> the thread from the suspend.
> 
> While we are here also change schedule_timeout to freezable variant to
> prevent from spurious wakeups by suspend.
> 
> Reported-by: Hendrik Woltersdorf <hendrikw@arcor.de>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Hendrik was able to test the patch so feel free to add his Tested-by
-- 
Michal Hocko
SUSE Labs

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