[PATCH 0/2] nfs/sunrpc: allow freezing of tasks with NFS calls in flight

[PATCH 0/2] nfs/sunrpc: allow freezing of tasks with NFS calls in flight

[Jeff Layton]

This patchset is a second (third?) attempt at fixing the issues with
suspending a machine that has an active NFS mount.

The bug reported against Fedora is here:

    https://bugzilla.redhat.com/show_bug.cgi?id=717735

This patchset is a bit different from the original one. Instead of
attempting to make TASK_KILLABLE sleeps wake up when the freezer runs,
this one makes the freezer skip those processes when it runs.

This is the method that Tejun, Rafael, and Oleg have recommended for
now. Long term, it might make more sense to add a new TASK_WAKEFREEZE
flag or something and awake tasks based on that, and have them then
call try_to_freeze() when awoken. For now though this seems to work
as expected.

So far, results with this patchset have been good, but it seems like
it's a bit late for 3.2. I'd like to see this go in early for 3.3,
probably via Rafael's tree.

Jeff Layton (2):
  sunrpc: make rpc_wait_bit_killable handle freeze events
  nfs: make TASK_KILLABLE sleeps attempt to freeze

 fs/nfs/inode.c          |    3 ++-
 fs/nfs/nfs3proc.c       |    3 ++-
 fs/nfs/nfs4proc.c       |    5 +++--
 fs/nfs/proc.c           |    3 ++-
 include/linux/freezer.h |   22 ++++++++++++++++++++++
 net/sunrpc/sched.c      |    3 ++-
 6 files changed, 33 insertions(+), 6 deletions(-)

-- 
1.7.6.4

[PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
layer. This should allow suspend and hibernate events to proceed, even
when there are RPC's pending on the wire.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/freezer.h |   12 ++++++++++++
 net/sunrpc/sched.c      |    3 ++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index a5386e3..8b60dd0 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
 }
 
 /*
+ * Freezer-friendly macro around schedule() in the kernel.
+ */
+#define freezable_schedule()						\
+({									\
+	freezer_do_not_count();						\
+	schedule();							\
+	freezer_count();						\
+})
+
+/*
  * Freezer-friendly wrappers around wait_event_interruptible(),
  * wait_event_killable() and wait_event_interruptible_timeout(), originally
  * defined in <linux/wait.h>
@@ -194,6 +204,8 @@ static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
 static inline void set_freezable_with_signal(void) {}
 
+#define freezable_schedule()  schedule()
+
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
 
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index d12ffa5..5317b93 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -18,6 +18,7 @@
 #include <linux/smp.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 
 #include <linux/sunrpc/clnt.h>
 
@@ -231,7 +232,7 @@ static int rpc_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	freezable_schedule();
 	return 0;
 }
 
-- 
1.7.6.4

[PATCH 2/2] nfs: make TASK_KILLABLE sleeps attempt to freeze

[Jeff Layton]

Wrap the TASK_KILLABLE sleeps in NFS layer in freezer_do_not_count and
freezer_count calls. This allows the freezer to skip these processes
when they are sleeping while looping on EJUKEBOX or NFS4ERR_DELAY sorts
of errors.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/inode.c          |    3 ++-
 fs/nfs/nfs3proc.c       |    3 ++-
 fs/nfs/nfs4proc.c       |    5 +++--
 fs/nfs/proc.c           |    3 ++-
 include/linux/freezer.h |   12 +++++++++++-
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c07a55a..f36543a 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -38,6 +38,7 @@
 #include <linux/nfs_xdr.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
+#include <linux/freezer.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>
@@ -77,7 +78,7 @@ int nfs_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	freezable_schedule();
 	return 0;
 }
 
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 85f1690..cc1d1e1 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -17,6 +17,7 @@
 #include <linux/nfs_page.h>
 #include <linux/lockd/bind.h>
 #include <linux/nfs_mount.h>
+#include <linux/freezer.h>
 
 #include "iostat.h"
 #include "internal.h"
@@ -32,7 +33,7 @@ nfs3_rpc_wrapper(struct rpc_clnt *clnt, struct rpc_message *msg, int flags)
 		res = rpc_call_sync(clnt, msg, flags);
 		if (res != -EJUKEBOX && res != -EKEYEXPIRED)
 			break;
-		schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
+		freezable_schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
 		res = -ERESTARTSYS;
 	} while (!fatal_signal_pending(current));
 	return res;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b60fddf..988a0e1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -53,6 +53,7 @@
 #include <linux/sunrpc/bc_xprt.h>
 #include <linux/xattr.h>
 #include <linux/utsname.h>
+#include <linux/freezer.h>
 
 #include "nfs4_fs.h"
 #include "delegation.h"
@@ -241,7 +242,7 @@ static int nfs4_delay(struct rpc_clnt *clnt, long *timeout)
 		*timeout = NFS4_POLL_RETRY_MIN;
 	if (*timeout > NFS4_POLL_RETRY_MAX)
 		*timeout = NFS4_POLL_RETRY_MAX;
-	schedule_timeout_killable(*timeout);
+	freezable_schedule_timeout_killable(*timeout);
 	if (fatal_signal_pending(current))
 		res = -ERESTARTSYS;
 	*timeout <<= 1;
@@ -3951,7 +3952,7 @@ int nfs4_proc_delegreturn(struct inode *inode, struct rpc_cred *cred, const nfs4
 static unsigned long
 nfs4_set_lock_task_retry(unsigned long timeout)
 {
-	schedule_timeout_killable(timeout);
+	freezable_schedule_timeout_killable(timeout);
 	timeout <<= 1;
 	if (timeout > NFS4_LOCK_MAXTIMEOUT)
 		return NFS4_LOCK_MAXTIMEOUT;
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index ac40b85..4dda8b5 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -41,6 +41,7 @@
 #include <linux/nfs_fs.h>
 #include <linux/nfs_page.h>
 #include <linux/lockd/bind.h>
+#include <linux/freezer.h>
 #include "internal.h"
 
 #define NFSDBG_FACILITY		NFSDBG_PROC
@@ -59,7 +60,7 @@ nfs_rpc_wrapper(struct rpc_clnt *clnt, struct rpc_message *msg, int flags)
 		res = rpc_call_sync(clnt, msg, flags);
 		if (res != -EKEYEXPIRED)
 			break;
-		schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
+		freezable_schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
 		res = -ERESTARTSYS;
 	} while (!fatal_signal_pending(current));
 	return res;
diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 8b60dd0..235cb05 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -135,7 +135,7 @@ static inline void set_freezable_with_signal(void)
 }
 
 /*
- * Freezer-friendly macro around schedule() in the kernel.
+ * Freezer-friendly macros around some sleeping functions.
  */
 #define freezable_schedule()						\
 ({									\
@@ -144,6 +144,13 @@ static inline void set_freezable_with_signal(void)
 	freezer_count();						\
 })
 
+#define freezable_schedule_timeout_killable(timeout)			\
+({									\
+	freezer_do_not_count();						\
+	schedule_timeout_killable(timeout);				\
+	freezer_count();						\
+})
+
 /*
  * Freezer-friendly wrappers around wait_event_interruptible(),
  * wait_event_killable() and wait_event_interruptible_timeout(), originally
@@ -206,6 +213,9 @@ static inline void set_freezable_with_signal(void) {}
 
 #define freezable_schedule()  schedule()
 
+#define freezable_schedule_timeout_killable(timeout)			\
+	schedule_timeout_killable(timeout)
+
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
 
-- 
1.7.6.4

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
> layer. This should allow suspend and hibernate events to proceed, even
> when there are RPC's pending on the wire.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  include/linux/freezer.h |   12 ++++++++++++
>  net/sunrpc/sched.c      |    3 ++-
>  2 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index a5386e3..8b60dd0 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
>  }
>  
>  /*
> + * Freezer-friendly macro around schedule() in the kernel.
> + */
> +#define freezable_schedule()						\
> +({									\
> +	freezer_do_not_count();						\
> +	schedule();							\
> +	freezer_count();						\

Don't we want try_to_freeze() here?  If the freezer thought the task
was safe to skip, it better stay inside freezer until the freezing
condition is lifted.

Thanks.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Mon, 21 Nov 2011 09:56:44 -0800
Tejun Heo <tj@kernel.org> wrote:

> On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> > Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
> > layer. This should allow suspend and hibernate events to proceed, even
> > when there are RPC's pending on the wire.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  include/linux/freezer.h |   12 ++++++++++++
> >  net/sunrpc/sched.c      |    3 ++-
> >  2 files changed, 14 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > index a5386e3..8b60dd0 100644
> > --- a/include/linux/freezer.h
> > +++ b/include/linux/freezer.h
> > @@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
> >  }
> >  
> >  /*
> > + * Freezer-friendly macro around schedule() in the kernel.
> > + */
> > +#define freezable_schedule()						\
> > +({									\
> > +	freezer_do_not_count();						\
> > +	schedule();							\
> > +	freezer_count();						\
> 
> Don't we want try_to_freeze() here?  If the freezer thought the task
> was safe to skip, it better stay inside freezer until the freezing
> condition is lifted.
> 
> Thanks.
> 

I suppose you're suggesting something like this?

	freezer_do_not_count();
	if (!try_to_freeze())
		schedule();
	freezer_count();

Hmm, so I guess the concern is that we'd race in such a way that
freezer_count() ends up running after the freezer has already skipped
the task?

If so, that seems just as racy...nothing guarantees that the freeze
event occurs before the try_to_freeze call...

The freezer is not really my forte', but I have to say that the whole
do_not_count scheme seems "sketchy". Would we not be better off with
something closer to the original method that I proposed, along with a
new TASK_WAKEFREEZE state bit? Processes that are sleeping in
uninterruptible sleep that are able to deal with freezer events could
set that bit and fake_signal_wake_up could be changed to also wake
processes with that bit set.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

Hello, Jeff.

On Mon, Nov 21, 2011 at 03:36:15PM -0500, Jeff Layton wrote:
> I suppose you're suggesting something like this?
> 
> 	freezer_do_not_count();
> 	if (!try_to_freeze())
> 		schedule();
> 	freezer_count();

No,

	freezer_do_not_count();
	schedule();
	freezer_count();
	try_to_freeze();

freezer_count() may make %current eligible for freezing again, so it
has to check whether freezing condition is pending afterwards.

> The freezer is not really my forte', but I have to say that the whole
> do_not_count scheme seems "sketchy". Would we not be better off with
> something closer to the original method that I proposed, along with a
> new TASK_WAKEFREEZE state bit? Processes that are sleeping in
> uninterruptible sleep that are able to deal with freezer events could
> set that bit and fake_signal_wake_up could be changed to also wake
> processes with that bit set.

AFAICS, the above should be race-free w/ the pending freezer update.

  http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=refs/heads/pm-freezer

Thank you.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:41:41PM -0800, Tejun Heo wrote:
> Hello, Jeff.
> 
> On Mon, Nov 21, 2011 at 03:36:15PM -0500, Jeff Layton wrote:
> > I suppose you're suggesting something like this?
> > 
> > 	freezer_do_not_count();
> > 	if (!try_to_freeze())
> > 		schedule();
> > 	freezer_count();
> 
> No,
> 
> 	freezer_do_not_count();
> 	schedule();
> 	freezer_count();
> 	try_to_freeze();
> 
> freezer_count() may make %current eligible for freezing again, so it
> has to check whether freezing condition is pending afterwards.

Ooh, freezer_count() already has that.  Heh, call me an idiot.  The
proposed code seems correct then.

Thanks.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
>  /*
> + * Freezer-friendly macro around schedule() in the kernel.
> + */
> +#define freezable_schedule()						\
> +({									\
> +	freezer_do_not_count();						\
> +	schedule();							\
> +	freezer_count();						\
> +})

So, yes, this seems correct to me but I'm not really sure about the
naming.  Given what freezable means for other interfaces, this is a
bit confusing to me but that could be because I know how each is
implemented.  Anyone with a better name?

Also, this definitely can use a lot more detailed comment.  What it's
supposed to do, how it's supposed to be used and so on.

Thank you.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Mon, 21 Nov 2011 12:46:20 -0800
Tejun Heo <tj@kernel.org> wrote:

> On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> >  /*
> > + * Freezer-friendly macro around schedule() in the kernel.
> > + */
> > +#define freezable_schedule()						\
> > +({									\
> > +	freezer_do_not_count();						\
> > +	schedule();							\
> > +	freezer_count();						\
> > +})
> 
> So, yes, this seems correct to me but I'm not really sure about the
> naming.  Given what freezable means for other interfaces, this is a
> bit confusing to me but that could be because I know how each is
> implemented.  Anyone with a better name?
> 
> Also, this definitely can use a lot more detailed comment.  What it's
> supposed to do, how it's supposed to be used and so on.
> 
> Thank you.
> 

That makes sense. I'll plan to flesh out the comments in a respin.

Yeah, naming these things is rather difficult -- too many "*able"
adjectives. I'm quite open to better suggestions for the name (and for
wait_event_freezekillable too).

-- 
Jeff Layton <jlayton@redhat.com>