Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

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

On 12/5/23 14:32, Bart Van Assche wrote:
> Fix the following two issues:
> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>   request order.
> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>   storage support in the mq-deadline scheduler.

Can you provide more details ? E.g. an example of a setup that breaks ordering ?

> This patch fixes both issues by disabling I/O prioritization for these
> two cases.

... when prio_aging_expire is set to 0. Right ? Otherwise, the sentence above
reads as if you are disabling IO priority for zoned devices...

Also,

> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/mq-deadline.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index fe5da2ade953..6781cef0109e 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -123,14 +123,16 @@ deadline_rb_root(struct dd_per_prio *per_prio, struct request *rq)
>   * Returns the I/O priority class (IOPRIO_CLASS_*) that has been assigned to a
>   * request.
>   */
> -static u8 dd_rq_ioclass(struct request *rq)
> +static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
>  {
> -	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(req_get_ioprio(rq)) :
> +				       IOPRIO_CLASS_NONE;

I personally would prefer the simpler:

	if (!dd->prio_aging_expire)
		return IOPRIO_CLASS_NONE;
	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));

>  }
>  
> -static u8 dd_bio_ioclass(struct bio *bio)
> +static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
>  {
> -	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(bio->bi_ioprio) :
> +				       IOPRIO_CLASS_NONE;

Same comment as above.

>  }
>  
>  /*
> @@ -233,7 +235,7 @@ static void dd_request_merged(struct request_queue *q, struct request *req,
>  			      enum elv_merge type)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(req);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, req);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>  
> @@ -253,7 +255,7 @@ static void dd_merged_requests(struct request_queue *q, struct request *req,
>  			       struct request *next)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(next);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, next);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  
>  	lockdep_assert_held(&dd->lock);
> @@ -550,7 +552,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>  	dd->batching++;
>  	deadline_move_request(dd, per_prio, rq);
>  done:
> -	ioprio_class = dd_rq_ioclass(rq);
> +	ioprio_class = dd_rq_ioclass(dd, rq);
>  	prio = ioprio_class_to_prio[ioprio_class];
>  	dd->per_prio[prio].latest_pos[data_dir] = blk_rq_pos(rq);
>  	dd->per_prio[prio].stats.dispatched++;
> @@ -749,7 +751,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
>  			    struct bio *bio)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_bio_ioclass(bio);
> +	const u8 ioprio_class = dd_bio_ioclass(dd, bio);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>  	sector_t sector = bio_end_sector(bio);
> @@ -814,7 +816,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  	 */
>  	blk_req_zone_write_unlock(rq);
>  
> -	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
> +	prio = ioprio_class_to_prio[dd_rq_ioclass(dd, rq)];
>  	per_prio = &dd->per_prio[prio];
>  	if (!rq->elv.priv[0]) {
>  		per_prio->stats.inserted++;
> @@ -923,7 +925,7 @@ static void dd_finish_request(struct request *rq)
>  {
>  	struct request_queue *q = rq->q;
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(rq);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, rq);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];

What about the call to dd_dispatch_prio_aged_requests() in
dd_dispatch_request() ? Shouldn't that call be skipped if prio_aging_expire is 0 ?

>  

-- 
Damien Le Moal
Western Digital Research