Skip to contentSkip to conversation

JBD and JBD2 missing set_freezable()

PATCH In review Nigel Cunningham 3 participants 4 messages Feb 3, 2012 – Mar 10, 2012

Subsystems (2)
VFS Journalling layer for block devices (JBD2)
lore
Share & tools
lore

Nigel Cunningham

author
#0 lore raw

PATCH 1/1 JBD and JBD2 missing set_freezable()

Hi all.

With the latest and greatest changes to the freezer, I started seeing panics that were caused by jbd2 running post-process freezing and hitting the canary BUG_ON for non-TuxOnIce I/O submission. I've traced this back to a lack of set_freezable calls in both jbd and jbd2. Since they're clearly meant to be frozen (there are tests for freezing()), I submit the following patch to add the missing calls.

Signed-off-by Nigel Cunningham <[email protected]>
fs/jbd/journal.c +2 -0
index 59c09f9..89cd985 100644
@@ -129,6 +129,8 @@ static int kjournald(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
fs/jbd2/journal.c +2 -0
index c0a5f9f..663e47c 100644
@@ -139,6 +139,8 @@ static int kjournald2(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
Signature · 3 lines
Evolution (n): A hypothetical process whereby improbable
events occur with alarming frequency, order arises from chaos, and
no one is given credit.
#1 lore raw
Quoted 10 lines · Nigel Cunningham

Hi all.

With the latest and greatest changes to the freezer, I started seeing panics that were caused by jbd2 running post-process freezing and hitting the canary BUG_ON for non-TuxOnIce I/O submission. I've traced this back to a lack of set_freezable calls in both jbd and jbd2. Since they're clearly meant to be frozen (there are tests for freezing()), I submit the following patch to add the missing calls.

Signed-off-by Nigel Cunningham <[email protected]>

Well, I wonder what the filesystems people think about that.

Thanks,
Rafael

Quoted 27 lines
fs/jbd/journal.c +2 -0
index 59c09f9..89cd985 100644
@@ -129,6 +129,8 @@ static int kjournald(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
fs/jbd2/journal.c +2 -0
index c0a5f9f..663e47c 100644
@@ -139,6 +139,8 @@ static int kjournald2(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
reviewer
#2 lore raw
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Nigel Cunningham

Hi all.

With the latest and greatest changes to the freezer, I started seeing panics that were caused by jbd2 running post-process freezing and hitting the canary BUG_ON for non-TuxOnIce I/O submission. I've traced this back to a lack of set_freezable calls in both jbd and jbd2. Since they're clearly meant to be frozen (there are tests for freezing()), I submit the following patch to add the missing calls.

Quoted 3 lines
Quoted 1 line
Signed-off-by Nigel Cunningham <[email protected]>

Well, I wonder what the filesystems people think about that.

Nigel is right that threads are meant to be freezable. If below patch
is needed for that, you have my ack.

Just two notes:
1) Freezing the journal thread will effectively block filesystem activity (not immediately but once transaction fills up) so you should better make sure you don't need to do anything with the filesystem after freezing the thread.

2) Is freezing journal thread still needed when we freeze filesystem in suspend code? Because no IO should happen once filesystem is frozen...

Honza

Quoted 33 lines

Thanks,
Rafael

Quoted 27 lines
fs/jbd/journal.c +2 -0
index 59c09f9..89cd985 100644
@@ -129,6 +129,8 @@ static int kjournald(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
fs/jbd2/journal.c +2 -0
index c0a5f9f..663e47c 100644
@@ -139,6 +139,8 @@ static int kjournald2(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#3 lore raw

Hi,

Sorry for the delay.

Quoted 11 lines · Jan Kara
Quoted 10 lines · Rafael J. Wysocki
Quoted 9 lines · Nigel Cunningham

Hi all.

With the latest and greatest changes to the freezer, I started seeing panics that were caused by jbd2 running post-process freezing and hitting the canary BUG_ON for non-TuxOnIce I/O submission. I've traced this back to a lack of set_freezable calls in both jbd and jbd2. Since they're clearly meant to be frozen (there are tests for freezing()), I submit the following patch to add the missing calls.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Nigel Cunningham <[email protected]>

Well, I wonder what the filesystems people think about that.

Nigel is right that threads are meant to be freezable. If below patch
is needed for that, you have my ack.

Yes, it is, thanks! I'm going to push it for v3.4 along with PM updates.

Quoted 5 lines

Just two notes:
1) Freezing the journal thread will effectively block filesystem activity (not immediately but once transaction fills up) so you should better make sure you don't need to do anything with the filesystem after freezing the thread.

Sure, no worries. :-)

Quoted 2 lines

2) Is freezing journal thread still needed when we freeze filesystem in suspend code? Because no IO should happen once filesystem is frozen...

It won't be strictly necessary, but it won't hurt either. However, until we start freezing filesystems in suspend code, it is needed.

Thanks,
Rafael

Quoted 29 lines
Quoted 28 lines
Quoted 27 lines
fs/jbd/journal.c +2 -0
index 59c09f9..89cd985 100644
@@ -129,6 +129,8 @@ static int kjournald(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);
fs/jbd2/journal.c +2 -0
index c0a5f9f..663e47c 100644
@@ -139,6 +139,8 @@ static int kjournald2(void *arg)
 	setup_timer(&journal->j_commit_timer, commit_timeout,
 			(unsigned long)current);
 
+	set_freezable();
+
 	/* Record that the journal thread is running */
 	journal->j_task = current;
 	wake_up(&journal->j_wait_done_commit);