Re: [PATCH] blk-mq: make sure active queue usage is held for bio_integrity_prep()

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

On Wed, Nov 08, 2023 at 11:30:10PM -0800, Christoph Hellwig wrote:
> > +++ b/block/blk-mq.c
> > @@ -2858,11 +2858,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 (blk_mq_attempt_bio_merge(q, bio, nsegs))
> > -		goto queue_exit;
> > +		return NULL;
> >  
> >  	rq_qos_throttle(q, bio);
> 
> For clarity splitting this out might be a bit more readable.

I'd rather not do too many things in single fix, which need backport,
but I am fine to do it in following cleanup.

> 
> > +static inline struct request *blk_mq_cached_req(const struct request_queue *q,
> > +		const struct blk_plug *plug)
> > +{
> > +	if (plug) {
> > +		struct request *rq = rq_list_peek(&plug->cached_rq);
> > +
> > +		if (rq && rq->q == q)
> > +			return rq;
> > +	}
> > +	return NULL;
> > +}
> 
> Not sure this helper adds much value over just open coding it.

I am fine with either way, but the function name actually
has document benefit.

> 
> > +/* return true if this bio needs to handle by allocating new request */
> > +static inline bool blk_mq_try_cached_rq(struct request *rq,
> >  		struct blk_plug *plug, struct bio **bio, unsigned int nsegs)
> 
> The polarity here is a bit weird, I'd expect a true return if the
> request can be used and a naming implying that.

Fine, my original local version actually returns false if cached req can
be used, and it is changed just for not checking
!blk_mq_try_cached_rq().

> 
> > +	rq = blk_mq_cached_req(q, plug);
> > +	if (rq) {
> > +		/* cached request held queue usage counter */
> > +		if (!bio_integrity_prep(bio))
> > +			return;
> > +
> > +		need_alloc = blk_mq_try_cached_rq(rq, plug, &bio, nr_segs);
> >  		if (!bio)
> >  			return;
> 
> If you take the blk_mq_attempt_bio_merge merge out of the helper,
> the calling convention becomes much saner.

IMO we shouldn't mix cleanup with fix and it is friendly to take
stable backport into account.

> 
> > +			/* cached request held queue usage counter */
> > +			percpu_ref_get(&q->q_usage_counter);
> 
> I don't think we can just do the percpu_ref_get here.  While getting
> the counter obviosuly can't fail, we still need to do the queue
> pm only check.

blk_pre_runtime_suspend() can't succeed if any active queue usage
counter is held, so it is fine to call percpu_ref_get() here.

> 
> Below is the untested version of how I'd do this that I hacked while
> reviewing it:
> 
> ---
> commit 1134ce39504c30a9804ed8147027e66bf7d3157e
> Author: Ming Lei <ming.lei@redhat.com>
> Date:   Wed Nov 8 16:05:04 2023 +0800
> 
>     blk-mq: make sure active queue usage is held for bio_integrity_prep()
>     
>     blk_integrity_unregister() can come if queue usage counter isn't held
>     for one bio with integrity prepared, so this request may be completed with
>     calling profile->complete_fn, then kernel panic.
>     
>     Another constraint is that bio_integrity_prep() needs to be called
>     before bio merge.
>     
>     Fix the issue by:
>     
>     - call bio_integrity_prep() with one queue usage counter grabbed reliably
>     
>     - call bio_integrity_prep() before bio merge
>     
>     Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
>     Reported-by: Yi Zhang <yi.zhang@redhat.com>
>     Signed-off-by: Ming Lei <ming.lei@redhat.com>
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e2d11183f62e37..b758dc8ed10957 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2858,11 +2858,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 (blk_mq_attempt_bio_merge(q, bio, nsegs))
> -		goto queue_exit;
> +		return NULL;
>  
>  	rq_qos_throttle(q, bio);
>  
> @@ -2878,35 +2875,24 @@ 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);
> -queue_exit:
> -	blk_queue_exit(q);
>  	return NULL;
>  }
>  
> -static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
> -		struct blk_plug *plug, struct bio **bio, unsigned int nsegs)
> +/* return true if this @rq can be used for @bio */
> +static bool blk_mq_can_use_cached_rq(struct request *rq, struct blk_plug *plug,
> +		struct bio *bio)

Switching to "*bio" is in my todo cleanup list too, and I will make it
standalone patch.

>  {
> -	struct request *rq;
> -	enum hctx_type type, hctx_type;
> +	struct request_queue *q = rq->q;
> +	enum hctx_type type = blk_mq_get_hctx_type(bio->bi_opf);
> +	enum hctx_type hctx_type = rq->mq_hctx->type;
>  
> -	if (!plug)
> -		return NULL;
> -	rq = rq_list_peek(&plug->cached_rq);
> -	if (!rq || rq->q != q)
> -		return NULL;
> +	WARN_ON_ONCE(rq_list_peek(&plug->cached_rq) != rq);
>  
> -	if (blk_mq_attempt_bio_merge(q, *bio, nsegs)) {
> -		*bio = NULL;
> -		return NULL;
> -	}
> -
> -	type = blk_mq_get_hctx_type((*bio)->bi_opf);
> -	hctx_type = rq->mq_hctx->type;
>  	if (type != hctx_type &&
>  	    !(type == HCTX_TYPE_READ && hctx_type == HCTX_TYPE_DEFAULT))
> -		return NULL;
> -	if (op_is_flush(rq->cmd_flags) != op_is_flush((*bio)->bi_opf))
> -		return NULL;
> +		return false;
> +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
> +		return false;
>  
>  	/*
>  	 * If any qos ->throttle() end up blocking, we will have flushed the
> @@ -2914,12 +2900,12 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
>  	 * before we throttle.
>  	 */
>  	plug->cached_rq = rq_list_next(rq);
> -	rq_qos_throttle(q, *bio);
> +	rq_qos_throttle(q, bio);
>  
>  	blk_mq_rq_time_init(rq, 0);
> -	rq->cmd_flags = (*bio)->bi_opf;
> +	rq->cmd_flags = bio->bi_opf;
>  	INIT_LIST_HEAD(&rq->queuelist);
> -	return rq;
> +	return true;
>  }
>  
>  static void bio_set_ioprio(struct bio *bio)
> @@ -2949,7 +2935,7 @@ void blk_mq_submit_bio(struct bio *bio)
>  	struct blk_plug *plug = blk_mq_plug(bio);
>  	const int is_sync = op_is_sync(bio->bi_opf);
>  	struct blk_mq_hw_ctx *hctx;
> -	struct request *rq;
> +	struct request *rq = NULL;
>  	unsigned int nr_segs = 1;
>  	blk_status_t ret;
>  
> @@ -2960,20 +2946,39 @@ void blk_mq_submit_bio(struct bio *bio)
>  			return;
>  	}
>  
> -	if (!bio_integrity_prep(bio))
> -		return;
> -
>  	bio_set_ioprio(bio);
>  
> -	rq = blk_mq_get_cached_request(q, plug, &bio, nr_segs);
> -	if (!rq) {
> -		if (!bio)
> +	if (plug) {
> +		rq = rq_list_peek(&plug->cached_rq);
> +		if (rq && rq->q != q)
> +			rq = NULL;
> +	}
> +	if (rq) {
> +		/* rq already holds a q_usage_counter reference */
> +		if (!bio_integrity_prep(bio))
>  			return;
> -		rq = blk_mq_get_new_requests(q, plug, bio, nr_segs);
> -		if (unlikely(!rq))
> +		if (blk_mq_attempt_bio_merge(q, bio, nr_segs))
> +			return;
> +
> +		if (blk_mq_can_use_cached_rq(rq, plug, bio))
> +			goto done;
> +
> +		percpu_ref_get(&q->q_usage_counter);
> +	} else {
> +		if (unlikely(bio_queue_enter(bio)))
> +			return;
> +
> +		if (!bio_integrity_prep(bio))
>  			return;

blk_queue_exit() is needed if bio_integrity_prep() fails.

I will try to make one easy fix first, then follows cleanup.

Thanks, 
Ming