[PATCH] bcachefs: Remove unnecessary kthread_should_stop()

[PATCH] bcachefs: Remove unnecessary kthread_should_stop()

[Kevin Hao]

In the macro kthread_wait_freezable(), there is already a check for
kthread_should_stop(), so there is no need to pass kthread_should_stop()
as a parameter to kthread_wait_freezable().

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 fs/bcachefs/movinggc.c  | 3 +--
 fs/bcachefs/rebalance.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
index fd239a261aca..bc1ec877a66b 100644
--- a/fs/bcachefs/movinggc.c
+++ b/fs/bcachefs/movinggc.c
@@ -345,8 +345,7 @@ static int bch2_copygc_thread(void *arg)
 
 		if (!c->copy_gc_enabled) {
 			move_buckets_wait(&ctxt, buckets, true);
-			kthread_wait_freezable(c->copy_gc_enabled ||
-					       kthread_should_stop());
+			kthread_wait_freezable(c->copy_gc_enabled);
 		}
 
 		if (unlikely(freezing(current))) {
diff --git a/fs/bcachefs/rebalance.c b/fs/bcachefs/rebalance.c
index ee452aa51a26..b83dd613e247 100644
--- a/fs/bcachefs/rebalance.c
+++ b/fs/bcachefs/rebalance.c
@@ -334,8 +334,7 @@ static int do_rebalance(struct moving_context *ctxt)
 	while (!bch2_move_ratelimit(ctxt)) {
 		if (!r->enabled) {
 			bch2_moving_ctxt_flush_all(ctxt);
-			kthread_wait_freezable(r->enabled ||
-					       kthread_should_stop());
+			kthread_wait_freezable(r->enabled);
 		}
 
 		if (kthread_should_stop())
-- 
2.39.2

Re: [PATCH] bcachefs: Remove unnecessary kthread_should_stop()

[Brian Foster]

On Sat, Dec 16, 2023 at 09:51:40AM +0800, Kevin Hao wrote:
> In the macro kthread_wait_freezable(), there is already a check for
> kthread_should_stop(), so there is no need to pass kthread_should_stop()
> as a parameter to kthread_wait_freezable().
> 
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---

Looks right to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/bcachefs/movinggc.c  | 3 +--
>  fs/bcachefs/rebalance.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/bcachefs/movinggc.c b/fs/bcachefs/movinggc.c
> index fd239a261aca..bc1ec877a66b 100644
> --- a/fs/bcachefs/movinggc.c
> +++ b/fs/bcachefs/movinggc.c
> @@ -345,8 +345,7 @@ static int bch2_copygc_thread(void *arg)
>  
>  		if (!c->copy_gc_enabled) {
>  			move_buckets_wait(&ctxt, buckets, true);
> -			kthread_wait_freezable(c->copy_gc_enabled ||
> -					       kthread_should_stop());
> +			kthread_wait_freezable(c->copy_gc_enabled);
>  		}
>  
>  		if (unlikely(freezing(current))) {
> diff --git a/fs/bcachefs/rebalance.c b/fs/bcachefs/rebalance.c
> index ee452aa51a26..b83dd613e247 100644
> --- a/fs/bcachefs/rebalance.c
> +++ b/fs/bcachefs/rebalance.c
> @@ -334,8 +334,7 @@ static int do_rebalance(struct moving_context *ctxt)
>  	while (!bch2_move_ratelimit(ctxt)) {
>  		if (!r->enabled) {
>  			bch2_moving_ctxt_flush_all(ctxt);
> -			kthread_wait_freezable(r->enabled ||
> -					       kthread_should_stop());
> +			kthread_wait_freezable(r->enabled);
>  		}
>  
>  		if (kthread_should_stop())
> -- 
> 2.39.2
> 

Re: [PATCH] bcachefs: Remove unnecessary kthread_should_stop()

[Kent Overstreet]

On Tue, Dec 19, 2023 at 08:01:59AM -0500, Brian Foster wrote:
> On Sat, Dec 16, 2023 at 09:51:40AM +0800, Kevin Hao wrote:
> > In the macro kthread_wait_freezable(), there is already a check for
> > kthread_should_stop(), so there is no need to pass kthread_should_stop()
> > as a parameter to kthread_wait_freezable().
> > 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> 
> Looks right to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

It is technically correct, but recently the issues have been more with
having too few kthread_should_stop() calls, not too many; I think I want
to leave this for now, even though it's redundant it's more explicit.

Re: [PATCH] bcachefs: Remove unnecessary kthread_should_stop()

[Brian Foster]

On Tue, Dec 19, 2023 at 03:55:38PM -0500, Kent Overstreet wrote:
> On Tue, Dec 19, 2023 at 08:01:59AM -0500, Brian Foster wrote:
> > On Sat, Dec 16, 2023 at 09:51:40AM +0800, Kevin Hao wrote:
> > > In the macro kthread_wait_freezable(), there is already a check for
> > > kthread_should_stop(), so there is no need to pass kthread_should_stop()
> > > as a parameter to kthread_wait_freezable().
> > > 
> > > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > > ---
> > 
> > Looks right to me:
> > 
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> It is technically correct, but recently the issues have been more with
> having too few kthread_should_stop() calls, not too many; I think I want
> to leave this for now, even though it's redundant it's more explicit.
> 

What recent issues do you mean? ISTM this logic serves more as a placebo
than anything. Is there a more explicit issue to dig into?

Brian