[PATCHSET 0/2] Add queue_is_busy helper

[PATCHSET 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.


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

-- 
Jens Axboe

[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

[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.

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

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

[Mike Snitzer]

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

> 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>

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

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

[Laurence Oberman]

On Thu, 2018-11-08 at 08:42 -0700, Jens Axboe wrote:
> +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;
> +}
> +

Hi Jens,

Trying to understand the logic, likely just my lack of knowledge
You return false after setting true because you say we know the queue
is busy.
How do we know we found a request here
I dont see you check the result of 
 bool *busy = (bool *) priv;
Is the assumption here because this was called we already knew we had a
request

Apologies it its just my lack of understanding

+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;
+}
+

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

[Jens Axboe]

On 11/8/18 8:54 AM, Laurence Oberman wrote:
> On Thu, 2018-11-08 at 08:42 -0700, Jens Axboe wrote:
>> +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;
>> +}
>> +
> 
> Hi Jens,
> 
> Trying to understand the logic, likely just my lack of knowledge
> You return false after setting true because you say we know the queue
> is busy.
> How do we know we found a request here
> I dont see you check the result of 
>  bool *busy = (bool *) priv;
> Is the assumption here because this was called we already knew we had a
> request

Yes, that's absolutely right - if we get into the helper, we know we
already have a request. Hence we know the queue is busy. Actually
thinking about it, for shared tags, we need to check if the request
is for the given queue. Will send a v2...

-- 
Jens Axboe

[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 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 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 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