Re: [PATCH v25 07/20] block/mq-deadline: Enable zoned write pipelining

[Damien Le Moal <dlemoal@kernel.org>]

On 2025/10/15 6:54, Bart Van Assche wrote:
> The hwq selected by blk_mq_run_hw_queues() for single-queue I/O schedulers
> depends on the CPU core that function has been called from. This may lead
> to concurrent dispatching of I/O requests on different CPU cores and hence
> may cause I/O reordering. Prevent as follows that zoned writes are
> reordered:
> - Set the ELEVATOR_FLAG_SUPPORTS_ZONED_WRITE_PIPELINING flag. This disables
>   the single hwq optimization in the block layer core.
> - Modify dd_has_work() such that it only reports that any work is pending
>   for zoned writes if the zoned writes have been submitted to the hwq that
>   has been passed as argument to dd_has_work().
> - Modify dd_dispatch_request() such that it only dispatches zoned writes
>   if the hwq argument passed to this function matches the hwq of the
>   pending zoned writes.

One of the goals of zone write plugging was to remove the dependence on IO
schedulers to control the ordering of write commands to zoned block devices.
Such change are going backward and I do not like that. What if the user sets
Kyber or bfq with your zone write pipelining ? Does it break ?

From the very light explanation above, it seems to me that what you are trying
to do can be generic in the block layer and leave mq-deadline untouched.

> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/mq-deadline.c | 68 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index 0a46d0f06f72..be6ed3d8fa36 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -319,11 +319,25 @@ static struct request *dd_start_request(struct deadline_data *dd,
>  	return rq;
>  }
>  
> +/*
> + * If write pipelining is enabled, only dispatch sequential zoned writes if
> + * rq->mq_hctx == hctx.
> + */
> +static bool dd_dispatch_from_hctx(struct blk_mq_hw_ctx *hctx,
> +				  struct request *rq)
> +{
> +	struct request_queue *q = hctx->queue;
> +
> +	return !(q->limits.features & BLK_FEAT_ORDERED_HWQ) ||
> +		rq->mq_hctx == hctx || !blk_rq_is_seq_zoned_write(rq);
> +}
> +
>  /*
>   * deadline_dispatch_requests selects the best request according to
>   * read/write expire, fifo_batch, etc and with a start time <= @latest_start.
>   */
>  static struct request *__dd_dispatch_request(struct deadline_data *dd,
> +					     struct blk_mq_hw_ctx *hctx,
>  					     struct dd_per_prio *per_prio,
>  					     unsigned long latest_start)
>  {
> @@ -336,7 +350,8 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>  	 * batches are currently reads XOR writes
>  	 */
>  	rq = deadline_next_request(dd, per_prio, dd->last_dir);
> -	if (rq && dd->batching < dd->fifo_batch) {
> +	if (rq && dd->batching < dd->fifo_batch &&
> +	    dd_dispatch_from_hctx(hctx, rq)) {
>  		/* we have a next request and are still entitled to batch */
>  		data_dir = rq_data_dir(rq);
>  		goto dispatch_request;
> @@ -396,7 +411,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>  		rq = next_rq;
>  	}
>  
> -	if (!rq)
> +	if (!rq || !dd_dispatch_from_hctx(hctx, rq))
>  		return NULL;
>  
>  	dd->last_dir = data_dir;
> @@ -418,8 +433,9 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>   * Check whether there are any requests with priority other than DD_RT_PRIO
>   * that were inserted more than prio_aging_expire jiffies ago.
>   */
> -static struct request *dd_dispatch_prio_aged_requests(struct deadline_data *dd,
> -						      unsigned long now)
> +static struct request *
> +dd_dispatch_prio_aged_requests(struct deadline_data *dd,
> +			       struct blk_mq_hw_ctx *hctx, unsigned long now)
>  {
>  	struct request *rq;
>  	enum dd_prio prio;
> @@ -433,7 +449,7 @@ static struct request *dd_dispatch_prio_aged_requests(struct deadline_data *dd,
>  		return NULL;
>  
>  	for (prio = DD_BE_PRIO; prio <= DD_PRIO_MAX; prio++) {
> -		rq = __dd_dispatch_request(dd, &dd->per_prio[prio],
> +		rq = __dd_dispatch_request(dd, hctx, &dd->per_prio[prio],
>  					   now - dd->prio_aging_expire);
>  		if (rq)
>  			return rq;
> @@ -466,7 +482,7 @@ static struct request *dd_dispatch_request(struct blk_mq_hw_ctx *hctx)
>  		goto unlock;
>  	}
>  
> -	rq = dd_dispatch_prio_aged_requests(dd, now);
> +	rq = dd_dispatch_prio_aged_requests(dd, hctx, now);
>  	if (rq)
>  		goto unlock;
>  
> @@ -475,7 +491,7 @@ static struct request *dd_dispatch_request(struct blk_mq_hw_ctx *hctx)
>  	 * requests if any higher priority requests are pending.
>  	 */
>  	for (prio = 0; prio <= DD_PRIO_MAX; prio++) {
> -		rq = __dd_dispatch_request(dd, &dd->per_prio[prio], now);
> +		rq = __dd_dispatch_request(dd, hctx, &dd->per_prio[prio], now);
>  		if (rq || dd_queued(dd, prio))
>  			break;
>  	}
> @@ -575,6 +591,8 @@ static int dd_init_sched(struct request_queue *q, struct elevator_queue *eq)
>  	/* We dispatch from request queue wide instead of hw queue */
>  	blk_queue_flag_set(QUEUE_FLAG_SQ_SCHED, q);
>  
> +	set_bit(ELEVATOR_FLAG_SUPPORTS_ZONED_WRITE_PIPELINING, &eq->flags);
> +
>  	q->elevator = eq;
>  	dd_depth_updated(q);
>  	return 0;
> @@ -731,10 +749,40 @@ static void dd_finish_request(struct request *rq)
>  		atomic_inc(&per_prio->stats.completed);
>  }
>  
> -static bool dd_has_work_for_prio(struct dd_per_prio *per_prio)
> +/* May be called from interrupt context. */
> +static bool dd_has_write_work(struct deadline_data *dd,
> +			      struct blk_mq_hw_ctx *hctx,
> +			      struct list_head *list)
> +{
> +	struct request_queue *q = hctx->queue;
> +	unsigned long flags;
> +	struct request *rq;
> +	bool has_work = false;
> +
> +	if (list_empty_careful(list))
> +		return false;
> +
> +	if (!(q->limits.features & BLK_FEAT_ORDERED_HWQ))
> +		return true;
> +
> +	spin_lock_irqsave(&dd->lock, flags);
> +	list_for_each_entry(rq, list, queuelist) {
> +		if (rq->mq_hctx == hctx) {
> +			has_work = true;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&dd->lock, flags);
> +
> +	return has_work;
> +}
> +
> +static bool dd_has_work_for_prio(struct deadline_data *dd,
> +				 struct blk_mq_hw_ctx *hctx,
> +				 struct dd_per_prio *per_prio)
>  {
>  	return !list_empty_careful(&per_prio->fifo_list[DD_READ]) ||
> -		!list_empty_careful(&per_prio->fifo_list[DD_WRITE]);
> +		dd_has_write_work(dd, hctx, &per_prio->fifo_list[DD_WRITE]);
>  }
>  
>  static bool dd_has_work(struct blk_mq_hw_ctx *hctx)
> @@ -746,7 +794,7 @@ static bool dd_has_work(struct blk_mq_hw_ctx *hctx)
>  		return true;
>  
>  	for (prio = 0; prio <= DD_PRIO_MAX; prio++)
> -		if (dd_has_work_for_prio(&dd->per_prio[prio]))
> +		if (dd_has_work_for_prio(dd, hctx, &dd->per_prio[prio]))
>  			return true;
>  
>  	return false;


-- 
Damien Le Moal
Western Digital Research