Re: [RFC PATCH V2 05/13] block: add req flag of REQ_TAG

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

On Fri, Mar 19, 2021 at 03:59:06PM +0800, JeffleXu wrote:
> 
> 
> On 3/19/21 12:48 AM, Ming Lei wrote:
> > Add one req flag REQ_TAG which will be used in the following patch for
> > supporting bio based IO polling.
> > 
> > Exactly this flag can help us to do:
> > 
> > 1) request flag is cloned in bio_fast_clone(), so if we mark one FS bio
> > as REQ_TAG, all bios cloned from this FS bio will be marked as REQ_TAG.
> > 
> > 2)create per-task io polling context if the bio based queue supports polling
> > and the submitted bio is HIPRI. This per-task io polling context will be
> > created during submit_bio() before marking this HIPRI bio as REQ_TAG. Then
> > we can avoid to create such io polling context if one cloned bio with REQ_TAG
> > is submitted from another kernel context.
> > 
> > 3) for supporting bio based io polling, we need to poll IOs from all
> > underlying queues of bio device/driver, this way help us to recognize which
> > IOs need to polled in bio based style, which will be implemented in next
> > patch.
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-core.c          | 29 +++++++++++++++++++++++++++--
> >  include/linux/blk_types.h |  4 ++++
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 0b00c21cbefb..efc7a61a84b4 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -840,11 +840,30 @@ static inline bool blk_queue_support_bio_poll(struct request_queue *q)
> >  static inline void blk_bio_poll_preprocess(struct request_queue *q,
> >  		struct bio *bio)
> >  {
> > +	bool mq;
> > +
> >  	if (!(bio->bi_opf & REQ_HIPRI))
> >  		return;
> >  
> > -	if (!blk_queue_poll(q) || (!queue_is_mq(q) && !blk_get_bio_poll_ctx()))
> > +	/*
> > +	 * Can't support bio based IO poll without per-task poll queue
> > +	 *
> > +	 * Now we have created per-task io poll context, and mark this
> > +	 * bio as REQ_TAG, so: 1) if any cloned bio from this bio is
> > +	 * submitted from another kernel context, we won't create bio
> > +	 * poll context for it, so that bio will be completed by IRQ;
> > +	 * 2) If such bio is submitted from current context, we will
> > +	 * complete it via blk_poll(); 3) If driver knows that one
> > +	 * underlying bio allocated from driver is for FS bio, meantime
> > +	 * it is submitted in current context, driver can mark such bio
> > +	 * as REQ_TAG manually, so the bio can be completed via blk_poll
> > +	 * too.
> > +	 */
> 
> Sorry I can't understand case 3, could you please further explain it? If

I meant driver may allocate bio and submit it in current context, and this
allocated bio is for completing FS hipri bio too. So far, HIPRI won't be
set for this bio, but driver may mark this bio as HIPRI and TAG, so this
created bio can be polled.

> 'driver marks such bio as REQ_TAG manually', then per-task io poll
> context won't be created, and thus REQ_HIPRI will be cleared, in which
> case the bio will be completed by IRQ. How could it be completed by
> blk_poll()?

The io poll context is created when FS HIPRI bio on based queue(DM) is
submitted, at that time, bio based driver's ->submit_bio isn't called
yet. So when bio driver's ->submit_bio() allocates new bios and submits
them in current context, if driver marks this bio as HIPRI and TAG, they
can be polled too.

> 
> 
> > +	mq = queue_is_mq(q);
> > +	if (!blk_queue_poll(q) || (!mq && !blk_get_bio_poll_ctx()))
> >  		bio->bi_opf &= ~REQ_HIPRI;
> 
> 
> 
> 
> If the use cases are mixed, saying one kernel context may submit IO with
> and without REQ_TAG at the meantime (though I don't know if this
> situation exists), then the above code may not work as we expect.

Poll context shouldn't be created for kernel context.

So far, this patch won't cover bios submitted from kernel context, and
for any bios submitted from kernel context, their HIPRI will be cleared.

> 
> For example, dm-XXX will return DM_MAPIO_SUBMITTED and actually submits
> the cloned bio (with REQ_TAG) with internal kernel threads. Besides,
> dm-XXX will also allocate bio (without REQ_TAG) of itself for metadata
> logging or something. When submitting bios (without REQ_TAG), per-task

But HIPRI won't be set for this allocated bio.

> io poll context will be allocated. Later when submitting cloned bios
> (with REQ_TAG), the poll context already exists and thus REQ_HIPRI is
> kept for these bios and they are submitted to polling hw queues.

Originally I planed to add new helper of submit_poll_bio() for current
HIPRI uses, and only create poll context in this code path, this way can
decouple REQ_TAG a bit. But looks it is enough to re-use REQ_TAG for this
purpose. If not, it can be quite easy to address issue wrt. creating poll
context.


Thanks, 
Ming