[PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

Hi
We experience the following problem when our board tries to suspend:

Jun  7 18:32:21 constin kernel: [ 2085.350000] Stopping tasks ... 
Jun  7 18:32:21 constin kernel: [ 2105.410000] Stopping kernel threads timed 
out after 20 seconds (1 tasks refusing to freeze):
Jun  7 18:32:21 constin kernel: [ 2105.410000]  jffs2_gcd_mtd3
Jun  7 18:32:21 constin kernel: [ 2105.410000] Restarting tasks ... <4> 
Strange, jffs2_gcd_mtd3 not stopped
Jun  7 18:32:21 constin kernel: [ 2105.440000] done.

The problem seems to happen, when suspend kicks in, after the call to 
try_to_freeze and before the while loop in jffs2_garbage_collect_thread.

The pending freeze will cause signal_pending to return true until the 
attempted suspend is over and thereby the while loop won't terminate until 
the suspend times out.

This patch will call try_to_freeze from within the while loop and adds an 
extra check for jffs2_thread_should_wake before we actually do the GC to 
avoid calling jffs2_garbage_collect_pass at every wakeup.

If there are no objections, please apply and push to mainline.

Signed-off-by: Uli Luckas <u.luckas@road.de>

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 888f236..f953824 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -96,9 +96,6 @@ static int jffs2_garbage_collect_thread(void *_c)
                        schedule();
                }
 
-               if (try_to_freeze())
-                       continue;
-
                /* This thread is purely an optimisation. But if it runs when
                   other things could be running, it actually makes things a
                   lot worse. Use yield() and put it at the back of the runqueue
@@ -113,6 +110,9 @@ static int jffs2_garbage_collect_thread(void *_c)
                        siginfo_t info;
                        unsigned long signr;
 
+                       if (try_to_freeze())
+                               continue;
+
                        signr = dequeue_signal_lock(current, &current->blocked, &info);
 
                        switch(signr) {
@@ -137,7 +137,8 @@ static int jffs2_garbage_collect_thread(void *_c)
                disallow_signal(SIGHUP);
 
                D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-               if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+               if (   jffs2_thread_should_wake(c) 
+                   && jffs2_garbage_collect_pass(c) == -ENOSPC) {
                        printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
                        goto die;
                }

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

Hi,
is there anything in the way of apllying this patch?

Regards
Uli


On Thursday, 14. June 2007, Uli Luckas wrote:
> Hi
> We experience the following problem when our board tries to suspend:
>
> Jun  7 18:32:21 constin kernel: [ 2085.350000] Stopping tasks ...
> Jun  7 18:32:21 constin kernel: [ 2105.410000] Stopping kernel threads
> timed out after 20 seconds (1 tasks refusing to freeze):
> Jun  7 18:32:21 constin kernel: [ 2105.410000]  jffs2_gcd_mtd3
> Jun  7 18:32:21 constin kernel: [ 2105.410000] Restarting tasks ... <4>
> Strange, jffs2_gcd_mtd3 not stopped
> Jun  7 18:32:21 constin kernel: [ 2105.440000] done.
>
> The problem seems to happen, when suspend kicks in, after the call to
> try_to_freeze and before the while loop in jffs2_garbage_collect_thread.
>
> The pending freeze will cause signal_pending to return true until the
> attempted suspend is over and thereby the while loop won't terminate until
> the suspend times out.
>
> This patch will call try_to_freeze from within the while loop and adds an
> extra check for jffs2_thread_should_wake before we actually do the GC to
> avoid calling jffs2_garbage_collect_pass at every wakeup.
>
> If there are no objections, please apply and push to mainline.
>
> Signed-off-by: Uli Luckas <u.luckas@road.de>
>
> diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
> index 888f236..f953824 100644
> --- a/fs/jffs2/background.c
> +++ b/fs/jffs2/background.c
> @@ -96,9 +96,6 @@ static int jffs2_garbage_collect_thread(void *_c)
>                         schedule();
>                 }
>  
> -               if (try_to_freeze())
> -                       continue;
> -
>                 /* This thread is purely an optimisation. But if it runs
> when other things could be running, it actually makes things a lot worse.
> Use yield() and put it at the back of the runqueue @@ -113,6 +110,9 @@
> static int jffs2_garbage_collect_thread(void *_c) siginfo_t info;
>                         unsigned long signr;
>  
> +                       if (try_to_freeze())
> +                               continue;
> +
>                         signr = dequeue_signal_lock(current,
> &current->blocked, &info); 
>                         switch(signr) {
> @@ -137,7 +137,8 @@ static int jffs2_garbage_collect_thread(void *_c)
>                 disallow_signal(SIGHUP);
>  
>                 D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread():
> pass\n")); -               if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
> +               if (   jffs2_thread_should_wake(c)
> +                   && jffs2_garbage_collect_pass(c) == -ENOSPC) {
>                         printk(KERN_NOTICE "No space for garbage
> collection. Aborting GC thread\n"); goto die;
>                 }



-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Tuesday, 19. June 2007, Uli Luckas wrote:
> Hi,
> is there anything in the way of apllying this patch?
>
Please, can someone tell me what it takes to get a jffs2 bug fix into the 
kernel? Or even reviewed? This patch should even get into 2.6.22. Am I doing 
something wrong here?

Regards
Uli

> Regards
> Uli
>
> On Thursday, 14. June 2007, Uli Luckas wrote:
> > Hi
> > We experience the following problem when our board tries to suspend:
> >
> > Jun  7 18:32:21 constin kernel: [ 2085.350000] Stopping tasks ...
> > Jun  7 18:32:21 constin kernel: [ 2105.410000] Stopping kernel threads
> > timed out after 20 seconds (1 tasks refusing to freeze):
> > Jun  7 18:32:21 constin kernel: [ 2105.410000]  jffs2_gcd_mtd3
> > Jun  7 18:32:21 constin kernel: [ 2105.410000] Restarting tasks ... <4>
> > Strange, jffs2_gcd_mtd3 not stopped
> > Jun  7 18:32:21 constin kernel: [ 2105.440000] done.
> >
> > The problem seems to happen, when suspend kicks in, after the call to
> > try_to_freeze and before the while loop in jffs2_garbage_collect_thread.
> >
> > The pending freeze will cause signal_pending to return true until the
> > attempted suspend is over and thereby the while loop won't terminate
> > until the suspend times out.
> >
> > This patch will call try_to_freeze from within the while loop and adds an
> > extra check for jffs2_thread_should_wake before we actually do the GC to
> > avoid calling jffs2_garbage_collect_pass at every wakeup.
> >
> > If there are no objections, please apply and push to mainline.
> >
> > Signed-off-by: Uli Luckas <u.luckas@road.de>
> >
> > diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
> > index 888f236..f953824 100644
> > --- a/fs/jffs2/background.c
> > +++ b/fs/jffs2/background.c
> > @@ -96,9 +96,6 @@ static int jffs2_garbage_collect_thread(void *_c)
> >                         schedule();
> >                 }
> >  
> > -               if (try_to_freeze())
> > -                       continue;
> > -
> >                 /* This thread is purely an optimisation. But if it runs
> > when other things could be running, it actually makes things a lot worse.
> > Use yield() and put it at the back of the runqueue @@ -113,6 +110,9 @@
> > static int jffs2_garbage_collect_thread(void *_c) siginfo_t info;
> >                         unsigned long signr;
> >  
> > +                       if (try_to_freeze())
> > +                               continue;
> > +
> >                         signr = dequeue_signal_lock(current,
> > &current->blocked, &info);
> >                         switch(signr) {
> > @@ -137,7 +137,8 @@ static int jffs2_garbage_collect_thread(void *_c)
> >                 disallow_signal(SIGHUP);
> >  
> >                 D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread():
> > pass\n")); -               if (jffs2_garbage_collect_pass(c) == -ENOSPC)
> > { +               if (   jffs2_thread_should_wake(c)
> > +                   && jffs2_garbage_collect_pass(c) == -ENOSPC) {
> >                         printk(KERN_NOTICE "No space for garbage
> > collection. Aborting GC thread\n"); goto die;
> >                 }



-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Josh Boyer]

On Tue, 2007-06-26 at 15:06 +0200, Uli Luckas wrote:
> On Tuesday, 19. June 2007, Uli Luckas wrote:
> > Hi,
> > is there anything in the way of apllying this patch?
> >
> Please, can someone tell me what it takes to get a jffs2 bug fix into the 
> kernel? Or even reviewed? This patch should even get into 2.6.22. Am I doing 
> something wrong here?

You aren't doing anything wrong.  David is away on business.

The one thing I would hesitate with about your patch is that you've now
broken the existing behavior of the thread in regards to SIGHUP.  You
used to be able to send the threads a SIGHUP and it would do a single
pass.  With your patch, it won't do that unless the thresholds are
right.  Being able to send SIGHUP to force a GC pass is a pretty useful
thing for debugging, etc.

josh

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Tuesday, 26. June 2007, Josh Boyer wrote:
> On Tue, 2007-06-26 at 15:06 +0200, Uli Luckas wrote:
> > On Tuesday, 19. June 2007, Uli Luckas wrote:
> > > Hi,
> > > is there anything in the way of apllying this patch?
> >
> > Please, can someone tell me what it takes to get a jffs2 bug fix into the
> > kernel? Or even reviewed? This patch should even get into 2.6.22. Am I
> > doing something wrong here?
>
> You aren't doing anything wrong.  David is away on business.
>
does that mean, we won't get any fixes applied, before David is back? And does 
anyone know, when he will be back?

> The one thing I would hesitate with about your patch is that you've now
> broken the existing behavior of the thread in regards to SIGHUP.  You
> used to be able to send the threads a SIGHUP and it would do a single
> pass.  With your patch, it won't do that unless the thresholds are
> right.  Being able to send SIGHUP to force a GC pass is a pretty useful
> thing for debugging, etc.
>
thanks Josh, alot for reviewing the patch. Following your advise, I force GC 
now, if SIGHUP was received.
The patch still introduces a change in sematics though for all other signals 
(except SIGKILL). But I feel, the new behavior is more what one would expect. 
In case, the old behavior is desired, it is simple enough to set force_gc in 
the coresponding branches.

regards,
Uli

Signed-off-by: Uli Luckas <u.luckas@road.de>

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 888f236..1843786 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -71,6 +71,7 @@ void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c)
 
 static int jffs2_garbage_collect_thread(void *_c)
 {
+	int force_gc;
 	struct jffs2_sb_info *c = _c;
 
 	daemonize("jffs2_gcd_mtd%d", c->mtd->index);
@@ -96,9 +97,6 @@ static int jffs2_garbage_collect_thread(void *_c)
 			schedule();
 		}
 
-		if (try_to_freeze())
-			continue;
-
 		/* This thread is purely an optimisation. But if it runs when
 		   other things could be running, it actually makes things a
 		   lot worse. Use yield() and put it at the back of the runqueue
@@ -109,10 +107,14 @@ static int jffs2_garbage_collect_thread(void *_c)
 
 		/* Put_super will send a SIGKILL and then wait on the sem.
 		 */
+		force_gc = 0;
 		while (signal_pending(current)) {
 			siginfo_t info;
 			unsigned long signr;
 
+			if (try_to_freeze())
+				continue;
+
 			signr = dequeue_signal_lock(current, &current->blocked, &info);
 
 			switch(signr) {
@@ -128,6 +130,7 @@ static int jffs2_garbage_collect_thread(void *_c)
 
 			case SIGHUP:
 				D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): SIGHUP received.\n"));
+				force_gc = 1;
 				break;
 			default:
 				D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): signal %ld received\n", signr));
@@ -137,7 +140,8 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);
 
 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+		if ((force_gc || jffs2_thread_should_wake(c))
+		     && jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH][update] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Josh Boyer]

On Tue, 2007-06-26 at 16:39 +0200, Uli Luckas wrote:
> On Tuesday, 26. June 2007, Josh Boyer wrote:
> > On Tue, 2007-06-26 at 15:06 +0200, Uli Luckas wrote:
> > > On Tuesday, 19. June 2007, Uli Luckas wrote:
> > > > Hi,
> > > > is there anything in the way of apllying this patch?
> > >
> > > Please, can someone tell me what it takes to get a jffs2 bug fix into the
> > > kernel? Or even reviewed? This patch should even get into 2.6.22. Am I
> > > doing something wrong here?
> >
> > You aren't doing anything wrong.  David is away on business.
> >
> does that mean, we won't get any fixes applied, before David is back? And does 
> anyone know, when he will be back?

Basically, yes.  He's the one with the permissions to commit to the mtd
git tree.

> > The one thing I would hesitate with about your patch is that you've now
> > broken the existing behavior of the thread in regards to SIGHUP.  You
> > used to be able to send the threads a SIGHUP and it would do a single
> > pass.  With your patch, it won't do that unless the thresholds are
> > right.  Being able to send SIGHUP to force a GC pass is a pretty useful
> > thing for debugging, etc.
> >
> thanks Josh, alot for reviewing the patch. Following your advise, I force GC 
> now, if SIGHUP was received.
> The patch still introduces a change in sematics though for all other signals 
> (except SIGKILL). But I feel, the new behavior is more what one would expect. 
> In case, the old behavior is desired, it is simple enough to set force_gc in 
> the coresponding branches.

Your updated patch looks good to me.  Will need a Signed-off-by: line
though.

josh

Re: [PATCH][update] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Tuesday, 26. June 2007, Josh Boyer wrote:
> On Tue, 2007-06-26 at 16:39 +0200, Uli Luckas wrote:
> > On Tuesday, 26. June 2007, Josh Boyer wrote:
> > > The one thing I would hesitate with about your patch is that you've now
> > > broken the existing behavior of the thread in regards to SIGHUP.  You
> > > used to be able to send the threads a SIGHUP and it would do a single
> > > pass.  With your patch, it won't do that unless the thresholds are
> > > right.  Being able to send SIGHUP to force a GC pass is a pretty useful
> > > thing for debugging, etc.
> > >
> > thanks Josh, alot for reviewing the patch. Following your advise, I force GC 
> > now, if SIGHUP was received.
> > The patch still introduces a change in sematics though for all other signals 
> > (except SIGKILL). But I feel, the new behavior is more what one would expect. 
> > In case, the old behavior is desired, it is simple enough to set force_gc in 
> > the coresponding branches.
> 
> Your updated patch looks good to me.  Will need a Signed-off-by: line
> though.
> 
It's there, right above the patch.

Uli

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[David Woodhouse]

On Thu, 2007-06-14 at 15:09 +0200, Uli Luckas wrote:
> The problem seems to happen, when suspend kicks in, after the call to 
> try_to_freeze and before the while loop in jffs2_garbage_collect_thread.
> 
> The pending freeze will cause signal_pending to return true until the 
> attempted suspend is over and thereby the while loop won't terminate until 
> the suspend times out.
> 
> This patch will call try_to_freeze from within the while loop and adds an 
> extra check for jffs2_thread_should_wake before we actually do the GC to 
> avoid calling jffs2_garbage_collect_pass at every wakeup.

Thanks. I've committed a slightly simpler fix:
http://git.infradead.org/?p=mtd-2.6.git;a=commitdiff;h=e716dd3644c2a79d6ffa4359bac06f57479dcb34

-- 
dwmw2

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Thursday, 28. June 2007, David Woodhouse wrote:
> On Thu, 2007-06-14 at 15:09 +0200, Uli Luckas wrote:
> > The problem seems to happen, when suspend kicks in, after the call to
> > try_to_freeze and before the while loop in jffs2_garbage_collect_thread.
> >
> > The pending freeze will cause signal_pending to return true until the
> > attempted suspend is over and thereby the while loop won't terminate
> > until the suspend times out.
> >
> > This patch will call try_to_freeze from within the while loop and adds an
> > extra check for jffs2_thread_should_wake before we actually do the GC to
> > avoid calling jffs2_garbage_collect_pass at every wakeup.
>
> Thanks. I've committed a slightly simpler fix:
> http://git.infradead.org/?p=mtd-2.6.git;a=commitdiff;h=e716dd3644c2a79d6ffa
>4359bac06f57479dcb34
>
But then again, my patch was more complex for a reason :-) How does your patch 
handle the situation, where try_to_freeze returns true _and_ there is a 
pending SIGHUP?

I plead for applying my patch.

regards,
Uli

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[David Woodhouse]

On Fri, 2007-06-29 at 10:24 +0200, Uli Luckas wrote:
> But then again, my patch was more complex for a reason :-) How does your patch 
> handle the situation, where try_to_freeze returns true _and_ there is a 
> pending SIGHUP?

It'll go back to the beginning of the loop, then if it _does_ take the 
!thread_should_wake() path it'll be woken immediately by the pending
SIGHUP, and it'll do one GC pass as intended.

-- 
dwmw2

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Friday, 29. June 2007, David Woodhouse wrote:
> On Fri, 2007-06-29 at 10:24 +0200, Uli Luckas wrote:
> > But then again, my patch was more complex for a reason :-) How does your
> > patch handle the situation, where try_to_freeze returns true _and_ there
> > is a pending SIGHUP?
>
> It'll go back to the beginning of the loop, then if it _does_ take the
> !thread_should_wake() path it'll be woken immediately by the pending
> SIGHUP, and it'll do one GC pass as intended.
>
Hi David,
Thanks for your explanation. You are obviously right. 
I missed the point where a pending signal keeps wakeing INTERRUPTIBLE tasks 
until it is handled.

regards,
Uli

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit

Re: [PATCH] jffs2_gcd_mtd3, Stopping kernel threads timed out

[Uli Luckas]

On Thursday, 28. June 2007, David Woodhouse wrote:
> On Thu, 2007-06-14 at 15:09 +0200, Uli Luckas wrote:
> > The problem seems to happen, when suspend kicks in, after the call to
> > try_to_freeze and before the while loop in jffs2_garbage_collect_thread.
> >
> > The pending freeze will cause signal_pending to return true until the
> > attempted suspend is over and thereby the while loop won't terminate
> > until the suspend times out.
> >
> > This patch will call try_to_freeze from within the while loop and adds an
> > extra check for jffs2_thread_should_wake before we actually do the GC to
> > avoid calling jffs2_garbage_collect_pass at every wakeup.
>
> Thanks. I've committed a slightly simpler fix:
> http://git.infradead.org/?p=mtd-2.6.git;a=commitdiff;h=e716dd3644c2a79d6ffa
>4359bac06f57479dcb34

Hi David,
could you please push this patch upstream?

regards,
Uli

-- 

------- ROAD ...the handyPC Company - - -  ) ) )

Uli Luckas
Software Development

ROAD GmbH
Bennigsenstr. 14 | 12159 Berlin | Germany
fon: +49 (30) 230069 - 64 | fax: +49 (30) 230069 - 69
url: www.road.de

Amtsgericht Charlottenburg: HRB 96688 B
Managing directors: Hans-Peter Constien, Hubertus von Streit