Re: [PATCH] block/mq-deadline: Fix WARN when set async_depth by sysfs

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

On 3/31/24 17:58, Zhiguo Niu wrote:
> On Sat, Mar 30, 2024 at 2:08 AM Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> On 3/28/24 7:44 PM, Zhiguo Niu wrote:
>>> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
>>> index 02a916b..89c516e 100644
>>> --- a/block/mq-deadline.c
>>> +++ b/block/mq-deadline.c
>>> @@ -646,10 +646,12 @@ static void dd_depth_updated(struct blk_mq_hw_ctx *hctx)
>>>        struct request_queue *q = hctx->queue;
>>>        struct deadline_data *dd = q->elevator->elevator_data;
>>>        struct blk_mq_tags *tags = hctx->sched_tags;
>>> +     unsigned int shift = tags->bitmap_tags.sb.shift;
>>> +     unsigned int dd_min_depth = max(1U, 3 * (1U << shift)  / 4);
>>>
>>>        dd->async_depth = max(1UL, 3 * q->nr_requests / 4);
>>>
>>> -     sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async_depth);
>>> +     sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd_min_depth);
>>>    }
>>
>> The above patch sets min_shallow_depth to the same value as commit
>> d47f9717e5cf ("block/mq-deadline: use correct way to throttling write
>> requests"). That commit got reverted because it was causing performance
>> problems. So the above patch reintroduces the performance problem that
>> has been fixed by commit 256aab46e316 ("Revert "block/mq-deadline: use
>> correct way to throttling write requests"").
> Hi Bart Van Assche,
> 
> This  patch only modifies the initial minimum value of
> min_shallow_depth and does not change "dd->async_depth",
> so it will not cause performance problems like the previous patch
> (d47f9717e5cf ("block/mq-deadline: use correct way to throttling write
> requests")).

Oops, I misread your patch. After having taken another look, my
conclusions are as follows:
* sbitmap_queue_min_shallow_depth() is called. This causes
   sbq->wake_batch to be modified but I don't think that it is a proper
   fix for dd_limit_depth().
* dd_limit_depth() still assigns a number in the range 1..nr_requests to
   data->shallow_depth while a number in the range 1..(1<<bt->sb.shift)
   should be assigned.

> So what are your suggestions for fixing the warning shown in commit
> msg if dd->async_depth is set by the user from sysfs?
> thanks

How about the two untested patches below?

Thanks,

Bart.


Subject: [PATCH 1/2] block: Call .limit_depth() after .hctx has been set

Prepare for using .hctx in dd_limit_depth().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  block/blk-mq.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 34060d885c5a..d0db9252bb71 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -435,6 +435,7 @@ __blk_mq_alloc_requests_batch(struct 
blk_mq_alloc_data *data)
  static struct request *__blk_mq_alloc_requests(struct 
blk_mq_alloc_data *data)
  {
  	struct request_queue *q = data->q;
+	struct elevator_mq_ops *ops = NULL;
  	u64 alloc_time_ns = 0;
  	struct request *rq;
  	unsigned int tag;
@@ -459,13 +460,11 @@ static struct request 
*__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
  		 */
  		if ((data->cmd_flags & REQ_OP_MASK) != REQ_OP_FLUSH &&
  		    !blk_op_is_passthrough(data->cmd_flags)) {
-			struct elevator_mq_ops *ops = &q->elevator->type->ops;
+			ops = &q->elevator->type->ops;

  			WARN_ON_ONCE(data->flags & BLK_MQ_REQ_RESERVED);

  			data->rq_flags |= RQF_USE_SCHED;
-			if (ops->limit_depth)
-				ops->limit_depth(data->cmd_flags, data);
  		}
  	}

@@ -478,6 +477,9 @@ static struct request 
*__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
  	if (data->flags & BLK_MQ_REQ_RESERVED)
  		data->rq_flags |= RQF_RESV;

+	if (ops && ops->limit_depth)
+		ops->limit_depth(data->cmd_flags, data);
+
  	/*
  	 * Try batched alloc if we want more than 1 tag.
  	 */



Subject: [PATCH 2/2] block/mq-deadline: Fix the tag reservation code

Fixes: 07757588e507 ("block/mq-deadline: Reserve 25% of scheduler tags 
for synchronous requests")
---
  block/mq-deadline.c | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 02a916ba62ee..8e780069d91b 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -621,6 +621,20 @@ static struct request *dd_dispatch_request(struct 
blk_mq_hw_ctx *hctx)
  	return rq;
  }

+/*
+ * 'depth' is a number in the range 0..q->nr_requests. Convert it to a 
number
+ * in the range 0..(1 << bt->sb.shift) since that is the range expected by
+ * sbitmap_get_shallow().
+ */
+static int dd_to_word_depth(struct blk_mq_hw_ctx *hctx, unsigned int 
qdepth)
+{
+	struct sbitmap_queue *bt = &hctx->sched_tags->bitmap_tags;
+	const unsigned int nrr = hctx->queue->nr_requests;
+
+	return max(((qdepth << bt->sb.shift) + nrr - 1) / nrr,
+		   bt->min_shallow_depth);
+}
+
  /*
   * Called by __blk_mq_alloc_request(). The shallow_depth value set by this
   * function is used by __blk_mq_get_tag().
@@ -637,7 +651,7 @@ static void dd_limit_depth(blk_opf_t opf, struct 
blk_mq_alloc_data *data)
  	 * Throttle asynchronous requests and writes such that these requests
  	 * do not block the allocation of synchronous requests.
  	 */
-	data->shallow_depth = dd->async_depth;
+	data->shallow_depth = dd_to_word_depth(data->hctx, dd->async_depth);
  }

  /* Called by blk_mq_update_nr_requests(). */
@@ -649,7 +663,7 @@ static void dd_depth_updated(struct blk_mq_hw_ctx *hctx)

  	dd->async_depth = max(1UL, 3 * q->nr_requests / 4);

-	sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, dd->async_depth);
+	sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, 1);
  }

  /* Called by blk_mq_init_hctx() and blk_mq_init_sched(). */