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

[Ming Lei <ming.lei@redhat.com>]

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