[PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Jens Axboe]

We have this functionality in sbitmap, but we don't export it in
blk-mq for users of the tags busy iteration. This can be useful
for stopping the iteration, if the caller doesn't need to find
more requests.

Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq-debugfs.c |  4 +++-
 block/blk-mq-tag.c     |  4 ++--
 block/blk-mq.c         | 16 +++++++++++-----
 include/linux/blk-mq.h |  4 ++--
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index cde19be36135..4f61632af6f4 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -424,13 +424,15 @@ struct show_busy_params {
  * Note: the state of a request may change while this function is in progress,
  * e.g. due to a concurrent blk_mq_finish_request() call.
  */
-static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
+static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
 {
 	const struct show_busy_params *params = data;
 
 	if (rq->mq_hctx == params->hctx)
 		__blk_mq_debugfs_rq_show(params->m,
 					 list_entry_rq(&rq->queuelist));
+
+	return true;
 }
 
 static int hctx_busy_show(void *data, struct seq_file *m)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index fb836d818b80..097e9a67d5f5 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -236,7 +236,7 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
 	 * test and set the bit before assigning ->rqs[].
 	 */
 	if (rq && rq->q == hctx->queue)
-		iter_data->fn(hctx, rq, iter_data->data, reserved);
+		return iter_data->fn(hctx, rq, iter_data->data, reserved);
 	return true;
 }
 
@@ -289,7 +289,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
 	 */
 	rq = tags->rqs[bitnr];
 	if (rq && blk_mq_request_started(rq))
-		iter_data->fn(rq, iter_data->data, reserved);
+		return iter_data->fn(rq, iter_data->data, reserved);
 
 	return true;
 }
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 45c92b8d4795..4a622c832b31 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -94,7 +94,7 @@ struct mq_inflight {
 	unsigned int *inflight;
 };
 
-static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 				  struct request *rq, void *priv,
 				  bool reserved)
 {
@@ -109,6 +109,8 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 		mi->inflight[0]++;
 	if (mi->part->partno)
 		mi->inflight[1]++;
+
+	return true;
 }
 
 void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
@@ -120,7 +122,7 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
 }
 
-static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
 				     struct request *rq, void *priv,
 				     bool reserved)
 {
@@ -128,6 +130,8 @@ static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
 
 	if (rq->part == mi->part)
 		mi->inflight[rq_data_dir(rq)]++;
+
+	return true;
 }
 
 void blk_mq_in_flight_rw(struct request_queue *q, struct hd_struct *part,
@@ -821,7 +825,7 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
 	return false;
 }
 
-static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 		struct request *rq, void *priv, bool reserved)
 {
 	unsigned long *next = priv;
@@ -831,7 +835,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	 * so we're not unnecessarilly synchronizing across CPUs.
 	 */
 	if (!blk_mq_req_expired(rq, next))
-		return;
+		return true;
 
 	/*
 	 * We have reason to believe the request may be expired. Take a
@@ -843,7 +847,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	 * timeout handler to posting a natural completion.
 	 */
 	if (!refcount_inc_not_zero(&rq->ref))
-		return;
+		return true;
 
 	/*
 	 * The request is now locked and cannot be reallocated underneath the
@@ -855,6 +859,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 		blk_mq_rq_timed_out(rq, reserved);
 	if (refcount_dec_and_test(&rq->ref))
 		__blk_mq_free_request(rq);
+
+	return true;
 }
 
 static void blk_mq_timeout_work(struct work_struct *work)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 9f5e93f40857..ff497dfcbbf9 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -129,9 +129,9 @@ typedef int (init_request_fn)(struct blk_mq_tag_set *set, struct request *,
 typedef void (exit_request_fn)(struct blk_mq_tag_set *set, struct request *,
 		unsigned int);
 
-typedef void (busy_iter_fn)(struct blk_mq_hw_ctx *, struct request *, void *,
+typedef bool (busy_iter_fn)(struct blk_mq_hw_ctx *, struct request *, void *,
 		bool);
-typedef void (busy_tag_iter_fn)(struct request *, void *, bool);
+typedef bool (busy_tag_iter_fn)(struct request *, void *, bool);
 typedef int (poll_fn)(struct blk_mq_hw_ctx *, unsigned int);
 typedef int (map_queues_fn)(struct blk_mq_tag_set *set);
 typedef bool (busy_fn)(struct request_queue *);
-- 
2.17.1

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Mike Snitzer]

On Thu, Nov 08 2018 at 10:42am -0500,
Jens Axboe <axboe@kernel.dk> wrote:

> We have this functionality in sbitmap, but we don't export it in
> blk-mq for users of the tags busy iteration. This can be useful
> for stopping the iteration, if the caller doesn't need to find
> more requests.
> 
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Reviewed-by: Mike Snitzer <snitzer@redhat.com>

(could add Tested-by too but probably overkill?)

Thanks,
Mike

[PATCHSET v2 0/2] Add queue_is_busy helper

[Jens Axboe]

DM currently uses atomic inc/dec to maintain a busy count of
IO on a given device. For the dm-mq path, we can replace this
with helper that just checks the state of the tags on the device.

First patch is a prep patch that allows the iteration helpers
to return true/false, like we support internally in sbitmap.
For a busy check we don't care about how many requests are
busy, just if some are or not. Hence we can stop iterating
tags as soon as we find one that is allocated.

Changes since v1:

- Remember to check if the queue matches, otherwise we could be
  returning false positive for shared tag sets.

 block/blk-mq-debugfs.c |  4 +++-
 block/blk-mq-tag.c     |  4 ++--
 block/blk-mq.c         | 42 +++++++++++++++++++++++++++++++++++++-----
 include/linux/blk-mq.h |  6 ++++--
 4 files changed, 46 insertions(+), 10 deletions(-)

-- 
Jens Axboe

[PATCH] blk-mq: provide a helper to check if a queue is busy

[Jens Axboe]

Returns true if the queue currently has requests pending,
false if not.

DM can use this to replace the atomic_inc/dec they do per device
to see if a device is busy.

Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 22 ++++++++++++++++++++++
 include/linux/blk-mq.h |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4a622c832b31..848adcc51c43 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -790,6 +790,28 @@ struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
 }
 EXPORT_SYMBOL(blk_mq_tag_to_rq);
 
+static bool blk_mq_check_busy(struct blk_mq_hw_ctx *hctx, struct request *rq,
+			      void *priv, bool reserved)
+{
+	bool *busy = (bool *) priv;
+
+	/*
+	 * If we find a request, we know the queue is busy. Return false
+	 * to stop the iteration.
+	 */
+	*busy = true;
+	return false;
+}
+
+bool blk_mq_queue_busy(struct request_queue *q)
+{
+	bool busy = false;
+
+	blk_mq_queue_tag_busy_iter(q, blk_mq_check_busy, &busy);
+	return busy;
+}
+EXPORT_SYMBOL_GPL(blk_mq_queue_busy);
+
 static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 {
 	req->rq_flags |= RQF_TIMED_OUT;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index ff497dfcbbf9..929e8abc5535 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -250,6 +250,8 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule);
 void blk_mq_free_request(struct request *rq);
 bool blk_mq_can_queue(struct blk_mq_hw_ctx *);
 
+bool blk_mq_queue_busy(struct request_queue *q);
+
 enum {
 	/* return when out of requests */
 	BLK_MQ_REQ_NOWAIT	= (__force blk_mq_req_flags_t)(1 << 0),
-- 
2.17.1

[PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Jens Axboe]

We have this functionality in sbitmap, but we don't export it in
blk-mq for users of the tags busy iteration. This can be useful
for stopping the iteration, if the caller doesn't need to find
more requests.

Reviewed-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq-debugfs.c |  4 +++-
 block/blk-mq-tag.c     |  4 ++--
 block/blk-mq.c         | 16 +++++++++++-----
 include/linux/blk-mq.h |  4 ++--
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index cde19be36135..4f61632af6f4 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -424,13 +424,15 @@ struct show_busy_params {
  * Note: the state of a request may change while this function is in progress,
  * e.g. due to a concurrent blk_mq_finish_request() call.
  */
-static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
+static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
 {
 	const struct show_busy_params *params = data;
 
 	if (rq->mq_hctx == params->hctx)
 		__blk_mq_debugfs_rq_show(params->m,
 					 list_entry_rq(&rq->queuelist));
+
+	return true;
 }
 
 static int hctx_busy_show(void *data, struct seq_file *m)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index fb836d818b80..097e9a67d5f5 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -236,7 +236,7 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
 	 * test and set the bit before assigning ->rqs[].
 	 */
 	if (rq && rq->q == hctx->queue)
-		iter_data->fn(hctx, rq, iter_data->data, reserved);
+		return iter_data->fn(hctx, rq, iter_data->data, reserved);
 	return true;
 }
 
@@ -289,7 +289,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
 	 */
 	rq = tags->rqs[bitnr];
 	if (rq && blk_mq_request_started(rq))
-		iter_data->fn(rq, iter_data->data, reserved);
+		return iter_data->fn(rq, iter_data->data, reserved);
 
 	return true;
 }
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 45c92b8d4795..4a622c832b31 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -94,7 +94,7 @@ struct mq_inflight {
 	unsigned int *inflight;
 };
 
-static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 				  struct request *rq, void *priv,
 				  bool reserved)
 {
@@ -109,6 +109,8 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 		mi->inflight[0]++;
 	if (mi->part->partno)
 		mi->inflight[1]++;
+
+	return true;
 }
 
 void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
@@ -120,7 +122,7 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
 }
 
-static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
 				     struct request *rq, void *priv,
 				     bool reserved)
 {
@@ -128,6 +130,8 @@ static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
 
 	if (rq->part == mi->part)
 		mi->inflight[rq_data_dir(rq)]++;
+
+	return true;
 }
 
 void blk_mq_in_flight_rw(struct request_queue *q, struct hd_struct *part,
@@ -821,7 +825,7 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
 	return false;
 }
 
-static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
+static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 		struct request *rq, void *priv, bool reserved)
 {
 	unsigned long *next = priv;
@@ -831,7 +835,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	 * so we're not unnecessarilly synchronizing across CPUs.
 	 */
 	if (!blk_mq_req_expired(rq, next))
-		return;
+		return true;
 
 	/*
 	 * We have reason to believe the request may be expired. Take a
@@ -843,7 +847,7 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	 * timeout handler to posting a natural completion.
 	 */
 	if (!refcount_inc_not_zero(&rq->ref))
-		return;
+		return true;
 
 	/*
 	 * The request is now locked and cannot be reallocated underneath the
@@ -855,6 +859,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 		blk_mq_rq_timed_out(rq, reserved);
 	if (refcount_dec_and_test(&rq->ref))
 		__blk_mq_free_request(rq);
+
+	return true;
 }
 
 static void blk_mq_timeout_work(struct work_struct *work)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 9f5e93f40857..ff497dfcbbf9 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -129,9 +129,9 @@ typedef int (init_request_fn)(struct blk_mq_tag_set *set, struct request *,
 typedef void (exit_request_fn)(struct blk_mq_tag_set *set, struct request *,
 		unsigned int);
 
-typedef void (busy_iter_fn)(struct blk_mq_hw_ctx *, struct request *, void *,
+typedef bool (busy_iter_fn)(struct blk_mq_hw_ctx *, struct request *, void *,
 		bool);
-typedef void (busy_tag_iter_fn)(struct request *, void *, bool);
+typedef bool (busy_tag_iter_fn)(struct request *, void *, bool);
 typedef int (poll_fn)(struct blk_mq_hw_ctx *, unsigned int);
 typedef int (map_queues_fn)(struct blk_mq_tag_set *set);
 typedef bool (busy_fn)(struct request_queue *);
-- 
2.17.1

[PATCH 2/2] blk-mq: provide a helper to check if a queue is busy

[Jens Axboe]

Returns true if the queue currently has requests pending,
false if not.

DM can use this to replace the atomic_inc/dec they do per device
to see if a device is busy.

Reviewed-by: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 26 ++++++++++++++++++++++++++
 include/linux/blk-mq.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4a622c832b31..65243c40ac7c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -790,6 +790,32 @@ struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
 }
 EXPORT_SYMBOL(blk_mq_tag_to_rq);
 
+static bool blk_mq_check_busy(struct blk_mq_hw_ctx *hctx, struct request *rq,
+			      void *priv, bool reserved)
+{
+	/*
+	 * If we find a request, we know the queue is busy. Return false
+	 * to stop the iteration.
+	 */
+	if (rq->q == hctx->queue) {
+		bool *busy = (bool *) priv;
+
+		*busy = true;
+		return false;
+	}
+
+	return false;
+}
+
+bool blk_mq_queue_busy(struct request_queue *q)
+{
+	bool busy = false;
+
+	blk_mq_queue_tag_busy_iter(q, blk_mq_check_busy, &busy);
+	return busy;
+}
+EXPORT_SYMBOL_GPL(blk_mq_queue_busy);
+
 static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 {
 	req->rq_flags |= RQF_TIMED_OUT;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index ff497dfcbbf9..929e8abc5535 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -250,6 +250,8 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule);
 void blk_mq_free_request(struct request *rq);
 bool blk_mq_can_queue(struct blk_mq_hw_ctx *);
 
+bool blk_mq_queue_busy(struct request_queue *q);
+
 enum {
 	/* return when out of requests */
 	BLK_MQ_REQ_NOWAIT	= (__force blk_mq_req_flags_t)(1 << 0),
-- 
2.17.1

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Bart Van Assche]

On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -424,13 +424,15 @@ struct show_busy_params {
>   * Note: the state of a request may change while this function is in progress,
>   * e.g. due to a concurrent blk_mq_finish_request() call.
>   */
> -static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
> +static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>  {

Please update the kdoc header above hctx_show_busy_rq() such that it reflects
the new behavior. I'm referring to the "will be called for each request" part.
Otherwise this patch looks fine to me.

Bart.

Re: [PATCH 2/2] blk-mq: provide a helper to check if a queue is busy

[Bart Van Assche]

On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
> +static bool blk_mq_check_busy(struct blk_mq_hw_ctx *hctx, struct request *rq,
> +			      void *priv, bool reserved)
> +{
> +	/*
> +	 * If we find a request, we know the queue is busy. Return false
> +	 * to stop the iteration.
> +	 */
> +	if (rq->q == hctx->queue) {
> +		bool *busy = (bool *) priv;

I think the "(bool *)" cast can be left out. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Jens Axboe]

On 11/8/18 9:28 AM, Bart Van Assche wrote:
> On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
>> --- a/block/blk-mq-debugfs.c
>> +++ b/block/blk-mq-debugfs.c
>> @@ -424,13 +424,15 @@ struct show_busy_params {
>>   * Note: the state of a request may change while this function is in progress,
>>   * e.g. due to a concurrent blk_mq_finish_request() call.
>>   */
>> -static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>> +static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>>  {
> 
> Please update the kdoc header above hctx_show_busy_rq() such that it reflects
> the new behavior. I'm referring to the "will be called for each request" part.
> Otherwise this patch looks fine to me.

Took a look at the comment, and what do you want changed? There's no change
in behavior for hctx_show_busy_rq(), it loops all requests just like before.
We just return true to ensure we continue iterating.

-- 
Jens Axboe

Re: [PATCH 2/2] blk-mq: provide a helper to check if a queue is busy

[Jens Axboe]

On 11/8/18 9:30 AM, Bart Van Assche wrote:
> On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
>> +static bool blk_mq_check_busy(struct blk_mq_hw_ctx *hctx, struct request *rq,
>> +			      void *priv, bool reserved)
>> +{
>> +	/*
>> +	 * If we find a request, we know the queue is busy. Return false
>> +	 * to stop the iteration.
>> +	 */
>> +	if (rq->q == hctx->queue) {
>> +		bool *busy = (bool *) priv;
> 
> I think the "(bool *)" cast can be left out. Anyway:

It can, I'll drop it.

> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks

-- 
Jens Axboe

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Bart Van Assche]

On Thu, 2018-11-08 at 09:31 -0700, Jens Axboe wrote:
> On 11/8/18 9:28 AM, Bart Van Assche wrote:
> > On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
> > > --- a/block/blk-mq-debugfs.c
> > > +++ b/block/blk-mq-debugfs.c
> > > @@ -424,13 +424,15 @@ struct show_busy_params {
> > >   * Note: the state of a request may change while this function is in progress,
> > >   * e.g. due to a concurrent blk_mq_finish_request() call.
> > >   */
> > > -static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
> > > +static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
> > >  {
> > 
> > Please update the kdoc header above hctx_show_busy_rq() such that it reflects
> > the new behavior. I'm referring to the "will be called for each request" part.
> > Otherwise this patch looks fine to me.
> 
> Took a look at the comment, and what do you want changed? There's no change
> in behavior for hctx_show_busy_rq(), it loops all requests just like before.
> We just return true to ensure we continue iterating.

Oops, I added my reply below the wrong function. I wanted to refer to the
following comment above blk_mq_queue_tag_busy_iter():

 * @fn:		Pointer to the function that will be called for each request

Additionally, how about similar comments above bt_for_each(), bt_tags_for_each()
blk_mq_all_tag_busy_iter() and blk_mq_tagset_busy_iter()? I think this patch
affects all these functions.

Thanks,

Bart.

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Jens Axboe]

On 11/8/18 10:35 AM, Bart Van Assche wrote:
> On Thu, 2018-11-08 at 09:31 -0700, Jens Axboe wrote:
>> On 11/8/18 9:28 AM, Bart Van Assche wrote:
>>> On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
>>>> --- a/block/blk-mq-debugfs.c
>>>> +++ b/block/blk-mq-debugfs.c
>>>> @@ -424,13 +424,15 @@ struct show_busy_params {
>>>>   * Note: the state of a request may change while this function is in progress,
>>>>   * e.g. due to a concurrent blk_mq_finish_request() call.
>>>>   */
>>>> -static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>>>> +static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>>>>  {
>>>
>>> Please update the kdoc header above hctx_show_busy_rq() such that it reflects
>>> the new behavior. I'm referring to the "will be called for each request" part.
>>> Otherwise this patch looks fine to me.
>>
>> Took a look at the comment, and what do you want changed? There's no change
>> in behavior for hctx_show_busy_rq(), it loops all requests just like before.
>> We just return true to ensure we continue iterating.
> 
> Oops, I added my reply below the wrong function. I wanted to refer to the
> following comment above blk_mq_queue_tag_busy_iter():
> 
>  * @fn:		Pointer to the function that will be called for each request
> 
> Additionally, how about similar comments above bt_for_each(), bt_tags_for_each()
> blk_mq_all_tag_busy_iter() and blk_mq_tagset_busy_iter()? I think this patch
> affects all these functions.

Fair enough, I'll add a documentation patch. Didn't consider it a big deal
since this is how it's always worked internally, but we haven't exposed
this break/continue functionality outside the sbitmap core until now.

-- 
Jens Axboe

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Jens Axboe]

On 11/8/18 10:47 AM, Jens Axboe wrote:
> On 11/8/18 10:35 AM, Bart Van Assche wrote:
>> On Thu, 2018-11-08 at 09:31 -0700, Jens Axboe wrote:
>>> On 11/8/18 9:28 AM, Bart Van Assche wrote:
>>>> On Thu, 2018-11-08 at 09:06 -0700, Jens Axboe wrote:
>>>>> --- a/block/blk-mq-debugfs.c
>>>>> +++ b/block/blk-mq-debugfs.c
>>>>> @@ -424,13 +424,15 @@ struct show_busy_params {
>>>>>   * Note: the state of a request may change while this function is in progress,
>>>>>   * e.g. due to a concurrent blk_mq_finish_request() call.
>>>>>   */
>>>>> -static void hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>>>>> +static bool hctx_show_busy_rq(struct request *rq, void *data, bool reserved)
>>>>>  {
>>>>
>>>> Please update the kdoc header above hctx_show_busy_rq() such that it reflects
>>>> the new behavior. I'm referring to the "will be called for each request" part.
>>>> Otherwise this patch looks fine to me.
>>>
>>> Took a look at the comment, and what do you want changed? There's no change
>>> in behavior for hctx_show_busy_rq(), it loops all requests just like before.
>>> We just return true to ensure we continue iterating.
>>
>> Oops, I added my reply below the wrong function. I wanted to refer to the
>> following comment above blk_mq_queue_tag_busy_iter():
>>
>>  * @fn:		Pointer to the function that will be called for each request
>>
>> Additionally, how about similar comments above bt_for_each(), bt_tags_for_each()
>> blk_mq_all_tag_busy_iter() and blk_mq_tagset_busy_iter()? I think this patch
>> affects all these functions.
> 
> Fair enough, I'll add a documentation patch. Didn't consider it a big deal
> since this is how it's always worked internally, but we haven't exposed
> this break/continue functionality outside the sbitmap core until now.

How about this incremental?


diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 097e9a67d5f5..87bc5df72d48 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -248,7 +248,8 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
  * @fn:		Pointer to the function that will be called for each request
  *		associated with @hctx that has been assigned a driver tag.
  *		@fn will be called as follows: @fn(@hctx, rq, @data, @reserved)
- *		where rq is a pointer to a request.
+ *		where rq is a pointer to a request. Return true to continue
+ *		iterating tags, false to stop.
  * @data:	Will be passed as third argument to @fn.
  * @reserved:	Indicates whether @bt is the breserved_tags member or the
  *		bitmap_tags member of struct blk_mq_tags.
@@ -301,7 +302,8 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
  *		or the bitmap_tags member of struct blk_mq_tags.
  * @fn:		Pointer to the function that will be called for each started
  *		request. @fn will be called as follows: @fn(rq, @data,
- *		@reserved) where rq is a pointer to a request.
+ *		@reserved) where rq is a pointer to a request. Return true
+ *		to continue iterating tags, false to stop.
  * @data:	Will be passed as second argument to @fn.
  * @reserved:	Indicates whether @bt is the breserved_tags member or the
  *		bitmap_tags member of struct blk_mq_tags.
@@ -326,7 +328,8 @@ static void bt_tags_for_each(struct blk_mq_tags *tags, struct sbitmap_queue *bt,
  * @fn:		Pointer to the function that will be called for each started
  *		request. @fn will be called as follows: @fn(rq, @priv,
  *		reserved) where rq is a pointer to a request. 'reserved'
- *		indicates whether or not @rq is a reserved request.
+ *		indicates whether or not @rq is a reserved request. Return
+ *		true to continue iterating tags, false to stop.
  * @priv:	Will be passed as second argument to @fn.
  */
 static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
@@ -343,7 +346,8 @@ static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
  * @fn:		Pointer to the function that will be called for each started
  *		request. @fn will be called as follows: @fn(rq, @priv,
  *		reserved) where rq is a pointer to a request. 'reserved'
- *		indicates whether or not @rq is a reserved request.
+ *		indicates whether or not @rq is a reserved request. Return
+ *		true to continue iterating tags, false to stop.
  * @priv:	Will be passed as second argument to @fn.
  */
 void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,

-- 
Jens Axboe

Re: [PATCH 1/2] blk-mq-tag: change busy_iter_fn to return whether to continue or not

[Bart Van Assche]

On Thu, 2018-11-08 at 10:50 -0700, Jens Axboe wrote:
> How about this incremental?
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 097e9a67d5f5..87bc5df72d48 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -248,7 +248,8 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>   * @fn:		Pointer to the function that will be called for each request
>   *		associated with @hctx that has been assigned a driver tag.
>   *		@fn will be called as follows: @fn(@hctx, rq, @data, @reserved)
> - *		where rq is a pointer to a request.
> + *		where rq is a pointer to a request. Return true to continue
> + *		iterating tags, false to stop.
>   * @data:	Will be passed as third argument to @fn.
>   * @reserved:	Indicates whether @bt is the breserved_tags member or the
>   *		bitmap_tags member of struct blk_mq_tags.
> @@ -301,7 +302,8 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
>   *		or the bitmap_tags member of struct blk_mq_tags.
>   * @fn:		Pointer to the function that will be called for each started
>   *		request. @fn will be called as follows: @fn(rq, @data,
> - *		@reserved) where rq is a pointer to a request.
> + *		@reserved) where rq is a pointer to a request. Return true
> + *		to continue iterating tags, false to stop.
>   * @data:	Will be passed as second argument to @fn.
>   * @reserved:	Indicates whether @bt is the breserved_tags member or the
>   *		bitmap_tags member of struct blk_mq_tags.
> @@ -326,7 +328,8 @@ static void bt_tags_for_each(struct blk_mq_tags *tags, struct sbitmap_queue *bt,
>   * @fn:		Pointer to the function that will be called for each started
>   *		request. @fn will be called as follows: @fn(rq, @priv,
>   *		reserved) where rq is a pointer to a request. 'reserved'
> - *		indicates whether or not @rq is a reserved request.
> + *		indicates whether or not @rq is a reserved request. Return
> + *		true to continue iterating tags, false to stop.
>   * @priv:	Will be passed as second argument to @fn.
>   */
>  static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
> @@ -343,7 +346,8 @@ static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
>   * @fn:		Pointer to the function that will be called for each started
>   *		request. @fn will be called as follows: @fn(rq, @priv,
>   *		reserved) where rq is a pointer to a request. 'reserved'
> - *		indicates whether or not @rq is a reserved request.
> + *		indicates whether or not @rq is a reserved request. Return
> + *		true to continue iterating tags, false to stop.
>   * @priv:	Will be passed as second argument to @fn.
>   */
>  void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,

This looks fine to me. Thanks!

Bart.

Re: [PATCHSET v2 0/2] Add queue_is_busy helper

[jianchao.wang]

Hi Jens

On 11/9/18 12:06 AM, Jens Axboe wrote:
> DM currently uses atomic inc/dec to maintain a busy count of
> IO on a given device. For the dm-mq path, we can replace this
> with helper that just checks the state of the tags on the device.
> 
> First patch is a prep patch that allows the iteration helpers
> to return true/false, like we support internally in sbitmap.
> For a busy check we don't care about how many requests are
> busy, just if some are or not. Hence we can stop iterating
> tags as soon as we find one that is allocated.

If we don't care about how many requests are busy, why not check
sb->map[idex].word directly ? It could be more efficient.

Thanks
Jianchao

Re: [PATCHSET v2 0/2] Add queue_is_busy helper

[jianchao.wang]

On 11/9/18 9:53 AM, jianchao.wang wrote:
> Hi Jens
> 
> On 11/9/18 12:06 AM, Jens Axboe wrote:
>> DM currently uses atomic inc/dec to maintain a busy count of
>> IO on a given device. For the dm-mq path, we can replace this
>> with helper that just checks the state of the tags on the device.
>>
>> First patch is a prep patch that allows the iteration helpers
>> to return true/false, like we support internally in sbitmap.
>> For a busy check we don't care about how many requests are
>> busy, just if some are or not. Hence we can stop iterating
>> tags as soon as we find one that is allocated.
> 
> If we don't care about how many requests are busy, why not check
> sb->map[idex].word directly ? It could be more efficient.
> 

Oh, we need to check rq->q for the tag shared case.

> Thanks
> Jianchao
>