[PATCH 0/4] allow freezing of tasks with netfs calls in flight

[PATCH 0/4] allow freezing of tasks with netfs calls in flight

[Jeff Layton]

We've had a number of reports recently of people with NFS and CIFS
mounts that were unable to suspend or hibernate their machines. Here
are a couple of Fedora bugs that illustrate the problem:

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

When it occurs the problem is pretty clear. We have a task that's
sleeping in the kernel in TASK_KILLABLE sleep, generally waiting
for a reply to come in. Often though, userspace has already taken
down the interface so that reply will never come. The process then
fails to freeze and the suspend fails.

This patch fixes this by allowing the TASK_KILLABLE sleeps in NFS and
CIFS to be awoken by the freezer and then to try to freeze. If a freeze
event does occur, then the code will treat it as if a schedule() has
already occured.

I've had a number of people test versions of this set and the results
have been positive. Part of the problem though is that there are
sometimes still unrelated problems with suspend/resume that seem to
cloud the results. As best we can tell though, this patchset does work
as advertised.

Jeff Layton (4):
  freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too
  cifs, freezer: add wait_event_freezekillable and have cifs use it
  sunrpc: make rpc_wait_bit_killable handle freeze events
  nfs: make TASK_KILLABLE sleeps attempt to freeze

 fs/cifs/transport.c     |    3 ++-
 fs/nfs/inode.c          |    4 +++-
 fs/nfs/nfs3proc.c       |    3 ++-
 fs/nfs/nfs4proc.c       |   12 ++++++++----
 fs/nfs/proc.c           |    3 ++-
 include/linux/freezer.h |   19 +++++++++++++++++--
 kernel/freezer.c        |    2 +-
 net/sunrpc/sched.c      |    4 +++-
 8 files changed, 38 insertions(+), 12 deletions(-)

-- 
1.7.6.2

[PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Jeff Layton]

TASK_KILLABLE is often used to put tasks to sleep for quite some time.
One of the most common uses is to put tasks to sleep while waiting for
replies from a server on a networked filesystem (such as CIFS or NFS).

Unfortunately, fake_signal_wake_up does not currently wake up tasks
that are sleeping in TASK_KILLABLE state. This means that even if the
code were in place to allow them to freeze while in this sleep, it
wouldn't work anyway.

This patch changes this function to wake tasks in this state as well.
This should be harmless -- if the code doing the sleeping doesn't have
handling to deal with freezer events, it should just go back to sleep.
If it does, then this will allow that code to do the right thing.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 kernel/freezer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 7b01de9..66a594e 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -67,7 +67,7 @@ static void fake_signal_wake_up(struct task_struct *p)
 	unsigned long flags;
 
 	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
+	signal_wake_up(p, 1);
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
 }
 
-- 
1.7.6.2

Re: [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Pavel Machek]

Hi!

> TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> One of the most common uses is to put tasks to sleep while waiting for
> replies from a server on a networked filesystem (such as CIFS or NFS).
> 
> Unfortunately, fake_signal_wake_up does not currently wake up tasks
> that are sleeping in TASK_KILLABLE state. This means that even if the
> code were in place to allow them to freeze while in this sleep, it
> wouldn't work anyway.
> 
> This patch changes this function to wake tasks in this state as well.
> This should be harmless -- if the code doing the sleeping doesn't have
> handling to deal with freezer events, it should just go back to sleep.

I'm pretty sure this will break something; but that does not mean it
is bad idea, just that it should be merged early and tested a lot.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Jeff Layton]

On Tue, 11 Oct 2011 08:18:48 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> 
> Hi!
> 
> > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> > One of the most common uses is to put tasks to sleep while waiting for
> > replies from a server on a networked filesystem (such as CIFS or NFS).
> > 
> > Unfortunately, fake_signal_wake_up does not currently wake up tasks
> > that are sleeping in TASK_KILLABLE state. This means that even if the
> > code were in place to allow them to freeze while in this sleep, it
> > wouldn't work anyway.
> > 
> > This patch changes this function to wake tasks in this state as well.
> > This should be harmless -- if the code doing the sleeping doesn't have
> > handling to deal with freezer events, it should just go back to sleep.
> 
> I'm pretty sure this will break something; but that does not mean it
> is bad idea, just that it should be merged early and tested a lot.
> 

FWIW, I looked at most of the places in the kernel that do
TASK_KILLABLE sleeps and they look like they'll handle this correctly.
The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
think it'll do the right thing too. I certainly could have missed
something though...

In any case, would you mind merging this via the linux-pm tree for 3.2?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Rafael J. Wysocki]

On Tuesday, October 11, 2011, Jeff Layton wrote:
> On Tue, 11 Oct 2011 08:18:48 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > 
> > Hi!
> > 
> > > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> > > One of the most common uses is to put tasks to sleep while waiting for
> > > replies from a server on a networked filesystem (such as CIFS or NFS).
> > > 
> > > Unfortunately, fake_signal_wake_up does not currently wake up tasks
> > > that are sleeping in TASK_KILLABLE state. This means that even if the
> > > code were in place to allow them to freeze while in this sleep, it
> > > wouldn't work anyway.
> > > 
> > > This patch changes this function to wake tasks in this state as well.
> > > This should be harmless -- if the code doing the sleeping doesn't have
> > > handling to deal with freezer events, it should just go back to sleep.
> > 
> > I'm pretty sure this will break something; but that does not mean it
> > is bad idea, just that it should be merged early and tested a lot.
> > 
> 
> FWIW, I looked at most of the places in the kernel that do
> TASK_KILLABLE sleeps and they look like they'll handle this correctly.
> The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
> think it'll do the right thing too. I certainly could have missed
> something though...
> 
> In any case, would you mind merging this via the linux-pm tree for 3.2?

I will push it for 3.2.

Thanks,
Rafael

Re: [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Jeff Layton]

On Tue, 11 Oct 2011 21:14:28 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Tuesday, October 11, 2011, Jeff Layton wrote:
> > On Tue, 11 Oct 2011 08:18:48 +0200
> > Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > 
> > > Hi!
> > > 
> > > > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> > > > One of the most common uses is to put tasks to sleep while waiting for
> > > > replies from a server on a networked filesystem (such as CIFS or NFS).
> > > > 
> > > > Unfortunately, fake_signal_wake_up does not currently wake up tasks
> > > > that are sleeping in TASK_KILLABLE state. This means that even if the
> > > > code were in place to allow them to freeze while in this sleep, it
> > > > wouldn't work anyway.
> > > > 
> > > > This patch changes this function to wake tasks in this state as well.
> > > > This should be harmless -- if the code doing the sleeping doesn't have
> > > > handling to deal with freezer events, it should just go back to sleep.
> > > 
> > > I'm pretty sure this will break something; but that does not mean it
> > > is bad idea, just that it should be merged early and tested a lot.
> > > 
> > 
> > FWIW, I looked at most of the places in the kernel that do
> > TASK_KILLABLE sleeps and they look like they'll handle this correctly.
> > The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
> > think it'll do the right thing too. I certainly could have missed
> > something though...
> > 
> > In any case, would you mind merging this via the linux-pm tree for 3.2?
> 
> I will push it for 3.2.
> 

Hi Rafael,

Trond asked if you would also be willing to push patches 3 and 4 in
this series for 3.2 as well [1]? Note that patch #4 got another revision so
we'll want to make sure that you get that one. I can resend the
nfs/sunrpc patches if that will help...

[1]: I think Steve F is going to push patch #2, so that one shouldn't
be an issue. 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Rafael J. Wysocki]

On Wednesday, October 26, 2011, Jeff Layton wrote:
> On Tue, 11 Oct 2011 21:14:28 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Tuesday, October 11, 2011, Jeff Layton wrote:
> > > On Tue, 11 Oct 2011 08:18:48 +0200
> > > Pavel Machek <pavel@ucw.cz> wrote:
> > > 
> > > > 
> > > > Hi!
> > > > 
> > > > > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> > > > > One of the most common uses is to put tasks to sleep while waiting for
> > > > > replies from a server on a networked filesystem (such as CIFS or NFS).
> > > > > 
> > > > > Unfortunately, fake_signal_wake_up does not currently wake up tasks
> > > > > that are sleeping in TASK_KILLABLE state. This means that even if the
> > > > > code were in place to allow them to freeze while in this sleep, it
> > > > > wouldn't work anyway.
> > > > > 
> > > > > This patch changes this function to wake tasks in this state as well.
> > > > > This should be harmless -- if the code doing the sleeping doesn't have
> > > > > handling to deal with freezer events, it should just go back to sleep.
> > > > 
> > > > I'm pretty sure this will break something; but that does not mean it
> > > > is bad idea, just that it should be merged early and tested a lot.
> > > > 
> > > 
> > > FWIW, I looked at most of the places in the kernel that do
> > > TASK_KILLABLE sleeps and they look like they'll handle this correctly.
> > > The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
> > > think it'll do the right thing too. I certainly could have missed
> > > something though...
> > > 
> > > In any case, would you mind merging this via the linux-pm tree for 3.2?
> > 
> > I will push it for 3.2.
> > 
> 
> Hi Rafael,
> 
> Trond asked if you would also be willing to push patches 3 and 4 in
> this series for 3.2 as well [1]? Note that patch #4 got another revision so
> we'll want to make sure that you get that one. I can resend the
> nfs/sunrpc patches if that will help...
> 
> [1]: I think Steve F is going to push patch #2, so that one shouldn't
> be an issue. 

Well, I've already sent my pull request.  I can keep these patches in my
tree for the next pull request, though (I'm sure there will be fixes against
3.2, so they will go along with those).

Thanks,
Rafael

Re: [linux-pm] [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Rafael J. Wysocki]

On Thursday, October 27, 2011, Rafael J. Wysocki wrote:
> On Wednesday, October 26, 2011, Jeff Layton wrote:
> > On Tue, 11 Oct 2011 21:14:28 +0200
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > On Tuesday, October 11, 2011, Jeff Layton wrote:
> > > > On Tue, 11 Oct 2011 08:18:48 +0200
> > > > Pavel Machek <pavel@ucw.cz> wrote:
> > > > 
> > > > > 
> > > > > Hi!
> > > > > 
> > > > > > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
> > > > > > One of the most common uses is to put tasks to sleep while waiting for
> > > > > > replies from a server on a networked filesystem (such as CIFS or NFS).
> > > > > > 
> > > > > > Unfortunately, fake_signal_wake_up does not currently wake up tasks
> > > > > > that are sleeping in TASK_KILLABLE state. This means that even if the
> > > > > > code were in place to allow them to freeze while in this sleep, it
> > > > > > wouldn't work anyway.
> > > > > > 
> > > > > > This patch changes this function to wake tasks in this state as well.
> > > > > > This should be harmless -- if the code doing the sleeping doesn't have
> > > > > > handling to deal with freezer events, it should just go back to sleep.
> > > > > 
> > > > > I'm pretty sure this will break something; but that does not mean it
> > > > > is bad idea, just that it should be merged early and tested a lot.
> > > > > 
> > > > 
> > > > FWIW, I looked at most of the places in the kernel that do
> > > > TASK_KILLABLE sleeps and they look like they'll handle this correctly.
> > > > The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
> > > > think it'll do the right thing too. I certainly could have missed
> > > > something though...
> > > > 
> > > > In any case, would you mind merging this via the linux-pm tree for 3.2?
> > > 
> > > I will push it for 3.2.
> > > 
> > 
> > Hi Rafael,
> > 
> > Trond asked if you would also be willing to push patches 3 and 4 in
> > this series for 3.2 as well [1]? Note that patch #4 got another revision so
> > we'll want to make sure that you get that one. I can resend the
> > nfs/sunrpc patches if that will help...
> > 
> > [1]: I think Steve F is going to push patch #2, so that one shouldn't
> > be an issue. 
> 
> Well, I've already sent my pull request.  I can keep these patches in my
> tree for the next pull request, though (I'm sure there will be fixes against
> 3.2, so they will go along with those).

BTW, do you have current versions handy?  Or hasn't they changed?

Rafael

Re: [linux-pm] [PATCH 1/4] freezer: make fake_signal_wake_up wake TASK_KILLABLE tasks too

[Steve French]

FYI - I included the cifs one "cifs, freezer: add
wait_event_freezekillable and have cifs use it" (and the corequisite
fix for the build break when freezer not enabled) in my recent cifs
merge request.

On Thu, Oct 27, 2011 at 3:22 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, October 27, 2011, Rafael J. Wysocki wrote:
>> On Wednesday, October 26, 2011, Jeff Layton wrote:
>> > On Tue, 11 Oct 2011 21:14:28 +0200
>> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>> >
>> > > On Tuesday, October 11, 2011, Jeff Layton wrote:
>> > > > On Tue, 11 Oct 2011 08:18:48 +0200
>> > > > Pavel Machek <pavel@ucw.cz> wrote:
>> > > >
>> > > > >
>> > > > > Hi!
>> > > > >
>> > > > > > TASK_KILLABLE is often used to put tasks to sleep for quite some time.
>> > > > > > One of the most common uses is to put tasks to sleep while waiting for
>> > > > > > replies from a server on a networked filesystem (such as CIFS or NFS).
>> > > > > >
>> > > > > > Unfortunately, fake_signal_wake_up does not currently wake up tasks
>> > > > > > that are sleeping in TASK_KILLABLE state. This means that even if the
>> > > > > > code were in place to allow them to freeze while in this sleep, it
>> > > > > > wouldn't work anyway.
>> > > > > >
>> > > > > > This patch changes this function to wake tasks in this state as well.
>> > > > > > This should be harmless -- if the code doing the sleeping doesn't have
>> > > > > > handling to deal with freezer events, it should just go back to sleep.
>> > > > >
>> > > > > I'm pretty sure this will break something; but that does not mean it
>> > > > > is bad idea, just that it should be merged early and tested a lot.
>> > > > >
>> > > >
>> > > > FWIW, I looked at most of the places in the kernel that do
>> > > > TASK_KILLABLE sleeps and they look like they'll handle this correctly.
>> > > > The main one I wasn't sure about was mem_cgroup_handle_oom(), but I
>> > > > think it'll do the right thing too. I certainly could have missed
>> > > > something though...
>> > > >
>> > > > In any case, would you mind merging this via the linux-pm tree for 3.2?
>> > >
>> > > I will push it for 3.2.
>> > >
>> >
>> > Hi Rafael,
>> >
>> > Trond asked if you would also be willing to push patches 3 and 4 in
>> > this series for 3.2 as well [1]? Note that patch #4 got another revision so
>> > we'll want to make sure that you get that one. I can resend the
>> > nfs/sunrpc patches if that will help...
>> >
>> > [1]: I think Steve F is going to push patch #2, so that one shouldn't
>> > be an issue.
>>
>> Well, I've already sent my pull request.  I can keep these patches in my
>> tree for the next pull request, though (I'm sure there will be fixes against
>> 3.2, so they will go along with those).
>
> BTW, do you have current versions handy?  Or hasn't they changed?
>
> Rafael
>



-- 
Thanks,

Steve

[PATCH 2/4] cifs, freezer: add wait_event_freezekillable and have cifs use it

[Jeff Layton]

CIFS currently uses wait_event_killable to put tasks to sleep while
they await replies from the server. That function though does not
allow the freezer to run. In many cases, the network interface may
be going down anyway, in which case the reply will never come. The
client then ends up blocking the computer from suspending.

Fix this by adding a new wait_event_freezable variant --
wait_event_freezekillable. The idea is to combine the behavior of
wait_event_killable and wait_event_freezable -- put the task to
sleep and only allow it to be awoken by fatal signals, but also
allow the freezer to do its job.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/transport.c     |    3 ++-
 include/linux/freezer.h |   19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 10ca6b2..791bc4f 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -26,6 +26,7 @@
 #include <linux/wait.h>
 #include <linux/net.h>
 #include <linux/delay.h>
+#include <linux/freezer.h>
 #include <asm/uaccess.h>
 #include <asm/processor.h>
 #include <linux/mempool.h>
@@ -324,7 +325,7 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
 {
 	int error;
 
-	error = wait_event_killable(server->response_q,
+	error = wait_event_freezekillable(server->response_q,
 				    midQ->midState != MID_REQUEST_SUBMITTED);
 	if (error < 0)
 		return -ERESTARTSYS;
diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..3672f73 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -134,10 +134,25 @@ static inline void set_freezable_with_signal(void)
 }
 
 /*
- * Freezer-friendly wrappers around wait_event_interruptible() and
- * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
+ * Freezer-friendly wrappers around wait_event_interruptible(),
+ * wait_event_killable() and wait_event_interruptible_timeout(), originally
+ * defined in <linux/wait.h>
  */
 
+#define wait_event_freezekillable(wq, condition)			\
+({									\
+	int __retval;							\
+	do {								\
+		__retval = wait_event_killable(wq, 			\
+				(condition) || freezing(current));	\
+		if (__retval && !freezing(current))			\
+			break;						\
+		else if (!(condition))					\
+			__retval = -ERESTARTSYS;			\
+	} while (try_to_freeze());					\
+	__retval;							\
+})
+
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-- 
1.7.6.2

Re: [PATCH 2/4] cifs, freezer: add wait_event_freezekillable and have cifs use it

[Steve French]

The general idea of the patch seems like a good idea to
me.  Assuming testing feedback was good from the problem
reporters, what tree would you want it merged from?

On Wed, Sep 28, 2011 at 6:52 AM, Jeff Layton <jlayton@redhat.com> wrote:
> CIFS currently uses wait_event_killable to put tasks to sleep while
> they await replies from the server. That function though does not
> allow the freezer to run. In many cases, the network interface may
> be going down anyway, in which case the reply will never come. The
> client then ends up blocking the computer from suspending.
>
> Fix this by adding a new wait_event_freezable variant --
> wait_event_freezekillable. The idea is to combine the behavior of
> wait_event_killable and wait_event_freezable -- put the task to
> sleep and only allow it to be awoken by fatal signals, but also
> allow the freezer to do its job.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/transport.c     |    3 ++-
>  include/linux/freezer.h |   19 +++++++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 10ca6b2..791bc4f 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -26,6 +26,7 @@
>  #include <linux/wait.h>
>  #include <linux/net.h>
>  #include <linux/delay.h>
> +#include <linux/freezer.h>
>  #include <asm/uaccess.h>
>  #include <asm/processor.h>
>  #include <linux/mempool.h>
> @@ -324,7 +325,7 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>  {
>        int error;
>
> -       error = wait_event_killable(server->response_q,
> +       error = wait_event_freezekillable(server->response_q,
>                                    midQ->midState != MID_REQUEST_SUBMITTED);
>        if (error < 0)
>                return -ERESTARTSYS;
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 1effc8b..3672f73 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -134,10 +134,25 @@ static inline void set_freezable_with_signal(void)
>  }
>
>  /*
> - * Freezer-friendly wrappers around wait_event_interruptible() and
> - * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
> + * Freezer-friendly wrappers around wait_event_interruptible(),
> + * wait_event_killable() and wait_event_interruptible_timeout(), originally
> + * defined in <linux/wait.h>
>  */
>
> +#define wait_event_freezekillable(wq, condition)                       \
> +({                                                                     \
> +       int __retval;                                                   \
> +       do {                                                            \
> +               __retval = wait_event_killable(wq,                      \
> +                               (condition) || freezing(current));      \
> +               if (__retval && !freezing(current))                     \
> +                       break;                                          \
> +               else if (!(condition))                                  \
> +                       __retval = -ERESTARTSYS;                        \
> +       } while (try_to_freeze());                                      \
> +       __retval;                                                       \
> +})
> +
>  #define wait_event_freezable(wq, condition)                            \
>  ({                                                                     \
>        int __retval;                                                   \
> --
> 1.7.6.2
>
>



-- 
Thanks,

Steve

Re: [PATCH 2/4] cifs, freezer: add wait_event_freezekillable and have cifs use it

[Jeff Layton]

On Wed, 28 Sep 2011 23:28:02 -0500
Steve French <smfrench@gmail.com> wrote:

> The general idea of the patch seems like a good idea to
> me.  Assuming testing feedback was good from the problem
> reporters, what tree would you want it merged from?
> 

There's the rub -- this requires a number of changes in different
areas. What I really need at this point is a verdict on patch #1. If
that looks OK, then that should probably go in via the one of the
linux-pm trees. Then patch #2 can probably go in via your tree and 3
and 4 can go in via Trond's.

However, none of this should go in unless #1 is ok. Make sense?

> On Wed, Sep 28, 2011 at 6:52 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > CIFS currently uses wait_event_killable to put tasks to sleep while
> > they await replies from the server. That function though does not
> > allow the freezer to run. In many cases, the network interface may
> > be going down anyway, in which case the reply will never come. The
> > client then ends up blocking the computer from suspending.
> >
> > Fix this by adding a new wait_event_freezable variant --
> > wait_event_freezekillable. The idea is to combine the behavior of
> > wait_event_killable and wait_event_freezable -- put the task to
> > sleep and only allow it to be awoken by fatal signals, but also
> > allow the freezer to do its job.
> >
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/transport.c     |    3 ++-
> >  include/linux/freezer.h |   19 +++++++++++++++++--
> >  2 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 10ca6b2..791bc4f 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/wait.h>
> >  #include <linux/net.h>
> >  #include <linux/delay.h>
> > +#include <linux/freezer.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/processor.h>
> >  #include <linux/mempool.h>
> > @@ -324,7 +325,7 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
> >  {
> >        int error;
> >
> > -       error = wait_event_killable(server->response_q,
> > +       error = wait_event_freezekillable(server->response_q,
> >                                    midQ->midState != MID_REQUEST_SUBMITTED);
> >        if (error < 0)
> >                return -ERESTARTSYS;
> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > index 1effc8b..3672f73 100644
> > --- a/include/linux/freezer.h
> > +++ b/include/linux/freezer.h
> > @@ -134,10 +134,25 @@ static inline void set_freezable_with_signal(void)
> >  }
> >
> >  /*
> > - * Freezer-friendly wrappers around wait_event_interruptible() and
> > - * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
> > + * Freezer-friendly wrappers around wait_event_interruptible(),
> > + * wait_event_killable() and wait_event_interruptible_timeout(), originally
> > + * defined in <linux/wait.h>
> >  */
> >
> > +#define wait_event_freezekillable(wq, condition)                       \
> > +({                                                                     \
> > +       int __retval;                                                   \
> > +       do {                                                            \
> > +               __retval = wait_event_killable(wq,                      \
> > +                               (condition) || freezing(current));      \
> > +               if (__retval && !freezing(current))                     \
> > +                       break;                                          \
> > +               else if (!(condition))                                  \
> > +                       __retval = -ERESTARTSYS;                        \
> > +       } while (try_to_freeze());                                      \
> > +       __retval;                                                       \
> > +})
> > +
> >  #define wait_event_freezable(wq, condition)                            \
> >  ({                                                                     \
> >        int __retval;                                                   \
> > --
> > 1.7.6.2
> >
> >
> 
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 2/4] cifs, freezer: add wait_event_freezekillable and have cifs use it

[Steve French]

On Thu, Sep 29, 2011 at 5:41 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 28 Sep 2011 23:28:02 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> The general idea of the patch seems like a good idea to
>> me.  Assuming testing feedback was good from the problem
>> reporters, what tree would you want it merged from?
>>
>
> There's the rub -- this requires a number of changes in different
> areas. What I really need at this point is a verdict on patch #1. If
> that looks OK, then that should probably go in via the one of the
> linux-pm trees. Then patch #2 can probably go in via your tree and 3
> and 4 can go in via Trond's.

Yes - makes sense, but wonder about ways to test the various
suspend/hibernate cases to make sure they work.

>
> However, none of this should go in unless #1 is ok. Make sense?
>
>> On Wed, Sep 28, 2011 at 6:52 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > CIFS currently uses wait_event_killable to put tasks to sleep while
>> > they await replies from the server. That function though does not
>> > allow the freezer to run. In many cases, the network interface may
>> > be going down anyway, in which case the reply will never come. The
>> > client then ends up blocking the computer from suspending.
>> >
>> > Fix this by adding a new wait_event_freezable variant --
>> > wait_event_freezekillable. The idea is to combine the behavior of
>> > wait_event_killable and wait_event_freezable -- put the task to
>> > sleep and only allow it to be awoken by fatal signals, but also
>> > allow the freezer to do its job.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> > ---
>> >  fs/cifs/transport.c     |    3 ++-
>> >  include/linux/freezer.h |   19 +++++++++++++++++--
>> >  2 files changed, 19 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> > index 10ca6b2..791bc4f 100644
>> > --- a/fs/cifs/transport.c
>> > +++ b/fs/cifs/transport.c
>> > @@ -26,6 +26,7 @@
>> >  #include <linux/wait.h>
>> >  #include <linux/net.h>
>> >  #include <linux/delay.h>
>> > +#include <linux/freezer.h>
>> >  #include <asm/uaccess.h>
>> >  #include <asm/processor.h>
>> >  #include <linux/mempool.h>
>> > @@ -324,7 +325,7 @@ wait_for_response(struct TCP_Server_Info *server, struct mid_q_entry *midQ)
>> >  {
>> >        int error;
>> >
>> > -       error = wait_event_killable(server->response_q,
>> > +       error = wait_event_freezekillable(server->response_q,
>> >                                    midQ->midState != MID_REQUEST_SUBMITTED);
>> >        if (error < 0)
>> >                return -ERESTARTSYS;
>> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
>> > index 1effc8b..3672f73 100644
>> > --- a/include/linux/freezer.h
>> > +++ b/include/linux/freezer.h
>> > @@ -134,10 +134,25 @@ static inline void set_freezable_with_signal(void)
>> >  }
>> >
>> >  /*
>> > - * Freezer-friendly wrappers around wait_event_interruptible() and
>> > - * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
>> > + * Freezer-friendly wrappers around wait_event_interruptible(),
>> > + * wait_event_killable() and wait_event_interruptible_timeout(), originally
>> > + * defined in <linux/wait.h>
>> >  */
>> >
>> > +#define wait_event_freezekillable(wq, condition)                       \
>> > +({                                                                     \
>> > +       int __retval;                                                   \
>> > +       do {                                                            \
>> > +               __retval = wait_event_killable(wq,                      \
>> > +                               (condition) || freezing(current));      \
>> > +               if (__retval && !freezing(current))                     \
>> > +                       break;                                          \
>> > +               else if (!(condition))                                  \
>> > +                       __retval = -ERESTARTSYS;                        \
>> > +       } while (try_to_freeze());                                      \
>> > +       __retval;                                                       \
>> > +})
>> > +
>> >  #define wait_event_freezable(wq, condition)                            \
>> >  ({                                                                     \
>> >        int __retval;                                                   \
>> > --
>> > 1.7.6.2
>> >
>> >
>>
>>
>>
>
>
> --
> Jeff Layton <jlayton@redhat.com>
>



-- 
Thanks,

Steve

Re: [PATCH 2/4] cifs, freezer: add wait_event_freezekillable and have cifs use it

[Jeff Layton]

On Thu, 29 Sep 2011 11:39:53 -0500
Steve French <smfrench@gmail.com> wrote:

> On Thu, Sep 29, 2011 at 5:41 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Wed, 28 Sep 2011 23:28:02 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> The general idea of the patch seems like a good idea to
> >> me.  Assuming testing feedback was good from the problem
> >> reporters, what tree would you want it merged from?
> >>
> >
> > There's the rub -- this requires a number of changes in different
> > areas. What I really need at this point is a verdict on patch #1. If
> > that looks OK, then that should probably go in via the one of the
> > linux-pm trees. Then patch #2 can probably go in via your tree and 3
> > and 4 can go in via Trond's.
> 
> Yes - makes sense, but wonder about ways to test the various
> suspend/hibernate cases to make sure they work.
> 

I think this set is unlikely to hurt anything (assuming of course that
waking up TASK_KILLABLE tasks on suspend events is OK). It's possible
that there are other places that need to be patched in the same way,
but this seems to cover the main pain points that people have reported.

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 3/4] 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>
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.2

Re: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle freeze events

[Pavel Machek]

On Wed 2011-09-28 07:52:40, Jeff Layton wrote:
> 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.

Will the RPC protocols used handle that correctly? What will happen
during resume?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Tue, 11 Oct 2011 08:19:23 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Wed 2011-09-28 07:52:40, Jeff Layton wrote:
> > 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.
> 
> Will the RPC protocols used handle that correctly? What will happen
> during resume?
> 

That depends on the state of the socket during resume. If the
suspend/resume is quick enough, then the socket may still be connected,
or if we're using UDP then we might just get the reply and carry on
successfully. Otherwise, the call will eventually time out, or will be
cancelled when the kernel finds that the socket has been closed.

Either way, this should do the right thing.

-- 
Jeff Layton <jlayton@redhat.com>

RE: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle freeze events

[Myklebust, Trond]

> -----Original Message-----
> From: Jeff Layton [mailto:jlayton@redhat.com]
> Sent: Tuesday, October 11, 2011 6:12 AM
> To: Pavel Machek
> Cc: Myklebust, Trond; smfrench@gmail.com; rjw@sisk.pl; linux-
> pm@lists.linux-foundation.org; linux-cifs@vger.kernel.org; linux-
> nfs@vger.kernel.org; john@Calva.COM; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle
freeze
> events
> 
> On Tue, 11 Oct 2011 08:19:23 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Wed 2011-09-28 07:52:40, Jeff Layton wrote:
> > > 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.
> >
> > Will the RPC protocols used handle that correctly? What will happen
> > during resume?
> >
> 
> That depends on the state of the socket during resume. If the
> suspend/resume is quick enough, then the socket may still be
connected, or
> if we're using UDP then we might just get the reply and carry on
successfully.
> Otherwise, the call will eventually time out, or will be cancelled
when the
> kernel finds that the socket has been closed.
> 
> Either way, this should do the right thing.

Well... The problem when this sort of thing happens is with the replay
cache. If the RPC in question was a mkdir, for instance, then replaying
the RPC call when you wake up can be problematic because chances are
that the server will have forgotten who created the directory, and so
will reply with EEXIST instead of OK.
However this is a generic problem when the client is unable to talk to
the server for a while, and is not particular to suspend.

Cheers
  Trond

Re: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Tue, 11 Oct 2011 05:52:57 -0700
"Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> > -----Original Message-----
> > From: Jeff Layton [mailto:jlayton@redhat.com]
> > Sent: Tuesday, October 11, 2011 6:12 AM
> > To: Pavel Machek
> > Cc: Myklebust, Trond; smfrench@gmail.com; rjw@sisk.pl; linux-
> > pm@lists.linux-foundation.org; linux-cifs@vger.kernel.org; linux-
> > nfs@vger.kernel.org; john@Calva.COM; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] sunrpc: make rpc_wait_bit_killable handle
> freeze
> > events
> > 
> > On Tue, 11 Oct 2011 08:19:23 +0200
> > Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > On Wed 2011-09-28 07:52:40, Jeff Layton wrote:
> > > > 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.
> > >
> > > Will the RPC protocols used handle that correctly? What will happen
> > > during resume?
> > >
> > 
> > That depends on the state of the socket during resume. If the
> > suspend/resume is quick enough, then the socket may still be
> connected, or
> > if we're using UDP then we might just get the reply and carry on
> successfully.
> > Otherwise, the call will eventually time out, or will be cancelled
> when the
> > kernel finds that the socket has been closed.
> > 
> > Either way, this should do the right thing.
> 
> Well... The problem when this sort of thing happens is with the replay
> cache. If the RPC in question was a mkdir, for instance, then replaying
> the RPC call when you wake up can be problematic because chances are
> that the server will have forgotten who created the directory, and so
> will reply with EEXIST instead of OK.
> However this is a generic problem when the client is unable to talk to
> the server for a while, and is not particular to suspend.
> 

Yeah, that's always a problem. You can hit the same thing if you just
unplug the cable from the network interface at an inopportune time.

I think this patch is really the best we can do under those
circumstances. Eventually we'll all be on 4.1 with sessions and this
problem will go away, right? ;)

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 4/4] 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 there are
RPC's pending on the wire.

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index fe12037..b7c4301 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..ee64bf3 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -32,7 +32,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 4700fae..12f92dd 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -244,10 +244,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;
 }
 
@@ -3970,8 +3972,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..e3f03d1 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -59,7 +59,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.2

[PATCH 4/4] nfs: make TASK_KILLABLE sleeps attempt to freeze (try #2)

[Jeff Layton]

Somehow I ended up sending out an earlier version of this patch instead
of the one actually tested. Here's the correct one.

Allow the TASK_KILLABLE sleeps in NFS 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>
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 fe12037..b7c4301 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 4700fae..86427d8 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"
@@ -244,10 +245,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;
 }
 
@@ -3970,8 +3973,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

Re: [PATCH 0/4] allow freezing of tasks with netfs calls in flight

[Pavel Machek]

Hi!

> We've had a number of reports recently of people with NFS and CIFS
> mounts that were unable to suspend or hibernate their machines. Here
> are a couple of Fedora bugs that illustrate the problem:
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=712088
>     https://bugzilla.redhat.com/show_bug.cgi?id=717735
> 
> When it occurs the problem is pretty clear. We have a task that's
> sleeping in the kernel in TASK_KILLABLE sleep, generally waiting
> for a reply to come in. Often though, userspace has already taken
> down the interface so that reply will never come. The process then
> fails to freeze and the suspend fails.

Userspace should not take interface down for suspend (*). Why do that?


> This patch fixes this by allowing the TASK_KILLABLE sleeps in NFS and
> CIFS to be awoken by the freezer and then to try to freeze. If a freeze
> event does occur, then the code will treat it as if a schedule() has
> already occured.

Looks like good idea...

(*) unless absolutely neccessary. openvpn?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/4] allow freezing of tasks with netfs calls in flight

[Jeff Layton]

On Tue, 11 Oct 2011 08:18:19 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > We've had a number of reports recently of people with NFS and CIFS
> > mounts that were unable to suspend or hibernate their machines. Here
> > are a couple of Fedora bugs that illustrate the problem:
> > 
> >     https://bugzilla.redhat.com/show_bug.cgi?id=712088
> >     https://bugzilla.redhat.com/show_bug.cgi?id=717735
> > 
> > When it occurs the problem is pretty clear. We have a task that's
> > sleeping in the kernel in TASK_KILLABLE sleep, generally waiting
> > for a reply to come in. Often though, userspace has already taken
> > down the interface so that reply will never come. The process then
> > fails to freeze and the suspend fails.
> 
> Userspace should not take interface down for suspend (*). Why do that?
> 

I suspect that NetworkManager does this to try and allow for the case
where someone suspends their laptop and then wanders off to another
network and then resumes. Either way, we still want to allow suspend
and hibernate to work regardless of what userspace does during the
process.

> > This patch fixes this by allowing the TASK_KILLABLE sleeps in NFS and
> > CIFS to be awoken by the freezer and then to try to freeze. If a freeze
> > event does occur, then the code will treat it as if a schedule() has
> > already occured.
> 
> Looks like good idea...
> 
> (*) unless absolutely neccessary. openvpn?
> 

Thanks for looking.
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/4] allow freezing of tasks with netfs calls in flight

[Rafael J. Wysocki]

On Tuesday, October 11, 2011, Jeff Layton wrote:
> On Tue, 11 Oct 2011 08:18:19 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > We've had a number of reports recently of people with NFS and CIFS
> > > mounts that were unable to suspend or hibernate their machines. Here
> > > are a couple of Fedora bugs that illustrate the problem:
> > > 
> > >     https://bugzilla.redhat.com/show_bug.cgi?id=712088
> > >     https://bugzilla.redhat.com/show_bug.cgi?id=717735
> > > 
> > > When it occurs the problem is pretty clear. We have a task that's
> > > sleeping in the kernel in TASK_KILLABLE sleep, generally waiting
> > > for a reply to come in. Often though, userspace has already taken
> > > down the interface so that reply will never come. The process then
> > > fails to freeze and the suspend fails.
> > 
> > Userspace should not take interface down for suspend (*). Why do that?
> > 
> 
> I suspect that NetworkManager does this to try and allow for the case
> where someone suspends their laptop and then wanders off to another
> network and then resumes.

That's correct.

> Either way, we still want to allow suspend and hibernate to work regardless
> of what userspace does during the process.

Very true. :-)

Thanks,
Rafael