[PATCH 0/2] block: prevent task hung from being triggered in sync dio

[PATCH 0/2] block: prevent task hung from being triggered in sync dio

[Ming Lei]

Hi,

The 1st patch adds one helper of blk_default_io_timeout().

The 2nd patch adds blk_io_schedule for prevent task hung from being
triggered in sync dio.


Ming Lei (2):
  block: add blk_default_io_timeout() for avoiding task hung in sync IO
  block: add blk_io_schedule() for avoiding task hung in sync dio

 block/bio.c            |  9 +++------
 block/blk-exec.c       |  8 +++-----
 fs/block_dev.c         |  4 ++--
 fs/direct-io.c         |  2 +-
 fs/iomap/direct-io.c   |  2 +-
 include/linux/blkdev.h | 16 ++++++++++++++++
 6 files changed, 26 insertions(+), 15 deletions(-)

Cc: Salman Qazi <sqazi@google.com>
Cc: Jesse Barnes <jsbarnes@google.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>


-- 
2.25.2

[PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Ming Lei]

Add helper of blk_default_io_timeout(), so that the two current users
can benefit from it.

Also direct IO users will use it in the following patch, so define the
helper in public header.

Cc: Salman Qazi <sqazi@google.com>
Cc: Jesse Barnes <jsbarnes@google.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio.c            |  9 +++------
 block/blk-exec.c       |  8 +++-----
 include/linux/blkdev.h | 10 ++++++++++
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 21cbaa6a1c20..f67afa159de7 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1069,18 +1069,15 @@ static void submit_bio_wait_endio(struct bio *bio)
 int submit_bio_wait(struct bio *bio)
 {
 	DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map);
-	unsigned long hang_check;
+	unsigned long timeout = blk_default_io_timeout();
 
 	bio->bi_private = &done;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
 
-	/* Prevent hang_check timer from firing at us during very long I/O */
-	hang_check = sysctl_hung_task_timeout_secs;
-	if (hang_check)
-		while (!wait_for_completion_io_timeout(&done,
-					hang_check * (HZ/2)))
+	if (timeout)
+		while (!wait_for_completion_io_timeout(&done, timeout))
 			;
 	else
 		wait_for_completion_io(&done);
diff --git a/block/blk-exec.c b/block/blk-exec.c
index e20a852ae432..17b5cf07e1a3 100644
--- a/block/blk-exec.c
+++ b/block/blk-exec.c
@@ -80,15 +80,13 @@ void blk_execute_rq(struct request_queue *q, struct gendisk *bd_disk,
 		   struct request *rq, int at_head)
 {
 	DECLARE_COMPLETION_ONSTACK(wait);
-	unsigned long hang_check;
+	unsigned long timeout = blk_default_io_timeout();
 
 	rq->end_io_data = &wait;
 	blk_execute_rq_nowait(q, bd_disk, rq, at_head, blk_end_sync_rq);
 
-	/* Prevent hang_check timer from firing at us during very long I/O */
-	hang_check = sysctl_hung_task_timeout_secs;
-	if (hang_check)
-		while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2)));
+	if (timeout)
+		while (!wait_for_completion_io_timeout(&wait, timeout));
 	else
 		wait_for_completion_io(&wait);
 }
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index f00bd4042295..3d594406b96c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -27,6 +27,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/scatterlist.h>
 #include <linux/blkzoned.h>
+#include <linux/sched/sysctl.h>
 
 struct module;
 struct scsi_ioctl_command;
@@ -1827,4 +1828,13 @@ static inline void blk_wake_io_task(struct task_struct *waiter)
 		wake_up_process(waiter);
 }
 
+/*
+ * Used in sync IO for avoiding to triger task hung warning, which may
+ * cause system panic or reboot.
+ */
+static inline unsigned long blk_default_io_timeout(void)
+{
+	return sysctl_hung_task_timeout_secs * (HZ / 2);
+}
+
 #endif
-- 
2.25.2

[PATCH 2/2] block: add blk_io_schedule() for avoiding task hung in sync dio

[Ming Lei]

Sync dio could be big, or may take long time in discard or in case of
IO failure.

We have prevented task hung in submit_bio_wait() and blk_execute_rq(),
so apply the similar trick for prevent task hung from happening in
sync dio.

Add helper of blk_io_schedule() and use io_schedule_timeout(
blk_default_io_timeout()) to prevent task hung warning.

Cc: Salman Qazi <sqazi@google.com>
Cc: Jesse Barnes <jsbarnes@google.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 fs/block_dev.c         | 4 ++--
 fs/direct-io.c         | 2 +-
 fs/iomap/direct-io.c   | 2 +-
 include/linux/blkdev.h | 6 ++++++
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 5eb30a474f6d..3b396f8c967c 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -256,7 +256,7 @@ __blkdev_direct_IO_simple(struct kiocb *iocb, struct iov_iter *iter,
 			break;
 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
 		    !blk_poll(bdev_get_queue(bdev), qc, true))
-			io_schedule();
+			blk_io_schedule();
 	}
 	__set_current_state(TASK_RUNNING);
 
@@ -450,7 +450,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
 
 		if (!(iocb->ki_flags & IOCB_HIPRI) ||
 		    !blk_poll(bdev_get_queue(bdev), qc, true))
-			io_schedule();
+			blk_io_schedule();
 	}
 	__set_current_state(TASK_RUNNING);
 
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 00b4d15bb811..6d5370eac2a8 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -500,7 +500,7 @@ static struct bio *dio_await_one(struct dio *dio)
 		spin_unlock_irqrestore(&dio->bio_lock, flags);
 		if (!(dio->iocb->ki_flags & IOCB_HIPRI) ||
 		    !blk_poll(dio->bio_disk->queue, dio->bio_cookie, true))
-			io_schedule();
+			blk_io_schedule();
 		/* wake up sets us TASK_RUNNING */
 		spin_lock_irqsave(&dio->bio_lock, flags);
 		dio->waiter = NULL;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 20dde5aadcdd..fd3bd06fabb6 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -561,7 +561,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 			    !dio->submit.last_queue ||
 			    !blk_poll(dio->submit.last_queue,
 					 dio->submit.cookie, true))
-				io_schedule();
+				blk_io_schedule();
 		}
 		__set_current_state(TASK_RUNNING);
 	}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d594406b96c..c47c76cbbd97 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -28,6 +28,7 @@
 #include <linux/scatterlist.h>
 #include <linux/blkzoned.h>
 #include <linux/sched/sysctl.h>
+#include <linux/sched.h>
 
 struct module;
 struct scsi_ioctl_command;
@@ -1837,4 +1838,9 @@ static inline unsigned long blk_default_io_timeout(void)
 	return sysctl_hung_task_timeout_secs * (HZ / 2);
 }
 
+static inline void blk_io_schedule(void)
+{
+	io_schedule_timeout(blk_default_io_timeout());
+}
+
 #endif
-- 
2.25.2

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Hannes Reinecke]

On 4/28/20 9:46 AM, Ming Lei wrote:
> Add helper of blk_default_io_timeout(), so that the two current users
> can benefit from it.
> 
> Also direct IO users will use it in the following patch, so define the
> helper in public header.
> 
> Cc: Salman Qazi <sqazi@google.com>
> Cc: Jesse Barnes <jsbarnes@google.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   block/bio.c            |  9 +++------
>   block/blk-exec.c       |  8 +++-----
>   include/linux/blkdev.h | 10 ++++++++++
>   3 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 21cbaa6a1c20..f67afa159de7 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1069,18 +1069,15 @@ static void submit_bio_wait_endio(struct bio *bio)
>   int submit_bio_wait(struct bio *bio)
>   {
>   	DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map);
> -	unsigned long hang_check;
> +	unsigned long timeout = blk_default_io_timeout();
>   
>   	bio->bi_private = &done;
>   	bio->bi_end_io = submit_bio_wait_endio;
>   	bio->bi_opf |= REQ_SYNC;
>   	submit_bio(bio);
>   
> -	/* Prevent hang_check timer from firing at us during very long I/O */
> -	hang_check = sysctl_hung_task_timeout_secs;
> -	if (hang_check)
> -		while (!wait_for_completion_io_timeout(&done,
> -					hang_check * (HZ/2)))
> +	if (timeout)
> +		while (!wait_for_completion_io_timeout(&done, timeout))
>   			;
>   	else
>   		wait_for_completion_io(&done);
> diff --git a/block/blk-exec.c b/block/blk-exec.c
> index e20a852ae432..17b5cf07e1a3 100644
> --- a/block/blk-exec.c
> +++ b/block/blk-exec.c
> @@ -80,15 +80,13 @@ void blk_execute_rq(struct request_queue *q, struct gendisk *bd_disk,
>   		   struct request *rq, int at_head)
>   {
>   	DECLARE_COMPLETION_ONSTACK(wait);
> -	unsigned long hang_check;
> +	unsigned long timeout = blk_default_io_timeout();
>   
>   	rq->end_io_data = &wait;
>   	blk_execute_rq_nowait(q, bd_disk, rq, at_head, blk_end_sync_rq);
>   
> -	/* Prevent hang_check timer from firing at us during very long I/O */
> -	hang_check = sysctl_hung_task_timeout_secs;
> -	if (hang_check)
> -		while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2)));
> +	if (timeout)
> +		while (!wait_for_completion_io_timeout(&wait, timeout));
>   	else
>   		wait_for_completion_io(&wait);
>   }
This probably just shows my ignorance, but why don't we check for 
rq->timeout here?
I do see that not all requests have a timeout, but what about those who 
have?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Ming Lei]

On Tue, Apr 28, 2020 at 10:05:36AM +0200, Hannes Reinecke wrote:
> On 4/28/20 9:46 AM, Ming Lei wrote:
> > Add helper of blk_default_io_timeout(), so that the two current users
> > can benefit from it.
> > 
> > Also direct IO users will use it in the following patch, so define the
> > helper in public header.
> > 
> > Cc: Salman Qazi <sqazi@google.com>
> > Cc: Jesse Barnes <jsbarnes@google.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   block/bio.c            |  9 +++------
> >   block/blk-exec.c       |  8 +++-----
> >   include/linux/blkdev.h | 10 ++++++++++
> >   3 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/block/bio.c b/block/bio.c
> > index 21cbaa6a1c20..f67afa159de7 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -1069,18 +1069,15 @@ static void submit_bio_wait_endio(struct bio *bio)
> >   int submit_bio_wait(struct bio *bio)
> >   {
> >   	DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map);
> > -	unsigned long hang_check;
> > +	unsigned long timeout = blk_default_io_timeout();
> >   	bio->bi_private = &done;
> >   	bio->bi_end_io = submit_bio_wait_endio;
> >   	bio->bi_opf |= REQ_SYNC;
> >   	submit_bio(bio);
> > -	/* Prevent hang_check timer from firing at us during very long I/O */
> > -	hang_check = sysctl_hung_task_timeout_secs;
> > -	if (hang_check)
> > -		while (!wait_for_completion_io_timeout(&done,
> > -					hang_check * (HZ/2)))
> > +	if (timeout)
> > +		while (!wait_for_completion_io_timeout(&done, timeout))
> >   			;
> >   	else
> >   		wait_for_completion_io(&done);
> > diff --git a/block/blk-exec.c b/block/blk-exec.c
> > index e20a852ae432..17b5cf07e1a3 100644
> > --- a/block/blk-exec.c
> > +++ b/block/blk-exec.c
> > @@ -80,15 +80,13 @@ void blk_execute_rq(struct request_queue *q, struct gendisk *bd_disk,
> >   		   struct request *rq, int at_head)
> >   {
> >   	DECLARE_COMPLETION_ONSTACK(wait);
> > -	unsigned long hang_check;
> > +	unsigned long timeout = blk_default_io_timeout();
> >   	rq->end_io_data = &wait;
> >   	blk_execute_rq_nowait(q, bd_disk, rq, at_head, blk_end_sync_rq);
> > -	/* Prevent hang_check timer from firing at us during very long I/O */
> > -	hang_check = sysctl_hung_task_timeout_secs;
> > -	if (hang_check)
> > -		while (!wait_for_completion_io_timeout(&wait, hang_check * (HZ/2)));
> > +	if (timeout)
> > +		while (!wait_for_completion_io_timeout(&wait, timeout));
> >   	else
> >   		wait_for_completion_io(&wait);
> >   }
> This probably just shows my ignorance, but why don't we check for
> rq->timeout here?
> I do see that not all requests have a timeout, but what about those who
> have?

Here the IO means IO from upper layer(FS, user space, ...), and this
kind of IO isn't same with block layer's IO request which is splitted
from upper layer's bio.

So we can't apply the rq->timeout directly, especially we want to avoid
the task hung on the sync IO from upper layer.

Thanks,
Ming

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Bart Van Assche]

On 2020-04-28 00:46, Ming Lei wrote:
> +/*
> + * Used in sync IO for avoiding to triger task hung warning, which may
> + * cause system panic or reboot.
> + */
> +static inline unsigned long blk_default_io_timeout(void)
> +{
> +	return sysctl_hung_task_timeout_secs * (HZ / 2);
> +}
> +
>  #endif

This function is only used inside the block layer. Has it been
considered to move this function from the public block layer API into a
private header file, e.g. block/blk.h?

Thanks,

Bart.

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Ming Lei]

On Tue, Apr 28, 2020 at 07:19:33AM -0700, Bart Van Assche wrote:
> On 2020-04-28 00:46, Ming Lei wrote:
> > +/*
> > + * Used in sync IO for avoiding to triger task hung warning, which may
> > + * cause system panic or reboot.
> > + */
> > +static inline unsigned long blk_default_io_timeout(void)
> > +{
> > +	return sysctl_hung_task_timeout_secs * (HZ / 2);
> > +}
> > +
> >  #endif
> 
> This function is only used inside the block layer. Has it been
> considered to move this function from the public block layer API into a
> private header file, e.g. block/blk.h?

Please look at the commit log or the 2nd patch, and the helper will be
used in 2nd patch in dio code.

Thanks,
Ming

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Bart Van Assche]

On 2020-04-28 18:17, Ming Lei wrote:
> On Tue, Apr 28, 2020 at 07:19:33AM -0700, Bart Van Assche wrote:
>> On 2020-04-28 00:46, Ming Lei wrote:
>>> +/*
>>> + * Used in sync IO for avoiding to triger task hung warning, which may
>>> + * cause system panic or reboot.
>>> + */
>>> +static inline unsigned long blk_default_io_timeout(void)
>>> +{
>>> +	return sysctl_hung_task_timeout_secs * (HZ / 2);
>>> +}
>>> +
>>>  #endif
>>
>> This function is only used inside the block layer. Has it been
>> considered to move this function from the public block layer API into a
>> private header file, e.g. block/blk.h?
> 
> Please look at the commit log or the 2nd patch, and the helper will be
> used in 2nd patch in dio code.

Has it been considered to use the expression
"sysctl_hung_task_timeout_secs * (HZ / 2)" directly instead of wrapping
that expression in a function? I think using the expression directly may
be more clear. Additionally, it is slightly confusing that the function
name starts with "blk_" but that nothing in the implementation of that
function is specific to the block layer.

Thanks,

Bart.

Re: [PATCH 1/2] block: add blk_default_io_timeout() for avoiding task hung in sync IO

[Ming Lei]

On Wed, Apr 29, 2020 at 08:08:03PM -0700, Bart Van Assche wrote:
> On 2020-04-28 18:17, Ming Lei wrote:
> > On Tue, Apr 28, 2020 at 07:19:33AM -0700, Bart Van Assche wrote:
> >> On 2020-04-28 00:46, Ming Lei wrote:
> >>> +/*
> >>> + * Used in sync IO for avoiding to triger task hung warning, which may
> >>> + * cause system panic or reboot.
> >>> + */
> >>> +static inline unsigned long blk_default_io_timeout(void)
> >>> +{
> >>> +	return sysctl_hung_task_timeout_secs * (HZ / 2);
> >>> +}
> >>> +
> >>>  #endif
> >>
> >> This function is only used inside the block layer. Has it been
> >> considered to move this function from the public block layer API into a
> >> private header file, e.g. block/blk.h?
> > 
> > Please look at the commit log or the 2nd patch, and the helper will be
> > used in 2nd patch in dio code.
> 
> Has it been considered to use the expression
> "sysctl_hung_task_timeout_secs * (HZ / 2)" directly instead of wrapping
> that expression in a function? I think using the expression directly may
> be more clear. Additionally, it is slightly confusing that the function
> name starts with "blk_" but that nothing in the implementation of that
> function is specific to the block layer.

Fine, will do it in V2.

thanks,
Ming