Re: [PATCH] blk-mq: fix issue with shared tag queue re-running

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

On Wed, Nov 08, 2017 at 03:48:51PM -0700, Jens Axboe wrote:
> This patch attempts to make the case of hctx re-running on driver tag
> failure more robust. Without this patch, it's pretty easy to trigger a
> stall condition with shared tags. An example is using null_blk like
> this:
> 
> modprobe null_blk queue_mode=2 nr_devices=4 shared_tags=1 submit_queues=1 hw_queue_depth=1
> 
> which sets up 4 devices, sharing the same tag set with a depth of 1.
> Running a fio job ala:
> 
> [global]
> bs=4k
> rw=randread
> norandommap
> direct=1
> ioengine=libaio
> iodepth=4
> 
> [nullb0]
> filename=/dev/nullb0
> [nullb1]
> filename=/dev/nullb1
> [nullb2]
> filename=/dev/nullb2
> [nullb3]
> filename=/dev/nullb3
> 
> will inevitably end with one or more threads being stuck waiting for a
> scheduler tag. That IO is then stuck forever, until someone else
> triggers a run of the queue.
> 
> Ensure that we always re-run the hardware queue, if the driver tag we
> were waiting for got freed before we added our leftover request entries
> back on the dispatch list.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 7f4a1ba532af..bb7f08415203 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -179,7 +179,6 @@ static const char *const hctx_state_name[] = {
>  	HCTX_STATE_NAME(STOPPED),
>  	HCTX_STATE_NAME(TAG_ACTIVE),
>  	HCTX_STATE_NAME(SCHED_RESTART),
> -	HCTX_STATE_NAME(TAG_WAITING),
>  	HCTX_STATE_NAME(START_ON_RUN),
>  };
>  #undef HCTX_STATE_NAME
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3d759bb8a5bb..8dc5db40df9d 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -998,49 +998,64 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
>  	return rq->tag != -1;
>  }
>  
> -static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode, int flags,
> -				void *key)
> +static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode,
> +				int flags, void *key)
>  {
>  	struct blk_mq_hw_ctx *hctx;
>  
>  	hctx = container_of(wait, struct blk_mq_hw_ctx, dispatch_wait);
>  
> -	list_del(&wait->entry);
> -	clear_bit_unlock(BLK_MQ_S_TAG_WAITING, &hctx->state);
> +	list_del_init(&wait->entry);
>  	blk_mq_run_hw_queue(hctx, true);
>  	return 1;
>  }
>  
> -static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx *hctx)
> +static bool blk_mq_dispatch_wait_add(struct blk_mq_hw_ctx **hctx,
> +				     struct request *rq)
>  {
> +	struct blk_mq_hw_ctx *this_hctx = *hctx;
> +	wait_queue_entry_t *wait = &this_hctx->dispatch_wait;
>  	struct sbq_wait_state *ws;
>  
> +	if (!list_empty_careful(&wait->entry))
> +		return false;
> +
> +	spin_lock(&this_hctx->lock);
> +	if (!list_empty(&wait->entry)) {
> +		spin_unlock(&this_hctx->lock);
> +		return false;
> +	}
> +
> +	ws = bt_wait_ptr(&this_hctx->tags->bitmap_tags, this_hctx);
> +	add_wait_queue(&ws->wait, wait);
> +
>  	/*
> -	 * The TAG_WAITING bit serves as a lock protecting hctx->dispatch_wait.
> -	 * The thread which wins the race to grab this bit adds the hardware
> -	 * queue to the wait queue.
> +	 * It's possible that a tag was freed in the window between the
> +	 * allocation failure and adding the hardware queue to the wait
> +	 * queue.
>  	 */
> -	if (test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state) ||
> -	    test_and_set_bit_lock(BLK_MQ_S_TAG_WAITING, &hctx->state))
> +	if (!blk_mq_get_driver_tag(rq, hctx, false)) {
> +		spin_unlock(&this_hctx->lock);
>  		return false;
> -
> -	init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
> -	ws = bt_wait_ptr(&hctx->tags->bitmap_tags, hctx);
> +	}
>  
>  	/*
> -	 * As soon as this returns, it's no longer safe to fiddle with
> -	 * hctx->dispatch_wait, since a completion can wake up the wait queue
> -	 * and unlock the bit.
> +	 * We got a tag, remove outselves from the wait queue to ensure
> +	 * someone else gets the wakeup.
>  	 */
> -	add_wait_queue(&ws->wait, &hctx->dispatch_wait);
> +	spin_lock_irq(&ws->wait.lock);
> +	list_del_init(&wait->entry);
> +	spin_unlock_irq(&ws->wait.lock);
> +	spin_unlock(&this_hctx->lock);
>  	return true;
>  }
>  
>  bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> -		bool got_budget)
> +			     bool got_budget)
>  {
>  	struct blk_mq_hw_ctx *hctx;
>  	struct request *rq, *nxt;
> +	bool no_tag = false;
>  	int errors, queued;
>  
>  	if (list_empty(list))
> @@ -1060,22 +1075,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  		if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
>  			/*
>  			 * The initial allocation attempt failed, so we need to
> -			 * rerun the hardware queue when a tag is freed.
> +			 * rerun the hardware queue when a tag is freed. The
> +			 * waitqueue takes care of that. If the queue is run
> +			 * before we add this entry back on the dispatch list,
> +			 * we'll re-run it below.
>  			 */
> -			if (!blk_mq_dispatch_wait_add(hctx)) {
> -				if (got_budget)
> -					blk_mq_put_dispatch_budget(hctx);
> -				break;
> -			}
> -
> -			/*
> -			 * It's possible that a tag was freed in the window
> -			 * between the allocation failure and adding the
> -			 * hardware queue to the wait queue.
> -			 */
> -			if (!blk_mq_get_driver_tag(rq, &hctx, false)) {
> +			if (!blk_mq_dispatch_wait_add(&hctx, rq)) {
>  				if (got_budget)
>  					blk_mq_put_dispatch_budget(hctx);
> +				no_tag = true;
>  				break;
>  			}
>  		}
> @@ -1140,10 +1148,10 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  		 * it is no longer set that means that it was cleared by another
>  		 * thread and hence that a queue rerun is needed.
>  		 *
> -		 * If TAG_WAITING is set that means that an I/O scheduler has
> -		 * been configured and another thread is waiting for a driver
> -		 * tag. To guarantee fairness, do not rerun this hardware queue
> -		 * but let the other thread grab the driver tag.
> +		 * If 'no_tag' is set, that means that we failed getting
> +		 * a driver tag with an I/O scheduler attached. If our dispatch
> +		 * waitqueue is no longer active, ensure that we run the queue
> +		 * AFTER adding our entries back to the list.
>  		 *
>  		 * If no I/O scheduler has been configured it is possible that
>  		 * the hardware queue got stopped and restarted before requests
> @@ -1156,7 +1164,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  		 *   and dm-rq.
>  		 */
>  		if (!blk_mq_sched_needs_restart(hctx) &&
> -		    !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state))
> +		    (no_tag && list_empty_careful(&hctx->dispatch_wait.entry)))
>  			blk_mq_run_hw_queue(hctx, true);

If one rq is just completed after the check on list_empty_careful(&hctx->dispatch_wait.entry),
the queue may not be run any more. May that be an issue?

>  	}
>  
> @@ -2020,6 +2028,9 @@ static int blk_mq_init_hctx(struct request_queue *q,
>  
>  	hctx->nr_ctx = 0;
>  
> +	init_waitqueue_func_entry(&hctx->dispatch_wait, blk_mq_dispatch_wake);
> +	INIT_LIST_HEAD(&hctx->dispatch_wait.entry);
> +
>  	if (set->ops->init_hctx &&
>  	    set->ops->init_hctx(hctx, set->driver_data, hctx_idx))
>  		goto free_bitmap;
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 674641527da7..4ae987c2352c 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -35,7 +35,7 @@ struct blk_mq_hw_ctx {
>  	struct blk_mq_ctx	**ctxs;
>  	unsigned int		nr_ctx;
>  
> -	wait_queue_entry_t		dispatch_wait;
> +	wait_queue_entry_t	dispatch_wait;
>  	atomic_t		wait_index;
>  
>  	struct blk_mq_tags	*tags;
> @@ -181,8 +181,7 @@ enum {
>  	BLK_MQ_S_STOPPED	= 0,
>  	BLK_MQ_S_TAG_ACTIVE	= 1,
>  	BLK_MQ_S_SCHED_RESTART	= 2,
> -	BLK_MQ_S_TAG_WAITING	= 3,
> -	BLK_MQ_S_START_ON_RUN	= 4,
> +	BLK_MQ_S_START_ON_RUN	= 3,
>  
>  	BLK_MQ_MAX_DEPTH	= 10240,

Looks the approach is smart, and effective, since requests are often
completed at batch. No regression on scsi test too.

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming