Re: [PATCH V2] block: avoid io timeout in case of sync polled dio

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

On Fri, Apr 15, 2022 at 07:18:44AM +0200, Christoph Hellwig wrote:
> On Fri, Apr 15, 2022 at 11:47:03AM +0800, Ming Lei wrote:
> > +	/* make sure the bio is issued before polling */
> > +	if (bio.bi_opf & REQ_POLLED)
> > +		blk_flush_plug(current->plug, false);
> 
> I still think the core code should handle this.  Without that we'd need
> to export the blk_flush_plug for anything that would want to poll bios
> from modules, in addition to it generally being a mess.  See a proposed

So far there isn't such usage yet. dm calls bio_poll() in ->iopoll(),
and its caller(io_uring) will finish the plug.

> patch for that below.  I'd also split the flush aspect from the poll
> aspect into two patches.
> 
> > +		if (bio.bi_opf & REQ_POLLED)
> > +			bio_poll(&bio, NULL, 0);
> > +		else
> >  			blk_io_schedule();
> 
> Instead of this duplicate logic everywhere I'd just make bio_boll
> call blk_io_schedule for the !REQ_POLLED case and simplify all the
> callers.

bio_poll() may be called with rcu read lock held, so I'd suggest to
not mix the two together.

> 
> > +			if (dio->submit.poll_bio &&
> > +					(dio->submit.poll_bio->bi_opf &
> > +						REQ_POLLED))
> 
> This indentation looks awfull,normal would be:
> 
> 			if (dio->submit.poll_bio &&
> 			    (dio->submit.poll_bio->bi_opf & REQ_POLLED))

That follows the indentation style of fs/iomap/direct-io.c for break in
'if'.

> 
> ---
> From 08ff61b0142eb708fc384cf867c72175561d974a Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 15 Apr 2022 07:15:42 +0200
> Subject: blk-mq: don't plug for synchronously polled requests
> 
> For synchronous polling to work, the bio must be issued to the driver from
> the submit_bio call, otherwise ->bi_cookie won't be set.
> 
> Based on a patch from Ming Lei.
> 
> Reported-by: Changhui Zhong <czhong@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ed3ed86f7dd24..bcc7e3d11296c 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2851,7 +2851,13 @@ void blk_mq_submit_bio(struct bio *bio)
>  		return;
>  	}
>  
> -	if (plug)
> +	/*
> +	 * We can't plug for synchronously polled submissions, otherwise
> +	 * bio->bi_cookie won't be set directly after submission, which is the
> +	 * indicator used by the submitter to check if a bio needs polling.
> +	 */
> +	if (plug &&
> +	    (rq->bio->bi_opf & (REQ_POLLED | REQ_NOWAIT)) != REQ_POLLED)
>  		blk_add_rq_to_plug(plug, rq);
>  	else if ((rq->rq_flags & RQF_ELV) ||
>  		 (rq->mq_hctx->dispatch_busy &&

It is nothing to do with REQ_NOWAIT. sync polled dio can be marked as
REQ_NOWAIT by userspace too. If '--nowait=1' is added in the fio
reproducer, io timeout is triggered too.



Thanks,
Ming