[PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Mikulas Patocka]

The block layer has a function blk_wait_io - it works like
wait_for_completion_io, except that it doesn't warn if the wait takes too
long. This commit renames the function to wait_for_completion_long_io and
moves it to kernel/sched/completion.c so that other kernel subsystems can
use it. It will be needed by the dm-io subsystem.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 block/bio.c                |    2 +-
 block/blk-mq.c             |    2 +-
 block/blk.h                |   12 ------------
 include/linux/completion.h |    1 +
 kernel/sched/completion.c  |   20 ++++++++++++++++++++
 5 files changed, 23 insertions(+), 14 deletions(-)

Index: linux-2.6/block/blk.h
===================================================================
--- linux-2.6.orig/block/blk.h	2024-04-17 19:41:14.000000000 +0200
+++ linux-2.6/block/blk.h	2024-04-17 19:41:14.000000000 +0200
@@ -72,18 +72,6 @@ static inline int bio_queue_enter(struct
 	return __bio_queue_enter(q, bio);
 }
 
-static inline void blk_wait_io(struct completion *done)
-{
-	/* Prevent hang_check timer from firing at us during very long I/O */
-	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
-
-	if (timeout)
-		while (!wait_for_completion_io_timeout(done, timeout))
-			;
-	else
-		wait_for_completion_io(done);
-}
-
 #define BIO_INLINE_VECS 4
 struct bio_vec *bvec_alloc(mempool_t *pool, unsigned short *nr_vecs,
 		gfp_t gfp_mask);
Index: linux-2.6/include/linux/completion.h
===================================================================
--- linux-2.6.orig/include/linux/completion.h	2024-04-17 19:41:14.000000000 +0200
+++ linux-2.6/include/linux/completion.h	2024-04-17 19:41:14.000000000 +0200
@@ -112,6 +112,7 @@ extern long wait_for_completion_interrup
 	struct completion *x, unsigned long timeout);
 extern long wait_for_completion_killable_timeout(
 	struct completion *x, unsigned long timeout);
+extern void wait_for_completion_long_io(struct completion *x);
 extern bool try_wait_for_completion(struct completion *x);
 extern bool completion_done(struct completion *x);
 
Index: linux-2.6/block/bio.c
===================================================================
--- linux-2.6.orig/block/bio.c	2024-04-17 19:41:14.000000000 +0200
+++ linux-2.6/block/bio.c	2024-04-17 19:41:14.000000000 +0200
@@ -1378,7 +1378,7 @@ int submit_bio_wait(struct bio *bio)
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
-	blk_wait_io(&done);
+	wait_for_completion_long_io(&done);
 
 	return blk_status_to_errno(bio->bi_status);
 }
Index: linux-2.6/block/blk-mq.c
===================================================================
--- linux-2.6.orig/block/blk-mq.c	2024-04-17 19:41:14.000000000 +0200
+++ linux-2.6/block/blk-mq.c	2024-04-17 19:41:14.000000000 +0200
@@ -1407,7 +1407,7 @@ blk_status_t blk_execute_rq(struct reque
 	if (blk_rq_is_poll(rq))
 		blk_rq_poll_completion(rq, &wait.done);
 	else
-		blk_wait_io(&wait.done);
+		wait_for_completion_long_io(&wait.done);
 
 	return wait.ret;
 }
Index: linux-2.6/kernel/sched/completion.c
===================================================================
--- linux-2.6.orig/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
+++ linux-2.6/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
@@ -290,6 +290,26 @@ wait_for_completion_killable_timeout(str
 EXPORT_SYMBOL(wait_for_completion_killable_timeout);
 
 /**
+ * wait_for_completion_long_io - waits for completion of a task
+ * @x:  holds the state of this particular completion
+ *
+ * This is like wait_for_completion_io, but it doesn't warn if the wait takes
+ * too long.
+ */
+void wait_for_completion_long_io(struct completion *x)
+{
+	/* Prevent hang_check timer from firing at us during very long I/O */
+	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
+
+	if (timeout)
+		while (!wait_for_completion_io_timeout(x, timeout))
+			;
+	else
+		wait_for_completion_io(x);
+}
+EXPORT_SYMBOL(wait_for_completion_long_io);
+
+/**
  *	try_wait_for_completion - try to decrement a completion without blocking
  *	@x:	completion structure
  *

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Peter Zijlstra]

On Wed, Apr 17, 2024 at 07:49:17PM +0200, Mikulas Patocka wrote:
> Index: linux-2.6/kernel/sched/completion.c
> ===================================================================
> --- linux-2.6.orig/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> +++ linux-2.6/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> @@ -290,6 +290,26 @@ wait_for_completion_killable_timeout(str
>  EXPORT_SYMBOL(wait_for_completion_killable_timeout);
>  
>  /**
> + * wait_for_completion_long_io - waits for completion of a task
> + * @x:  holds the state of this particular completion
> + *
> + * This is like wait_for_completion_io, but it doesn't warn if the wait takes
> + * too long.
> + */
> +void wait_for_completion_long_io(struct completion *x)
> +{
> +	/* Prevent hang_check timer from firing at us during very long I/O */
> +	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
> +
> +	if (timeout)
> +		while (!wait_for_completion_io_timeout(x, timeout))
> +			;
> +	else
> +		wait_for_completion_io(x);
> +}
> +EXPORT_SYMBOL(wait_for_completion_long_io);

Urgh, why is it a sane thing to circumvent the hang check timer? 

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Mikulas Patocka]

On Wed, 17 Apr 2024, Peter Zijlstra wrote:

> On Wed, Apr 17, 2024 at 07:49:17PM +0200, Mikulas Patocka wrote:
> > Index: linux-2.6/kernel/sched/completion.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> > +++ linux-2.6/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> > @@ -290,6 +290,26 @@ wait_for_completion_killable_timeout(str
> >  EXPORT_SYMBOL(wait_for_completion_killable_timeout);
> >  
> >  /**
> > + * wait_for_completion_long_io - waits for completion of a task
> > + * @x:  holds the state of this particular completion
> > + *
> > + * This is like wait_for_completion_io, but it doesn't warn if the wait takes
> > + * too long.
> > + */
> > +void wait_for_completion_long_io(struct completion *x)
> > +{
> > +	/* Prevent hang_check timer from firing at us during very long I/O */
> > +	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
> > +
> > +	if (timeout)
> > +		while (!wait_for_completion_io_timeout(x, timeout))
> > +			;
> > +	else
> > +		wait_for_completion_io(x);
> > +}
> > +EXPORT_SYMBOL(wait_for_completion_long_io);
> 
> Urgh, why is it a sane thing to circumvent the hang check timer? 

The block layer already does it - the bios can have arbitrary size, so 
waiting for them takes arbitrary time.

Mikulas

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Christoph Hellwig]

On Wed, Apr 17, 2024 at 08:00:22PM +0200, Mikulas Patocka wrote:
> > > +EXPORT_SYMBOL(wait_for_completion_long_io);
> > 
> > Urgh, why is it a sane thing to circumvent the hang check timer? 
> 
> The block layer already does it - the bios can have arbitrary size, so 
> waiting for them takes arbitrary time.

And as mentioned the last few times around, I think we want a task
state to say that task can sleep long or even forever and not propagate
this hack even further.

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Jens Axboe]

On 4/17/24 10:57 PM, Christoph Hellwig wrote:
> On Wed, Apr 17, 2024 at 08:00:22PM +0200, Mikulas Patocka wrote:
>>>> +EXPORT_SYMBOL(wait_for_completion_long_io);
>>>
>>> Urgh, why is it a sane thing to circumvent the hang check timer? 
>>
>> The block layer already does it - the bios can have arbitrary size, so 
>> waiting for them takes arbitrary time.
> 
> And as mentioned the last few times around, I think we want a task
> state to say that task can sleep long or even forever and not propagate
> this hack even further.

It certainly is a hack/work-around, but unless there are a lot more that
should be using something like this, I don't think adding extra core
complexity in terms of a special task state (or per-task flag, at least
that would be easier) is really warranted.

-- 
Jens Axboe

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Christoph Hellwig]

On Thu, Apr 18, 2024 at 08:30:14AM -0600, Jens Axboe wrote:
> It certainly is a hack/work-around, but unless there are a lot more that
> should be using something like this, I don't think adding extra core
> complexity in terms of a special task state (or per-task flag, at least
> that would be easier) is really warranted.

Basically any kernel thread doing on-demand work has the same problem.
It just has an easier workaround hack, as the kernel threads can simply
claim to do an interruptible sleep to not trigger the softlockup
warnings.

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Jens Axboe]

On 4/18/24 8:46 AM, Christoph Hellwig wrote:
> On Thu, Apr 18, 2024 at 08:30:14AM -0600, Jens Axboe wrote:
>> It certainly is a hack/work-around, but unless there are a lot more that
>> should be using something like this, I don't think adding extra core
>> complexity in terms of a special task state (or per-task flag, at least
>> that would be easier) is really warranted.
> 
> Basically any kernel thread doing on-demand work has the same problem.
> It just has an easier workaround hack, as the kernel threads can simply
> claim to do an interruptible sleep to not trigger the softlockup
> warnings.

A kernel thread can just use TASK_INTERRUPTIBLE, as it doesn't take
signals anyway. But yeah, I guess you could view that as a work-around
as well.

Outside of that, mostly only a block problem, where our sleep is always
uninterruptible. Unless there are similar hacks elsewhere in the kernel
that I'm not aware of?

-- 
Jens Axboe

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Peter Zijlstra]

On Wed, Apr 17, 2024 at 08:00:22PM +0200, Mikulas Patocka wrote:
> 
> 
> On Wed, 17 Apr 2024, Peter Zijlstra wrote:
> 
> > On Wed, Apr 17, 2024 at 07:49:17PM +0200, Mikulas Patocka wrote:
> > > Index: linux-2.6/kernel/sched/completion.c
> > > ===================================================================
> > > --- linux-2.6.orig/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> > > +++ linux-2.6/kernel/sched/completion.c	2024-04-17 19:41:14.000000000 +0200
> > > @@ -290,6 +290,26 @@ wait_for_completion_killable_timeout(str
> > >  EXPORT_SYMBOL(wait_for_completion_killable_timeout);
> > >  
> > >  /**
> > > + * wait_for_completion_long_io - waits for completion of a task
> > > + * @x:  holds the state of this particular completion
> > > + *
> > > + * This is like wait_for_completion_io, but it doesn't warn if the wait takes
> > > + * too long.
> > > + */
> > > +void wait_for_completion_long_io(struct completion *x)
> > > +{
> > > +	/* Prevent hang_check timer from firing at us during very long I/O */
> > > +	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
> > > +
> > > +	if (timeout)
> > > +		while (!wait_for_completion_io_timeout(x, timeout))
> > > +			;
> > > +	else
> > > +		wait_for_completion_io(x);
> > > +}
> > > +EXPORT_SYMBOL(wait_for_completion_long_io);
> > 
> > Urgh, why is it a sane thing to circumvent the hang check timer? 
> 
> The block layer already does it - the bios can have arbitrary size, so 
> waiting for them takes arbitrary time.

Yeah, but now you make it generic and your comment doesn't warn people
away, it makes them think this is a sane thing to do.

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Peter Zijlstra]

On Wed, Apr 17, 2024 at 09:57:04PM -0700, Christoph Hellwig wrote:
> On Wed, Apr 17, 2024 at 08:00:22PM +0200, Mikulas Patocka wrote:
> > > > +EXPORT_SYMBOL(wait_for_completion_long_io);
> > > 
> > > Urgh, why is it a sane thing to circumvent the hang check timer? 
> > 
> > The block layer already does it - the bios can have arbitrary size, so 
> > waiting for them takes arbitrary time.
> 
> And as mentioned the last few times around, I think we want a task
> state to say that task can sleep long or even forever and not propagate
> this hack even further.

A bit like TASK_NOLOAD (which is used to make TASK_IDLE work), but
different I suppose.

TASK_NOHUNG would be trivial to add ofc. But is it worth it?

Anyway, as per the other email, anything like this needs to come with a
big fat warning. You get to keep the pieces etc..

---
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3c2abbc587b4..83b25327c233 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -112,7 +112,8 @@ struct user_event_mm;
 #define TASK_FREEZABLE			0x00002000
 #define __TASK_FREEZABLE_UNSAFE	       (0x00004000 * IS_ENABLED(CONFIG_LOCKDEP))
 #define TASK_FROZEN			0x00008000
-#define TASK_STATE_MAX			0x00010000
+#define TASK_NOHUNG			0x00010000
+#define TASK_STATE_MAX			0x00020000
 
 #define TASK_ANY			(TASK_STATE_MAX-1)
 
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index b2fc2727d654..126fac835e5e 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -210,7 +210,8 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
 		state = READ_ONCE(t->__state);
 		if ((state & TASK_UNINTERRUPTIBLE) &&
 		    !(state & TASK_WAKEKILL) &&
-		    !(state & TASK_NOLOAD))
+		    !(state & TASK_NOLOAD) &&
+		    !(state & TASK_NOHUNG))
 			check_hung_task(t, timeout);
 	}
  unlock:

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Mikulas Patocka]

On Mon, 22 Apr 2024, Peter Zijlstra wrote:

> On Wed, Apr 17, 2024 at 09:57:04PM -0700, Christoph Hellwig wrote:
> > On Wed, Apr 17, 2024 at 08:00:22PM +0200, Mikulas Patocka wrote:
> > > > > +EXPORT_SYMBOL(wait_for_completion_long_io);
> > > > 
> > > > Urgh, why is it a sane thing to circumvent the hang check timer? 
> > > 
> > > The block layer already does it - the bios can have arbitrary size, so 
> > > waiting for them takes arbitrary time.
> > 
> > And as mentioned the last few times around, I think we want a task
> > state to say that task can sleep long or even forever and not propagate
> > this hack even further.
> 
> A bit like TASK_NOLOAD (which is used to make TASK_IDLE work), but
> different I suppose.
> 
> TASK_NOHUNG would be trivial to add ofc. But is it worth it?
> 
> Anyway, as per the other email, anything like this needs to come with a
> big fat warning. You get to keep the pieces etc..

This seems better than the blk_wait_io hack.

Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>

> ---
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 3c2abbc587b4..83b25327c233 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -112,7 +112,8 @@ struct user_event_mm;
>  #define TASK_FREEZABLE			0x00002000
>  #define __TASK_FREEZABLE_UNSAFE	       (0x00004000 * IS_ENABLED(CONFIG_LOCKDEP))
>  #define TASK_FROZEN			0x00008000
> -#define TASK_STATE_MAX			0x00010000
> +#define TASK_NOHUNG			0x00010000
> +#define TASK_STATE_MAX			0x00020000
>  
>  #define TASK_ANY			(TASK_STATE_MAX-1)
>  
> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
> index b2fc2727d654..126fac835e5e 100644
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -210,7 +210,8 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)
>  		state = READ_ONCE(t->__state);
>  		if ((state & TASK_UNINTERRUPTIBLE) &&
>  		    !(state & TASK_WAKEKILL) &&
> -		    !(state & TASK_NOLOAD))
> +		    !(state & TASK_NOLOAD) &&
> +		    !(state & TASK_NOHUNG))
>  			check_hung_task(t, timeout);
>  	}
>   unlock:
> 

Re: [PATCH 1/2] completion: move blk_wait_io to kernel/sched/completion.c

[Christoph Hellwig]

On Mon, Apr 22, 2024 at 12:59:56PM +0200, Peter Zijlstra wrote:
> A bit like TASK_NOLOAD (which is used to make TASK_IDLE work), but
> different I suppose.
> 
> TASK_NOHUNG would be trivial to add ofc. But is it worth it?

Yes.  And it would allow us to kill the horrible existing block hack.