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

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

On Sat, Apr 16, 2022 at 07:49:13AM +0200, Christoph Hellwig wrote:
> On Fri, Apr 15, 2022 at 07:00:37PM +0800, Ming Lei wrote:
> > 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.
> 
> Yes.  But not doing this automatically also means you keep easily
> forgetting callsites.  For example iomap still does not flush the plug
> in your patch.

It is reasonable for flush user(usually submission) to be responsible
for finishing/flushing plug.

iomap is one good example to show this point, since it does flush the plug
before call bio_poll(), see __iomap_dio_rw().

> 
> > > 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.
> 
> Ok, makes sense.
> 
> > > 
> > > > +			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'.
> 
> It doesn't.  Just look at the conditional you replaced for example :)

OK, I will change to your style.

> 
> > > +	/*
> > > +	 * 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.
> 
> True.  So I guess we'll need a new flag to distinguish the cases.

If there will be more such kind of poll usage in kernel, I think it is fine
to add the flag, but so far all the three aren't used very often.


Thanks,
Ming