Re: [patch] kjournald locking fix

Re: [patch] kjournald locking fix

[Ishan O. Jayawardena]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 787 bytes --]

Greetings everyone,

	Here's an improved version of my previous patch. Thanks to Andrew
Morton for the advice. 2.5 needs this too I think, since preemption is
part of it now...

Ishan O. Jayawardena


----------------------------------------------------------------

--- linux-preempt/fs/jbd/journal.c.1	Wed Apr  3 08:05:08 2002
+++ linux-preempt/fs/jbd/journal.c	Thu Apr  4 16:41:56 2002
@@ -204,6 +204,7 @@ int kjournald(void *arg)
 
 	lock_kernel();
 	daemonize();
+	reparent_to_init();
 	spin_lock_irq(&current->sigmask_lock);
 	sigfillset(&current->blocked);
 	recalc_sigpending(current);
@@ -267,6 +268,7 @@ int kjournald(void *arg)
 
 	journal->j_task = NULL;
 	wake_up(&journal->j_wait_done_commit);
+	unlock_kernel();
 	jbd_debug(1, "Journal thread exiting.\n");
 	return 0;
 }

[-- Attachment #2: Type: TEXT/PLAIN, Size: 538 bytes --]

--- linux-preempt/fs/jbd/journal.c.1	Wed Apr  3 08:05:08 2002
+++ linux-preempt/fs/jbd/journal.c	Thu Apr  4 16:41:56 2002
@@ -204,6 +204,7 @@ int kjournald(void *arg)
 
 	lock_kernel();
 	daemonize();
+	reparent_to_init();
 	spin_lock_irq(&current->sigmask_lock);
 	sigfillset(&current->blocked);
 	recalc_sigpending(current);
@@ -267,6 +268,7 @@ int kjournald(void *arg)
 
 	journal->j_task = NULL;
 	wake_up(&journal->j_wait_done_commit);
+	unlock_kernel();
 	jbd_debug(1, "Journal thread exiting.\n");
 	return 0;
 }

Re: [patch] kjournald locking fix

[Robert Love]

On Thu, 2002-04-04 at 17:55, Ishan O. Jayawardena wrote:

> 	Here's an improved version of my previous patch. Thanks to Andrew
> Morton for the advice. 2.5 needs this too I think, since preemption is
> part of it now...

I already sent a fix of my own to Linus last night, so 2.5 is covered.

	Robert Love

[patch] kjournald locking fix

[Ishan O. Jayawardena]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1423 bytes --]


Greetings,

	kjournald seems to be missing an unlock_kernel() for a matching
lock_kernel(). A posting by  Dennis Vadura to l-k mentions (among other
things) a kernel message that says kjournald exited with preempt_count ==
1. The attached patch (text/plain) adds the necessary
unlock_kernel(). [But I haven't been able to reproduce the hang that
Dennis experiences...]
	Tested only on UP. Patch is for 2.4.19-pre5 + prempt-kernel, _no_
lock-break. I hope the positioning of unlock_kernel() is correct... please
correct me if I'm wrong.

	Please CC me (ioshadij@hotmail.com). I can't subscribe to the list
with my own ISP because they aren't ECN-friendly, and subscribing via 

PS: Of course, the reparent_to_init() isn't part of the fix, but I've seen
kjournald become a zombie in an ugly episode with a deadlock in devfs many
moons ago.

---------------------------------------------------------------
--- linux-preempt/fs/jbd/journal.c.1	Wed Apr  3 08:05:08 2002
+++ linux-preempt/fs/jbd/journal.c	Thu Apr  4 15:09:29 2002
@@ -203,6 +203,7 @@ int kjournald(void *arg)
 	current_journal = journal;
 
 	lock_kernel();
+	reparent_to_init();
 	daemonize();
 	spin_lock_irq(&current->sigmask_lock);
 	sigfillset(&current->blocked);
@@ -267,6 +268,7 @@ int kjournald(void *arg)
 
 	journal->j_task = NULL;
 	wake_up(&journal->j_wait_done_commit);
+	unlock_kernel();
 	jbd_debug(1, "Journal thread exiting.\n");
 	return 0;
 }

[-- Attachment #2: Type: TEXT/PLAIN, Size: 537 bytes --]

--- linux-preempt/fs/jbd/journal.c.1	Wed Apr  3 08:05:08 2002
+++ linux-preempt/fs/jbd/journal.c	Thu Apr  4 15:09:29 2002
@@ -203,6 +203,7 @@ int kjournald(void *arg)
 	current_journal = journal;
 
 	lock_kernel();
+	reparent_to_init();
 	daemonize();
 	spin_lock_irq(&current->sigmask_lock);
 	sigfillset(&current->blocked);
@@ -267,6 +268,7 @@ int kjournald(void *arg)
 
 	journal->j_task = NULL;
 	wake_up(&journal->j_wait_done_commit);
+	unlock_kernel();
 	jbd_debug(1, "Journal thread exiting.\n");
 	return 0;
 }

Re: [patch] kjournald locking fix

[Andrew Morton]

"Ishan O. Jayawardena" wrote:
> 
> Greetings,
> 
>         kjournald seems to be missing an unlock_kernel() for a matching
> lock_kernel(). A posting by  Dennis Vadura to l-k mentions (among other
> things) a kernel message that says kjournald exited with preempt_count ==
> 1. The attached patch (text/plain) adds the necessary
> unlock_kernel(). [But I haven't been able to reproduce the hang that
> Dennis experiences...]
>         Tested only on UP. Patch is for 2.4.19-pre5 + prempt-kernel, _no_
> lock-break. I hope the positioning of unlock_kernel() is correct... please
> correct me if I'm wrong.

The unlock_kernel() is fine.  The kernel will drop the
lock for us as the task makes its final call to schedule()
on its way to the process graveyard, but it's neater this way.

>         Please CC me (ioshadij@hotmail.com). I can't subscribe to the list
> with my own ISP because they aren't ECN-friendly, and subscribing via
> 
> PS: Of course, the reparent_to_init() isn't part of the fix, but I've seen
> kjournald become a zombie in an ugly episode with a deadlock in devfs many
> moons ago.

I've always put the reparent_to_init() call after the daemonize()
call.  I don't immediately see anything wrong with doing it
beforehand, but that is a less tested code sequence.

But yes, you're right - kjournald needs to call reparent_to_init(),
else it'll turn into a zombie if the process which mounted the
filesystem is still running.

Could you please move the reparent_to_init() down a line and send
a diff to Marcelo?

Thanks.

Re: [patch] kjournald locking fix

[Robert Love]

On Thu, 2002-04-04 at 17:02, Ishan O. Jayawardena wrote:
> 
> Greetings,
> 
> 	kjournald seems to be missing an unlock_kernel() for a matching
> lock_kernel(). A posting by  Dennis Vadura to l-k mentions (among other
> things) a kernel message that says kjournald exited with preempt_count ==
> 1. The attached patch (text/plain) adds the necessary
> unlock_kernel(). [But I haven't been able to reproduce the hang that
> Dennis experiences...]
> 	Tested only on UP. Patch is for 2.4.19-pre5 + prempt-kernel, _no_
> lock-break. I hope the positioning of unlock_kernel() is correct... please
> correct me if I'm wrong.

The unlock_kernel is correct, thanks.

In this case, the messae about the nonzero preempt_count is not a
problem - kjournald just exits without dropping the BKL because it knows
schedule will do it for you.  It is cleaner, however, to explicitly
relinquish the lock, and it removes the preempt_count debug whining.

So the patch is good - thanks,

	Robert Love