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

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

[Jeff Layton]

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

Tested-by: John Hughes <john@Calva.COM>
Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/sched.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index d12ffa5..09bb64e 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,8 @@ static int rpc_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	if (!try_to_freeze())
+		schedule();
 	return 0;
 }
 
-- 
1.7.6.4

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

[Jeff Layton]

Allow the TASK_KILLABLE sleeps in NFS layer to respect the freezer. This
should allow suspend and hibernate events to occur, even when the client
is looping on an EJUKEBOX or NFS4ERR_DELAY sort of error.

Tested-by: John Hughes <john@Calva.COM>
Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/inode.c    |    4 +++-
 fs/nfs/nfs3proc.c |    4 +++-
 fs/nfs/nfs4proc.c |   13 +++++++++----
 fs/nfs/proc.c     |    4 +++-
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 4dc6d07..4b91e24 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,8 @@ int nfs_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	if (!try_to_freeze())
+		schedule();
 	return 0;
 }
 
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 85f1690..5354219 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,8 @@ 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);
+		if (!try_to_freeze())
+			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 d2ae413..085eb8a 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,10 +242,12 @@ 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);
+	if (!try_to_freeze()) {
+		schedule_timeout_killable(*timeout);
+		*timeout <<= 1;
+	}
 	if (fatal_signal_pending(current))
 		res = -ERESTARTSYS;
-	*timeout <<= 1;
 	return res;
 }
 
@@ -3951,8 +3954,10 @@ 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);
-	timeout <<= 1;
+	if (!try_to_freeze()) {
+		schedule_timeout_killable(timeout);
+		timeout <<= 1;
+	}
 	if (timeout > NFS4_LOCK_MAXTIMEOUT)
 		return NFS4_LOCK_MAXTIMEOUT;
 	return timeout;
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index ac40b85..0b70e85 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,8 @@ 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);
+		if (!try_to_freeze())
+			schedule_timeout_killable(NFS_JUKEBOX_RETRY_TIME);
 		res = -ERESTARTSYS;
 	} while (!fatal_signal_pending(current));
 	return res;
-- 
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

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>