Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

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

On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:
> On 11/12/21 5:47 AM, Ming Lei wrote:
> > On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
> >> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> >>>> can only be used for reads, and no fua can be set if the preallocating
> >>>> I/O didn't use fua, etc.
> >>>>
> >>>> What are the pitfalls of just chanigng cmd_flags?
> >>>
> >>> Then we need to check cmd_flags carefully, such as hctx->type has to
> >>> be same, flush & passthrough flags has to be same, that said all
> >>> ->cmd_flags used for allocating rqs have to be same with the following
> >>> bio->bi_opf.
> >>>
> >>> In usual cases, I guess all IOs submitted from same plug batch should be
> >>> same type. If not, we can switch to change cmd_flags.
> >>
> >> Jens: is this a limit fitting into your use cases?
> >>
> >> I guess as a quick fix this rejecting different flags is probably the
> >> best we can do for now, but I suspect we'll want to eventually relax
> >> them.
> > 
> > rw mixed workload will be affected, so I think we need to switch to
> > change cmd_flags, how about the following patch?
> > 
> > From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
> > From: Ming Lei <ming.lei@redhat.com>
> > Date: Fri, 12 Nov 2021 08:14:38 +0800
> > Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
> > 
> > submit_bio_checks() may update bio->bi_opf, so we have to initialize
> > blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
> > returns when allocating new request.
> > 
> > In case of using cached request, fallback to allocate new request if
> > cached rq isn't compatible with the incoming bio, otherwise change
> > rq->cmd_flags with incoming bio->bi_opf.
> > 
> > Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
> >  block/blk-mq.h | 26 +++++++++++++++-----------
> >  2 files changed, 45 insertions(+), 20 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index f511db395c7f..3ab34c4f20da 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >  	};
> >  	struct request *rq;
> >  
> > -	if (unlikely(bio_queue_enter(bio)))
> > -		return NULL;
> > -	if (unlikely(!submit_bio_checks(bio)))
> > -		goto put_exit;
> >  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
> > -		goto put_exit;
> > +		return NULL;
> >  
> >  	rq_qos_throttle(q, bio);
> >  
> > @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >  	rq_qos_cleanup(q, bio);
> >  	if (bio->bi_opf & REQ_NOWAIT)
> >  		bio_wouldblock_error(bio);
> > -put_exit:
> > -	blk_queue_exit(q);
> > +
> >  	return NULL;
> >  }
> >  
> > +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
> > +		struct bio *bio)
> > +{
> > +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
> > +		return false;
> > +
> > +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
> > +		return false;
> > +
> > +	return true;
> 
> I think we should just check if hctx is the same, that should be enough.
> We don't need to match the type, just disallow if hw queue has changed.

But bio doesn't have hw queue. Figuring out exact hw queue seems
necessary and needs more cpu cycles than getting hctx type.


Thanks,
Ming