[PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[Paolo Valente]

Hi Jens,
a user reported a warning, followed by freezes, in case he increases
nr_requests to more than 64 [1]. After reproducing the issues, I
reverted the commit f0635b8a416e ("bfq: calculate shallow depths at
init time"), plus the related commit bd7d4ef6a4c9 ("bfq-iosched:
remove unused variable"). The problem went away.

Maybe the assumption in commit f0635b8a416e ("bfq: calculate shallow
depths at init time") does not hold true?

Thanks,
Paolo

[1] https://bugzilla.kernel.org/show_bug.cgi?id=200813

Paolo Valente (2):
  Revert "bfq-iosched: remove unused variable"
  Revert "bfq: calculate shallow depths at init time"

 block/bfq-iosched.c | 116 ++++++++++++++++++++++----------------------
 block/bfq-iosched.h |   6 +++
 2 files changed, 63 insertions(+), 59 deletions(-)

--
2.20.1

[PATCH BUGFIX RFC 1/2] Revert "bfq-iosched: remove unused variable"

[Paolo Valente]

This reverts commit bd7d4ef6a4c9b3611fa487a0065bf042c71ce620.
---
 block/bfq-iosched.c | 15 ++++++++-------
 block/bfq-iosched.h |  6 ++++++
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index cd307767a134..8cc3032b66de 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5303,25 +5303,26 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
 				      struct sbitmap_queue *bt)
 {
 	unsigned int i, j, min_shallow = UINT_MAX;
+	bfqd->sb_shift = bt->sb.shift;
 
 	/*
 	 * In-word depths if no bfq_queue is being weight-raised:
 	 * leaving 25% of tags only for sync reads.
 	 *
 	 * In next formulas, right-shift the value
-	 * (1U<<bt->sb.shift), instead of computing directly
-	 * (1U<<(bt->sb.shift - something)), to be robust against
-	 * any possible value of bt->sb.shift, without having to
+	 * (1U<<bfqd->sb_shift), instead of computing directly
+	 * (1U<<(bfqd->sb_shift - something)), to be robust against
+	 * any possible value of bfqd->sb_shift, without having to
 	 * limit 'something'.
 	 */
 	/* no more than 50% of tags for async I/O */
-	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 1, 1U);
+	bfqd->word_depths[0][0] = max((1U<<bfqd->sb_shift)>>1, 1U);
 	/*
 	 * no more than 75% of tags for sync writes (25% extra tags
 	 * w.r.t. async I/O, to prevent async I/O from starving sync
 	 * writes)
 	 */
-	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U<<bfqd->sb_shift) * 3)>>2, 1U);
 
 	/*
 	 * In-word depths in case some bfq_queue is being weight-
@@ -5331,9 +5332,9 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
 	 * shortage.
 	 */
 	/* no more than ~18% of tags for async I/O */
-	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max(((1U<<bfqd->sb_shift) * 3)>>4, 1U);
 	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max(((1U<<bfqd->sb_shift) * 6)>>4, 1U);
 
 	for (i = 0; i < 2; i++)
 		for (j = 0; j < 2; j++)
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 0b02bf302de0..4de5dc349a1e 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -697,6 +697,12 @@ struct bfq_data {
 	/* bfqq associated with the task issuing current bio for merging */
 	struct bfq_queue *bio_bfqq;
 
+	/*
+	 * Cached sbitmap shift, used to compute depth limits in
+	 * bfq_update_depths.
+	 */
+	unsigned int sb_shift;
+
 	/*
 	 * Depth limits used in bfq_limit_depth (see comments on the
 	 * function)
-- 
2.20.1

[PATCH BUGFIX RFC 2/2] Revert "bfq: calculate shallow depths at init time"

[Paolo Valente]

This reverts commit f0635b8a416e3b99dc6fd9ac3ce534764869d0c8.
---
 block/bfq-iosched.c | 117 +++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 60 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 8cc3032b66de..92214d58510c 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -520,6 +520,54 @@ static struct request *bfq_choose_req(struct bfq_data *bfqd,
 	}
 }
 
+/*
+ * See the comments on bfq_limit_depth for the purpose of
+ * the depths set in the function. Return minimum shallow depth we'll use.
+ */
+static unsigned int bfq_update_depths(struct bfq_data *bfqd,
+				      struct sbitmap_queue *bt)
+{
+	unsigned int i, j, min_shallow = UINT_MAX;
+	bfqd->sb_shift = bt->sb.shift;
+
+	/*
+	 * In-word depths if no bfq_queue is being weight-raised:
+	 * leaving 25% of tags only for sync reads.
+	 *
+	 * In next formulas, right-shift the value
+	 * (1U<<bfqd->sb_shift), instead of computing directly
+	 * (1U<<(bfqd->sb_shift - something)), to be robust against
+	 * any possible value of bfqd->sb_shift, without having to
+	 * limit 'something'.
+	 */
+	/* no more than 50% of tags for async I/O */
+	bfqd->word_depths[0][0] = max((1U<<bfqd->sb_shift)>>1, 1U);
+	/*
+	 * no more than 75% of tags for sync writes (25% extra tags
+	 * w.r.t. async I/O, to prevent async I/O from starving sync
+	 * writes)
+	 */
+	bfqd->word_depths[0][1] = max(((1U<<bfqd->sb_shift) * 3)>>2, 1U);
+
+	/*
+	 * In-word depths in case some bfq_queue is being weight-
+	 * raised: leaving ~63% of tags for sync reads. This is the
+	 * highest percentage for which, in our tests, application
+	 * start-up times didn't suffer from any regression due to tag
+	 * shortage.
+	 */
+	/* no more than ~18% of tags for async I/O */
+	bfqd->word_depths[1][0] = max(((1U<<bfqd->sb_shift) * 3)>>4, 1U);
+	/* no more than ~37% of tags for sync writes (~20% extra tags) */
+	bfqd->word_depths[1][1] = max(((1U<<bfqd->sb_shift) * 6)>>4, 1U);
+
+	for (i = 0; i < 2; i++)
+		for (j = 0; j < 2; j++)
+			min_shallow = min(min_shallow, bfqd->word_depths[i][j]);
+
+	return min_shallow;
+}
+
 /*
  * Async I/O can easily starve sync I/O (both sync reads and sync
  * writes), by consuming all tags. Similarly, storms of sync writes,
@@ -529,11 +577,20 @@ static struct request *bfq_choose_req(struct bfq_data *bfqd,
  */
 static void bfq_limit_depth(unsigned int op, struct blk_mq_alloc_data *data)
 {
+	struct blk_mq_tags *tags = blk_mq_tags_from_data(data);
 	struct bfq_data *bfqd = data->q->elevator->elevator_data;
+	struct sbitmap_queue *bt;
 
 	if (op_is_sync(op) && !op_is_write(op))
 		return;
 
+	bt = &tags->bitmap_tags;
+
+	if (unlikely(bfqd->sb_shift != bt->sb.shift)) {
+		unsigned int min_shallow = bfq_update_depths(bfqd, bt);
+		sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, min_shallow);
+	}
+
 	data->shallow_depth =
 		bfqd->word_depths[!!bfqd->wr_busy_queues][op_is_sync(op)];
 
@@ -5295,65 +5352,6 @@ void bfq_put_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg)
 	__bfq_put_async_bfqq(bfqd, &bfqg->async_idle_bfqq);
 }
 
-/*
- * See the comments on bfq_limit_depth for the purpose of
- * the depths set in the function. Return minimum shallow depth we'll use.
- */
-static unsigned int bfq_update_depths(struct bfq_data *bfqd,
-				      struct sbitmap_queue *bt)
-{
-	unsigned int i, j, min_shallow = UINT_MAX;
-	bfqd->sb_shift = bt->sb.shift;
-
-	/*
-	 * In-word depths if no bfq_queue is being weight-raised:
-	 * leaving 25% of tags only for sync reads.
-	 *
-	 * In next formulas, right-shift the value
-	 * (1U<<bfqd->sb_shift), instead of computing directly
-	 * (1U<<(bfqd->sb_shift - something)), to be robust against
-	 * any possible value of bfqd->sb_shift, without having to
-	 * limit 'something'.
-	 */
-	/* no more than 50% of tags for async I/O */
-	bfqd->word_depths[0][0] = max((1U<<bfqd->sb_shift)>>1, 1U);
-	/*
-	 * no more than 75% of tags for sync writes (25% extra tags
-	 * w.r.t. async I/O, to prevent async I/O from starving sync
-	 * writes)
-	 */
-	bfqd->word_depths[0][1] = max(((1U<<bfqd->sb_shift) * 3)>>2, 1U);
-
-	/*
-	 * In-word depths in case some bfq_queue is being weight-
-	 * raised: leaving ~63% of tags for sync reads. This is the
-	 * highest percentage for which, in our tests, application
-	 * start-up times didn't suffer from any regression due to tag
-	 * shortage.
-	 */
-	/* no more than ~18% of tags for async I/O */
-	bfqd->word_depths[1][0] = max(((1U<<bfqd->sb_shift) * 3)>>4, 1U);
-	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max(((1U<<bfqd->sb_shift) * 6)>>4, 1U);
-
-	for (i = 0; i < 2; i++)
-		for (j = 0; j < 2; j++)
-			min_shallow = min(min_shallow, bfqd->word_depths[i][j]);
-
-	return min_shallow;
-}
-
-static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
-{
-	struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
-	struct blk_mq_tags *tags = hctx->sched_tags;
-	unsigned int min_shallow;
-
-	min_shallow = bfq_update_depths(bfqd, &tags->bitmap_tags);
-	sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, min_shallow);
-	return 0;
-}
-
 static void bfq_exit_queue(struct elevator_queue *e)
 {
 	struct bfq_data *bfqd = e->elevator_data;
@@ -5773,7 +5771,6 @@ static struct elevator_type iosched_bfq_mq = {
 		.requests_merged	= bfq_requests_merged,
 		.request_merged		= bfq_request_merged,
 		.has_work		= bfq_has_work,
-		.init_hctx		= bfq_init_hctx,
 		.init_sched		= bfq_init_queue,
 		.exit_sched		= bfq_exit_queue,
 	},
-- 
2.20.1

Re: [PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[Jens Axboe]

On 1/18/19 4:52 AM, Paolo Valente wrote:
> Hi Jens,
> a user reported a warning, followed by freezes, in case he increases
> nr_requests to more than 64 [1]. After reproducing the issues, I
> reverted the commit f0635b8a416e ("bfq: calculate shallow depths at
> init time"), plus the related commit bd7d4ef6a4c9 ("bfq-iosched:
> remove unused variable"). The problem went away.

For reverts, please put the justification into the actual revert
commit. With this series, if applied as-is, we'd have two patches
in the tree that just says "revert X" without any hint as to why
that was done.

> Maybe the assumption in commit f0635b8a416e ("bfq: calculate shallow
> depths at init time") does not hold true?

It apparently doesn't! But let's try and figure this out instead of
blindly reverting it. OK, I think I see it. For the sched_tags
case, when we grow the requests, we allocate a new set. Hence any
old cache would be stale at that point.

How about something like this? It still keeps the code of having
to update this out of the hot IO path, and only calls it when we
actually change the depths.

Totally untested...


diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index cd307767a134..b09589915667 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5342,7 +5342,7 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
 	return min_shallow;
 }
 
-static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
+static void bfq_depth_updated(struct blk_mq_hw_ctx *hctx)
 {
 	struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
 	struct blk_mq_tags *tags = hctx->sched_tags;
@@ -5350,6 +5350,11 @@ static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
 
 	min_shallow = bfq_update_depths(bfqd, &tags->bitmap_tags);
 	sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, min_shallow);
+}
+
+static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
+{
+	bfq_depth_updated(hctx);
 	return 0;
 }
 
@@ -5772,6 +5777,7 @@ static struct elevator_type iosched_bfq_mq = {
 		.requests_merged	= bfq_requests_merged,
 		.request_merged		= bfq_request_merged,
 		.has_work		= bfq_has_work,
+		.depth_updated		= bfq_depth_updated,
 		.init_hctx		= bfq_init_hctx,
 		.init_sched		= bfq_init_queue,
 		.exit_sched		= bfq_exit_queue,
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3ba37b9e15e9..a047b297ade5 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3101,6 +3101,8 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
 		}
 		if (ret)
 			break;
+		if (q->elevator && q->elevator->type->ops.depth_updated)
+			q->elevator->type->ops.depth_updated(hctx);
 	}
 
 	if (!ret)
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index 2e9e2763bf47..6e8bc53740f0 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -31,6 +31,7 @@ struct elevator_mq_ops {
 	void (*exit_sched)(struct elevator_queue *);
 	int (*init_hctx)(struct blk_mq_hw_ctx *, unsigned int);
 	void (*exit_hctx)(struct blk_mq_hw_ctx *, unsigned int);
+	void (*depth_updated)(struct blk_mq_hw_ctx *);
 
 	bool (*allow_merge)(struct request_queue *, struct request *, struct bio *);
 	bool (*bio_merge)(struct blk_mq_hw_ctx *, struct bio *);

-- 
Jens Axboe

Re: [PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[Jens Axboe]

On 1/18/19 6:35 AM, Jens Axboe wrote:
> On 1/18/19 4:52 AM, Paolo Valente wrote:
>> Hi Jens,
>> a user reported a warning, followed by freezes, in case he increases
>> nr_requests to more than 64 [1]. After reproducing the issues, I
>> reverted the commit f0635b8a416e ("bfq: calculate shallow depths at
>> init time"), plus the related commit bd7d4ef6a4c9 ("bfq-iosched:
>> remove unused variable"). The problem went away.
> 
> For reverts, please put the justification into the actual revert
> commit. With this series, if applied as-is, we'd have two patches
> in the tree that just says "revert X" without any hint as to why
> that was done.
> 
>> Maybe the assumption in commit f0635b8a416e ("bfq: calculate shallow
>> depths at init time") does not hold true?
> 
> It apparently doesn't! But let's try and figure this out instead of
> blindly reverting it. OK, I think I see it. For the sched_tags
> case, when we grow the requests, we allocate a new set. Hence any
> old cache would be stale at that point.
> 
> How about something like this? It still keeps the code of having
> to update this out of the hot IO path, and only calls it when we
> actually change the depths.
> 
> Totally untested...

Now tested, and it seems to work for me. Note that haven't tried to
reproduce the issue, I just verified that the patch functionally
does what it should - when depths are updated, the hook is invoked
and updates the internal BFQ depth map.

-- 
Jens Axboe

Re: [PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[Paolo Valente]

> Il giorno 18 gen 2019, alle ore 14:35, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 1/18/19 4:52 AM, Paolo Valente wrote:
>> Hi Jens,
>> a user reported a warning, followed by freezes, in case he increases
>> nr_requests to more than 64 [1]. After reproducing the issues, I
>> reverted the commit f0635b8a416e ("bfq: calculate shallow depths at
>> init time"), plus the related commit bd7d4ef6a4c9 ("bfq-iosched:
>> remove unused variable"). The problem went away.
> 
> For reverts, please put the justification into the actual revert
> commit. With this series, if applied as-is, we'd have two patches
> in the tree that just says "revert X" without any hint as to why
> that was done.
> 

I forget to say explicitly that these patches were meant only to give
you and anybody else something concrete to test and check.

With me you're as safe as houses, in terms of amount of comments in
final patches :)

>> Maybe the assumption in commit f0635b8a416e ("bfq: calculate shallow
>> depths at init time") does not hold true?
> 
> It apparently doesn't! But let's try and figure this out instead of
> blindly reverting it.

Totally agree.

> OK, I think I see it. For the sched_tags
> case, when we grow the requests, we allocate a new set. Hence any
> old cache would be stale at that point.
> 

ok

> How about something like this? It still keeps the code of having
> to update this out of the hot IO path, and only calls it when we
> actually change the depths.
> 

Looks rather clean and efficient.

> Totally untested...
> 

It seems to work here too.

Thanks,
Paolo

> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index cd307767a134..b09589915667 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5342,7 +5342,7 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
> 	return min_shallow;
> }
> 
> -static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
> +static void bfq_depth_updated(struct blk_mq_hw_ctx *hctx)
> {
> 	struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
> 	struct blk_mq_tags *tags = hctx->sched_tags;
> @@ -5350,6 +5350,11 @@ static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
> 
> 	min_shallow = bfq_update_depths(bfqd, &tags->bitmap_tags);
> 	sbitmap_queue_min_shallow_depth(&tags->bitmap_tags, min_shallow);
> +}
> +
> +static int bfq_init_hctx(struct blk_mq_hw_ctx *hctx, unsigned int index)
> +{
> +	bfq_depth_updated(hctx);
> 	return 0;
> }
> 
> @@ -5772,6 +5777,7 @@ static struct elevator_type iosched_bfq_mq = {
> 		.requests_merged	= bfq_requests_merged,
> 		.request_merged		= bfq_request_merged,
> 		.has_work		= bfq_has_work,
> +		.depth_updated		= bfq_depth_updated,
> 		.init_hctx		= bfq_init_hctx,
> 		.init_sched		= bfq_init_queue,
> 		.exit_sched		= bfq_exit_queue,
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 3ba37b9e15e9..a047b297ade5 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -3101,6 +3101,8 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
> 		}
> 		if (ret)
> 			break;
> +		if (q->elevator && q->elevator->type->ops.depth_updated)
> +			q->elevator->type->ops.depth_updated(hctx);
> 	}
> 
> 	if (!ret)
> diff --git a/include/linux/elevator.h b/include/linux/elevator.h
> index 2e9e2763bf47..6e8bc53740f0 100644
> --- a/include/linux/elevator.h
> +++ b/include/linux/elevator.h
> @@ -31,6 +31,7 @@ struct elevator_mq_ops {
> 	void (*exit_sched)(struct elevator_queue *);
> 	int (*init_hctx)(struct blk_mq_hw_ctx *, unsigned int);
> 	void (*exit_hctx)(struct blk_mq_hw_ctx *, unsigned int);
> +	void (*depth_updated)(struct blk_mq_hw_ctx *);
> 
> 	bool (*allow_merge)(struct request_queue *, struct request *, struct bio *);
> 	bool (*bio_merge)(struct blk_mq_hw_ctx *, struct bio *);
> 
> -- 
> Jens Axboe
> 

Re: [PATCH BUGFIX RFC 0/2] reverting two commits causing freezes

[Jens Axboe]

On 1/18/19 10:24 AM, Paolo Valente wrote:
> 
> 
>> Il giorno 18 gen 2019, alle ore 14:35, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 1/18/19 4:52 AM, Paolo Valente wrote:
>>> Hi Jens,
>>> a user reported a warning, followed by freezes, in case he increases
>>> nr_requests to more than 64 [1]. After reproducing the issues, I
>>> reverted the commit f0635b8a416e ("bfq: calculate shallow depths at
>>> init time"), plus the related commit bd7d4ef6a4c9 ("bfq-iosched:
>>> remove unused variable"). The problem went away.
>>
>> For reverts, please put the justification into the actual revert
>> commit. With this series, if applied as-is, we'd have two patches
>> in the tree that just says "revert X" without any hint as to why
>> that was done.
>>
> 
> I forget to say explicitly that these patches were meant only to give
> you and anybody else something concrete to test and check.
> 
> With me you're as safe as houses, in terms of amount of comments in
> final patches :)

It's almost an example of the classic case of "if you want a real
solution to a problem, post a knowingly bad and half assed solution".
That always gets people out of the woodwork :-)

>>> Maybe the assumption in commit f0635b8a416e ("bfq: calculate shallow
>>> depths at init time") does not hold true?
>>
>> It apparently doesn't! But let's try and figure this out instead of
>> blindly reverting it.
> 
> Totally agree.
> 
>> OK, I think I see it. For the sched_tags
>> case, when we grow the requests, we allocate a new set. Hence any
>> old cache would be stale at that point.
>>
> 
> ok
> 
>> How about something like this? It still keeps the code of having
>> to update this out of the hot IO path, and only calls it when we
>> actually change the depths.
>>
> 
> Looks rather clean and efficient.
> 
>> Totally untested...
>>
> 
> It seems to work here too.

OK good, I've posted it "officially" now.
 
-- 
Jens Axboe