Check for error returned by kthread_create on creating journal thread

PATCH In review Pavel Emelianov 3 participants 6 messages Apr 16, 2007 – Apr 23, 2007 Journalling layer for block devices (JBD2)

lore

Share & tools

lore

Pavel Emelianov

author

2007-04-16 07:36 UTC #0 lore raw

PATCH 1/1 Check for error returned by kthread_create on creating journal thread

If the thread failed to create the subsequent wait_event will hang forever.

This is likely to happen if kernel hits max_threads limit.

Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.

fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +9 -4

@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 
-static void journal_start_thread(journal_t *journal)
+static int journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);
+	return 0;
 }
 
 static void journal_kill_thread(journal_t *journal)

@@ -840,8 +846,7 @@ static int journal_reset(journal_t *jour
 
 	/* Add the dynamic fields and write it to disk. */
 	journal_update_superblock(journal, 1);
-	journal_start_thread(journal);
-	return 0;
+	return journal_start_thread(journal);
 }
 
 /**

fs/jbd2/journal.c renamed from fs/jbd2/journal.c.jbdthreads +9 -4

@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 
-static void jbd2_journal_start_thread(journal_t *journal)
+static int jbd2_journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald2, journal, "kjournald2");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald2, journal, "kjournald2");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);
+	return 0;
 }
 
 static void journal_kill_thread(journal_t *journal)

@@ -840,8 +846,7 @@ static int journal_reset(journal_t *jour
 
 	/* Add the dynamic fields and write it to disk. */
 	jbd2_journal_update_superblock(journal, 1);
-	jbd2_journal_start_thread(journal);
-	return 0;
+	return jbd2_journal_start_thread(journal);
 }
 
 /**

Christoph Hellwig

2007-04-16 10:21 UTC #1 lore raw

Quoted 15 lines · Pavel Emelianov

If the thread failed to create the subsequent wait_event will hang forever.
This is likely to happen if kernel hits max_threads limit.
Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +0 -1
@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 
-static void journal_start_thread(journal_t *journal)

fs/jbd/journal.c end_loop: · line 222

Quoted 10 lines

fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +7 -1

+static int journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);

Note that this wait_event should exist at all, and the return value of kthread_run should be assigned to journal->j_task. Also the code doesn't use the kthread primitives in other places leading to crufty code.

Pavel Emelianov

author

2007-04-16 11:07 UTC #2 lore raw

Quoted 15 lines · Christoph Hellwig

Quoted 14 lines · Pavel Emelianov
If the thread failed to create the subsequent wait_event will hang forever.
This is likely to happen if kernel hits max_threads limit.
Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +0 -0
@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 

Quoted 13 lines

fs/jbd/journal.c end_loop: · line 222

Quoted 11 lines

fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +7 -2

-static void journal_start_thread(journal_t *journal)
+static int journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);

Note that this wait_event should exist at all, and the return

Should NOT you mean?

Quoted 3 lines

value of kthread_run should be assigned to journal->j_task. Also the code doesn't use the kthread primitives in other places leading to crufty code.

Well, this could be done with a separate patch, I think.

Quoted 6 lines

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Christoph Hellwig

2007-04-16 11:10 UTC #3 lore raw

Quoted 30 lines · Pavel Emelianov

Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Pavel Emelianov
If the thread failed to create the subsequent wait_event will hang forever.
This is likely to happen if kernel hits max_threads limit.
Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +0 -0
@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 
Quoted 13 lines
fs/jbd/journal.c end_loop: · line 222
Quoted 11 lines
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +7 -2
-static void journal_start_thread(journal_t *journal)
+static int journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);
Note that this wait_event should exist at all, and the return

Quoted 1 line

Should NOT you mean?

Umm, yes - of course :)

Quoted 5 lines

Quoted 3 lines
value of kthread_run should be assigned to journal->j_task. Also the code doesn't use the kthread primitives in other places leading to crufty code.
Well, this could be done with a separate patch, I think.

Andrew Morton

2007-04-20 22:38 UTC #4 lore raw

In reply to Pavel Emelianov

Quoted 19 lines · On Mon, 16 Apr 2007 11:41:14 +0400 Pavel Emelianov <[email protected]> wrote:

If the thread failed to create the subsequent wait_event will hang forever.
This is likely to happen if kernel hits max_threads limit.
Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.
[diff-jbd-check-start-journal-thread-return-value text/plain (1.7KB)]
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +1 -1
@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 
-static void journal_start_thread(journal_t *journal)
+static int journal_start_thread(journal_t *journal)
 {

fs/jbd/journal.c end_loop: · line 224

Quoted 10 lines

fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +7 -1

-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);
+	return 0;
 }

Thanks. Please don't forget those Signed-off-by:s

I assume that you runtime tested this and that the mount failed in an appropriate fashion?

Pavel Emelianov

author

2007-04-23 06:13 UTC #5 lore raw

Quoted 19 lines · Andrew Morton

Quoted 16 lines · On Mon, 16 Apr 2007 11:41:14 +0400 Pavel Emelianov <[email protected]> wrote:
If the thread failed to create the subsequent wait_event will hang forever.
This is likely to happen if kernel hits max_threads limit.
Will be critical for virtualization systems that limit the number of tasks and kernel memory usage within the container.
[diff-jbd-check-start-journal-thread-return-value text/plain (1.7KB)]
fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +0 -0
@@ -211,10 +211,16 @@ end_loop:
 	return 0;
 }
 

Quoted 15 lines

fs/jbd/journal.c end_loop: · line 224

Quoted 13 lines

fs/jbd/journal.c renamed from fs/jbd/journal.c.jbdthreads +8 -2

-static void journal_start_thread(journal_t *journal)
+static int journal_start_thread(journal_t *journal)
 {
-	kthread_run(kjournald, journal, "kjournald");
+	struct task_struct *t;
+
+	t = kthread_run(kjournald, journal, "kjournald");
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	wait_event(journal->j_wait_done_commit, journal->j_task != 0);
+	return 0;
 }

Thanks. Please don't forget those Signed-off-by:s

I will :)

Quoted 3 lines

I assume that you runtime tested this and that the mount failed in an appropriate fashion?

Yes. This was easy to test as we can force kthread_run() to fail at the moment we want with the help of the beancounters ;)

Moreover this patch lives in our tree for a year or so in both stable and development branches.