On 4/8/23 08:58, Bart Van Assche wrote:
> If a queue is run before all requeued requests have been sent to the I/O
> scheduler, the I/O scheduler may dispatch the wrong request. Fix this by
> making __blk_mq_run_hw_queue() process the requeue_list instead of
> blk_mq_requeue_work().
I think that the part of patch 5 that move the requeue work to per hctx should
be together with this patch. That would make the review easier.
I am guessing that the move to per hctx is to try to reduce lock contention ?
That is not clearly explained. Given that requeue events should be infrequent
exceptions, is that really necessary ?
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Cc: Ming Lei <ming.lei@xxxxxxxxxx>
> Cc: Mike Snitzer <snitzer@xxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> block/blk-mq.c | 35 ++++++++++-------------------------
> include/linux/blk-mq.h | 1 -
> 2 files changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index deb3d08a6b26..562868dff43f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -64,6 +64,7 @@ static inline blk_qc_t blk_rq_to_qc(struct request *rq)
> static bool blk_mq_hctx_has_pending(struct blk_mq_hw_ctx *hctx)
> {
> return !list_empty_careful(&hctx->dispatch) ||
> + !list_empty_careful(&hctx->requeue_list) ||
> sbitmap_any_bit_set(&hctx->ctx_map) ||
> blk_mq_sched_has_work(hctx);
> }
> @@ -1409,10 +1410,8 @@ void blk_mq_requeue_request(struct request *rq, bool kick_requeue_list)
> }
> EXPORT_SYMBOL(blk_mq_requeue_request);
>
> -static void blk_mq_requeue_work(struct work_struct *work)
> +static void blk_mq_process_requeue_list(struct blk_mq_hw_ctx *hctx)
> {
> - struct blk_mq_hw_ctx *hctx =
> - container_of(work, struct blk_mq_hw_ctx, requeue_work.work);
> LIST_HEAD(rq_list);
> struct request *rq, *next;
>
> @@ -1437,8 +1436,6 @@ static void blk_mq_requeue_work(struct work_struct *work)
> list_del_init(&rq->queuelist);
> blk_mq_sched_insert_request(rq, false, false, false);
> }
> -
> - blk_mq_run_hw_queue(hctx, false);
> }
>
> void blk_mq_add_to_requeue_list(struct request *rq, bool at_head,
> @@ -1464,30 +1461,19 @@ void blk_mq_add_to_requeue_list(struct request *rq, bool at_head,
> spin_unlock_irqrestore(&hctx->requeue_lock, flags);
>
> if (kick_requeue_list)
> - blk_mq_kick_requeue_list(rq->q);
> + blk_mq_run_hw_queue(hctx, /*async=*/true);
> }
>
> void blk_mq_kick_requeue_list(struct request_queue *q)
> {
> - struct blk_mq_hw_ctx *hctx;
> - unsigned long i;
> -
> - queue_for_each_hw_ctx(q, hctx, i)
> - kblockd_mod_delayed_work_on(WORK_CPU_UNBOUND,
> - &hctx->requeue_work, 0);
> + blk_mq_run_hw_queues(q, true);
> }
> EXPORT_SYMBOL(blk_mq_kick_requeue_list);
>
> void blk_mq_delay_kick_requeue_list(struct request_queue *q,
> unsigned long msecs)
> {
> - struct blk_mq_hw_ctx *hctx;
> - unsigned long i;
> -
> - queue_for_each_hw_ctx(q, hctx, i)
> - kblockd_mod_delayed_work_on(WORK_CPU_UNBOUND,
> - &hctx->requeue_work,
> - msecs_to_jiffies(msecs));
> + blk_mq_delay_run_hw_queues(q, msecs);
> }
> EXPORT_SYMBOL(blk_mq_delay_kick_requeue_list);
>
> @@ -2146,6 +2132,8 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
> */
> WARN_ON_ONCE(in_interrupt());
>
> + blk_mq_process_requeue_list(hctx);
> +
> blk_mq_run_dispatch_ops(hctx->queue,
> blk_mq_sched_dispatch_requests(hctx));
> }
> @@ -2317,7 +2305,7 @@ void blk_mq_run_hw_queues(struct request_queue *q, bool async)
> * scheduler.
> */
> if (!sq_hctx || sq_hctx == hctx ||
> - !list_empty_careful(&hctx->dispatch))
> + blk_mq_hctx_has_pending(hctx))
> blk_mq_run_hw_queue(hctx, async);
> }
> }
> @@ -2353,7 +2341,7 @@ void blk_mq_delay_run_hw_queues(struct request_queue *q, unsigned long msecs)
> * scheduler.
> */
> if (!sq_hctx || sq_hctx == hctx ||
> - !list_empty_careful(&hctx->dispatch))
> + blk_mq_hctx_has_pending(hctx))
> blk_mq_delay_run_hw_queue(hctx, msecs);
> }
> }
> @@ -3608,7 +3596,6 @@ static int blk_mq_init_hctx(struct request_queue *q,
> struct blk_mq_tag_set *set,
> struct blk_mq_hw_ctx *hctx, unsigned hctx_idx)
> {
> - INIT_DELAYED_WORK(&hctx->requeue_work, blk_mq_requeue_work);
> INIT_LIST_HEAD(&hctx->requeue_list);
> spin_lock_init(&hctx->requeue_lock);
>
> @@ -4771,10 +4758,8 @@ void blk_mq_cancel_work_sync(struct request_queue *q)
> struct blk_mq_hw_ctx *hctx;
> unsigned long i;
>
> - queue_for_each_hw_ctx(q, hctx, i) {
> - cancel_delayed_work_sync(&hctx->requeue_work);
> + queue_for_each_hw_ctx(q, hctx, i)
> cancel_delayed_work_sync(&hctx->run_work);
> - }
> }
>
> static int __init blk_mq_init(void)
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 0157f1569980..e62feb17af96 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -313,7 +313,6 @@ struct blk_mq_hw_ctx {
>
> struct list_head requeue_list;
> spinlock_t requeue_lock;
> - struct delayed_work requeue_work;
>
> /**
> * @run_work: Used for scheduling a hardware queue run at a later time.
