Re: [PATCH 3/4] blk-mq: remove a pointless queue enter pair in blk_mq_alloc_request_hctx

[Bart Van Assche <bvanassche@acm.org>]

On 2020-05-16 08:34, Christoph Hellwig wrote:
> No need for two queue references.  Also reduce the q_usage_counter
> critical section to just the actual request allocation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d96d3931f33e6..69e58cc4244c0 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -439,26 +439,20 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
>  	if (hctx_idx >= q->nr_hw_queues)
>  		return ERR_PTR(-EIO);
>  
> -	ret = blk_queue_enter(q, flags);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>  	/*
>  	 * Check if the hardware context is actually mapped to anything.
>  	 * If not tell the caller that it should skip this queue.
>  	 */
>  	alloc_data.hctx = q->queue_hw_ctx[hctx_idx];
> -	if (!blk_mq_hw_queue_mapped(alloc_data.hctx)) {
> -		blk_queue_exit(q);
> +	if (!blk_mq_hw_queue_mapped(alloc_data.hctx))
>  		return ERR_PTR(-EXDEV);
> -	}
>  	cpu = cpumask_first_and(alloc_data.hctx->cpumask, cpu_online_mask);
>  	alloc_data.ctx = __blk_mq_get_ctx(q, cpu);
>  
> -	blk_queue_enter_live(q);
> +	ret = blk_queue_enter(q, flags);
> +	if (ret)
> +		return ERR_PTR(ret);
>  	rq = blk_mq_get_request(q, NULL, &alloc_data);
> -	blk_queue_exit(q);
> -
>  	if (!rq) {
>  		blk_queue_exit(q);
>  		return ERR_PTR(-EWOULDBLOCK);

This change looks wrong to me. blk_mq_update_nr_hw_queues() modifies
q->queue_hw_ctx so q_usage_counter must be incremented before that
pointer is dereferenced.

Bart.