[PATCH 1/2] btrfs: cleaner_kthread() doesn't need explicit freeze

[PATCH 1/2] btrfs: cleaner_kthread() doesn't need explicit freeze

[Jiri Kosina]

cleaner_kthread() is not marked freezable, and therefore calling 
try_to_freeze() in its context is a pointless no-op.

In addition to that, as has been clearly demonstrated by 80ad623edd2d 
("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()"), it's perfectly 
valid / legal for cleaner_kthread() to stay scheduled out in an arbitrary 
place during suspend (in that particular example that was waiting for 
reading of extent pages), so there is no need to leave any traces of 
freezer in this kthread.

Fixes: 80ad623edd2d ("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()")
Fixes: 696249132158 ("btrfs: clear PF_NOFREEZE in cleaner_kthread()")
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 fs/btrfs/disk-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4545e2e..d8d68af 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1830,7 +1830,7 @@ static int cleaner_kthread(void *arg)
 		 */
 		btrfs_delete_unused_bgs(root->fs_info);
 sleep:
-		if (!try_to_freeze() && !again) {
+		if (!again) {
 			set_current_state(TASK_INTERRUPTIBLE);
 			if (!kthread_should_stop())
 				schedule();
-- 
Jiri Kosina
SUSE Labs

[PATCH 2/2] btrfs: transaction_kthread() is not freezable

[Jiri Kosina]

transaction_kthread() is calling try_to_freeze(), but that's just an 
expeinsive no-op given the fact that the thread is not marked freezable.

After removing this, disk-io.c is now independent on freezer API.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 fs/btrfs/disk-io.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d8d68af..4c7361a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -25,7 +25,6 @@
 #include <linux/buffer_head.h>
 #include <linux/workqueue.h>
 #include <linux/kthread.h>
-#include <linux/freezer.h>
 #include <linux/slab.h>
 #include <linux/migrate.h>
 #include <linux/ratelimit.h>
@@ -1920,14 +1919,12 @@ sleep:
 		if (unlikely(test_bit(BTRFS_FS_STATE_ERROR,
 				      &root->fs_info->fs_state)))
 			btrfs_cleanup_transaction(root);
-		if (!try_to_freeze()) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			if (!kthread_should_stop() &&
-			    (!btrfs_transaction_blocked(root->fs_info) ||
-			     cannot_commit))
-				schedule_timeout(delay);
-			__set_current_state(TASK_RUNNING);
-		}
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (!kthread_should_stop() &&
+				(!btrfs_transaction_blocked(root->fs_info) ||
+				 cannot_commit))
+			schedule_timeout(delay);
+		__set_current_state(TASK_RUNNING);
 	} while (!kthread_should_stop());
 	return 0;
 }

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] btrfs: cleaner_kthread() doesn't need explicit freeze

[Jiri Kosina]

On Tue, 15 Mar 2016, Jiri Kosina wrote:

> cleaner_kthread() is not marked freezable, and therefore calling 
> try_to_freeze() in its context is a pointless no-op.
> 
> In addition to that, as has been clearly demonstrated by 80ad623edd2d 
> ("Revert "btrfs: clear PF_NOFREEZE in cleaner_kthread()"), it's perfectly 
> valid / legal for cleaner_kthread() to stay scheduled out in an arbitrary 
> place during suspend (in that particular example that was waiting for 
> reading of extent pages), so there is no need to leave any traces of 
> freezer in this kthread.

Given some questions I've received offline, let me clarify a little bit 
more here.

Currently, the try_to_freeze() call is completely useless here, because it 
will never actually try to freeze the kthread (as it's PF_NOFREEZE).

When attempted to make the kthread properly freezable, it turned out (see 
e.g. 80ad623edd2d) that it's actually sleeping in various places during 
suspend for long periods of time (my guess would be that it doesn't really 
matter whether the cleaning happens before or after suspend, but this'd be 
something I'd like to have clarified from btrfs folks).

So in a nutshell, this patch (a) doesn't make things worse, as it's an 
equivalent code transformation (b) brings more sanity to how the kthread 
freezing API is used throughout the kernel.
It might very well be that the code was broken before; but it's not more 
broken after this patch, and the API usage is sane.

The ultimate goal is first to bring some sanity into how the freezer API 
is used throughout the kernel, and then eventually get rid of it 
completely in favor of fs freezing (currently it's not even possible to 
analyze all the uses in the kernel, as there are way too many and most of 
them are totally broken).

-- 
Jiri Kosina
SUSE Labs