* [PATCH 0/3] blk-mq: misc fixes
@ 2021-11-02 13:34 Ming Lei
2021-11-02 13:35 ` [PATCH 1/3] blk-mq: only try to run plug merge if request has same queue with incoming bio Ming Lei
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Ming Lei @ 2021-11-02 13:34 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei
Hello,
Here are 3 small fixes against latest linus tree.
Ming Lei (3):
blk-mq: only try to run plug merge if request has same queue with
incoming bio
blk-mq: add RQF_ELV debug entry
blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
block/blk-merge.c | 6 ++++--
block/blk-mq-debugfs.c | 1 +
block/blk-mq.c | 15 +++++++++++++--
block/blk-mq.h | 12 +++++++++---
4 files changed, 27 insertions(+), 7 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] blk-mq: only try to run plug merge if request has same queue with incoming bio
2021-11-02 13:34 [PATCH 0/3] blk-mq: misc fixes Ming Lei
@ 2021-11-02 13:35 ` Ming Lei
2021-11-02 13:35 ` [PATCH 2/3] blk-mq: add RQF_ELV debug entry Ming Lei
2021-11-02 13:35 ` [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch() Ming Lei
2 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-11-02 13:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei
It is obvious that io merge can't be done between two different queues, so
just try to run io merge in case of same queue.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-merge.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index df69f4bb7717..893c1a60b701 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -1101,9 +1101,11 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
* the same queue, there should be only one such rq in a queue
*/
*same_queue_rq = true;
+
+ if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) ==
+ BIO_MERGE_OK)
+ return true;
}
- if (blk_attempt_bio_merge(q, rq, bio, nr_segs, false) == BIO_MERGE_OK)
- return true;
return false;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] blk-mq: add RQF_ELV debug entry
2021-11-02 13:34 [PATCH 0/3] blk-mq: misc fixes Ming Lei
2021-11-02 13:35 ` [PATCH 1/3] blk-mq: only try to run plug merge if request has same queue with incoming bio Ming Lei
@ 2021-11-02 13:35 ` Ming Lei
2021-11-02 13:35 ` [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch() Ming Lei
2 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-11-02 13:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei
Looks it is missed so add it.
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-mq-debugfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index f5076c173477..4f2cf8399f3d 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -308,6 +308,7 @@ static const char *const rqf_name[] = {
RQF_NAME(SPECIAL_PAYLOAD),
RQF_NAME(ZONE_WRITE_LOCKED),
RQF_NAME(MQ_POLL_SLEPT),
+ RQF_NAME(ELV),
};
#undef RQF_NAME
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 13:34 [PATCH 0/3] blk-mq: misc fixes Ming Lei
2021-11-02 13:35 ` [PATCH 1/3] blk-mq: only try to run plug merge if request has same queue with incoming bio Ming Lei
2021-11-02 13:35 ` [PATCH 2/3] blk-mq: add RQF_ELV debug entry Ming Lei
@ 2021-11-02 13:35 ` Ming Lei
2021-11-02 13:47 ` Jens Axboe
2 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2021-11-02 13:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei, Shinichiro Kawasaki
In case of shared tags and none io sched, batched completion still may
be run into, and hctx->nr_active is accounted when getting driver tag,
so it has to be updated in blk_mq_end_request_batch().
Otherwise, hctx->nr_active may become same with queue depth, then
hctx_may_queue() always return false, then io hang is caused.
Fixes the issue by updating the counter in batched way.
Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
block/blk-mq.c | 15 +++++++++++++--
block/blk-mq.h | 12 +++++++++---
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 07eb1412760b..0dbe75034f61 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
struct blk_mq_hw_ctx *cur_hctx = NULL;
struct request *rq;
u64 now = 0;
+ int active = 0;
if (iob->need_ts)
now = ktime_get_ns();
@@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
rq_qos_done(rq->q, rq);
if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
- if (cur_hctx)
+ if (cur_hctx) {
+ if (active)
+ __blk_mq_sub_active_requests(cur_hctx,
+ active);
blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
+ }
nr_tags = 0;
+ active = 0;
cur_hctx = rq->mq_hctx;
}
tags[nr_tags++] = rq->tag;
+ if (rq->rq_flags & RQF_MQ_INFLIGHT)
+ active++;
}
- if (nr_tags)
+ if (nr_tags) {
+ if (active)
+ __blk_mq_sub_active_requests(cur_hctx, active);
blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
+ }
}
EXPORT_SYMBOL_GPL(blk_mq_end_request_batch);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 28859fc5faee..cb0b5482ca5e 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -225,12 +225,18 @@ static inline void __blk_mq_inc_active_requests(struct blk_mq_hw_ctx *hctx)
atomic_inc(&hctx->nr_active);
}
-static inline void __blk_mq_dec_active_requests(struct blk_mq_hw_ctx *hctx)
+static inline void __blk_mq_sub_active_requests(struct blk_mq_hw_ctx *hctx,
+ int val)
{
if (blk_mq_is_shared_tags(hctx->flags))
- atomic_dec(&hctx->queue->nr_active_requests_shared_tags);
+ atomic_sub(val, &hctx->queue->nr_active_requests_shared_tags);
else
- atomic_dec(&hctx->nr_active);
+ atomic_sub(val, &hctx->nr_active);
+}
+
+static inline void __blk_mq_dec_active_requests(struct blk_mq_hw_ctx *hctx)
+{
+ __blk_mq_sub_active_requests(hctx, 1);
}
static inline int __blk_mq_active_requests(struct blk_mq_hw_ctx *hctx)
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 13:35 ` [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch() Ming Lei
@ 2021-11-02 13:47 ` Jens Axboe
2021-11-02 13:57 ` Ming Lei
0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-11-02 13:47 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Shinichiro Kawasaki
On 11/2/21 7:35 AM, Ming Lei wrote:
> In case of shared tags and none io sched, batched completion still may
> be run into, and hctx->nr_active is accounted when getting driver tag,
> so it has to be updated in blk_mq_end_request_batch().
>
> Otherwise, hctx->nr_active may become same with queue depth, then
> hctx_may_queue() always return false, then io hang is caused.
>
> Fixes the issue by updating the counter in batched way.
>
> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> block/blk-mq.c | 15 +++++++++++++--
> block/blk-mq.h | 12 +++++++++---
> 2 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 07eb1412760b..0dbe75034f61 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> struct blk_mq_hw_ctx *cur_hctx = NULL;
> struct request *rq;
> u64 now = 0;
> + int active = 0;
>
> if (iob->need_ts)
> now = ktime_get_ns();
> @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> rq_qos_done(rq->q, rq);
>
> if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
> - if (cur_hctx)
> + if (cur_hctx) {
> + if (active)
> + __blk_mq_sub_active_requests(cur_hctx,
> + active);
> blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
> + }
> nr_tags = 0;
> + active = 0;
> cur_hctx = rq->mq_hctx;
> }
> tags[nr_tags++] = rq->tag;
> + if (rq->rq_flags & RQF_MQ_INFLIGHT)
> + active++;
Are there any cases where either none or all of requests have the flag set, and
hence active == nr_tags?
Apart from that, the patch looks conceptually good.
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 13:47 ` Jens Axboe
@ 2021-11-02 13:57 ` Ming Lei
2021-11-02 14:57 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2021-11-02 13:57 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Shinichiro Kawasaki, ming.lei
On Tue, Nov 02, 2021 at 07:47:44AM -0600, Jens Axboe wrote:
> On 11/2/21 7:35 AM, Ming Lei wrote:
> > In case of shared tags and none io sched, batched completion still may
> > be run into, and hctx->nr_active is accounted when getting driver tag,
> > so it has to be updated in blk_mq_end_request_batch().
> >
> > Otherwise, hctx->nr_active may become same with queue depth, then
> > hctx_may_queue() always return false, then io hang is caused.
> >
> > Fixes the issue by updating the counter in batched way.
> >
> > Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > block/blk-mq.c | 15 +++++++++++++--
> > block/blk-mq.h | 12 +++++++++---
> > 2 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 07eb1412760b..0dbe75034f61 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> > struct blk_mq_hw_ctx *cur_hctx = NULL;
> > struct request *rq;
> > u64 now = 0;
> > + int active = 0;
> >
> > if (iob->need_ts)
> > now = ktime_get_ns();
> > @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> > rq_qos_done(rq->q, rq);
> >
> > if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
> > - if (cur_hctx)
> > + if (cur_hctx) {
> > + if (active)
> > + __blk_mq_sub_active_requests(cur_hctx,
> > + active);
> > blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
> > + }
> > nr_tags = 0;
> > + active = 0;
> > cur_hctx = rq->mq_hctx;
> > }
> > tags[nr_tags++] = rq->tag;
> > + if (rq->rq_flags & RQF_MQ_INFLIGHT)
> > + active++;
>
> Are there any cases where either none or all of requests have the flag set, and
> hence active == nr_tags?
none and BLK_MQ_F_TAG_QUEUE_SHARED, and Shinichiro only observed the
issue on two NSs.
Thanks,
Ming
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 13:57 ` Ming Lei
@ 2021-11-02 14:57 ` Jens Axboe
2021-11-02 15:08 ` Ming Lei
0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-11-02 14:57 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Shinichiro Kawasaki
On 11/2/21 7:57 AM, Ming Lei wrote:
> On Tue, Nov 02, 2021 at 07:47:44AM -0600, Jens Axboe wrote:
>> On 11/2/21 7:35 AM, Ming Lei wrote:
>>> In case of shared tags and none io sched, batched completion still may
>>> be run into, and hctx->nr_active is accounted when getting driver tag,
>>> so it has to be updated in blk_mq_end_request_batch().
>>>
>>> Otherwise, hctx->nr_active may become same with queue depth, then
>>> hctx_may_queue() always return false, then io hang is caused.
>>>
>>> Fixes the issue by updating the counter in batched way.
>>>
>>> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>> Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>> block/blk-mq.c | 15 +++++++++++++--
>>> block/blk-mq.h | 12 +++++++++---
>>> 2 files changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 07eb1412760b..0dbe75034f61 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>> struct blk_mq_hw_ctx *cur_hctx = NULL;
>>> struct request *rq;
>>> u64 now = 0;
>>> + int active = 0;
>>>
>>> if (iob->need_ts)
>>> now = ktime_get_ns();
>>> @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>> rq_qos_done(rq->q, rq);
>>>
>>> if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
>>> - if (cur_hctx)
>>> + if (cur_hctx) {
>>> + if (active)
>>> + __blk_mq_sub_active_requests(cur_hctx,
>>> + active);
>>> blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
>>> + }
>>> nr_tags = 0;
>>> + active = 0;
>>> cur_hctx = rq->mq_hctx;
>>> }
>>> tags[nr_tags++] = rq->tag;
>>> + if (rq->rq_flags & RQF_MQ_INFLIGHT)
>>> + active++;
>>
>> Are there any cases where either none or all of requests have the
>> flag set, and hence active == nr_tags?
>
> none and BLK_MQ_F_TAG_QUEUE_SHARED, and Shinichiro only observed the
> issue on two NSs.
Maybe I wasn't clear enough. What I'm saying is that either all of the
requests will have RQF_MQ_INFLIGHT set, or none of them. Hence active
should be either 0, or == nr_tags.
That's the hypothesis that I wanted to check, because if that's true,
then we can do this in a better way.
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 14:57 ` Jens Axboe
@ 2021-11-02 15:08 ` Ming Lei
2021-11-02 15:20 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2021-11-02 15:08 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Shinichiro Kawasaki, ming.lei
On Tue, Nov 02, 2021 at 08:57:41AM -0600, Jens Axboe wrote:
> On 11/2/21 7:57 AM, Ming Lei wrote:
> > On Tue, Nov 02, 2021 at 07:47:44AM -0600, Jens Axboe wrote:
> >> On 11/2/21 7:35 AM, Ming Lei wrote:
> >>> In case of shared tags and none io sched, batched completion still may
> >>> be run into, and hctx->nr_active is accounted when getting driver tag,
> >>> so it has to be updated in blk_mq_end_request_batch().
> >>>
> >>> Otherwise, hctx->nr_active may become same with queue depth, then
> >>> hctx_may_queue() always return false, then io hang is caused.
> >>>
> >>> Fixes the issue by updating the counter in batched way.
> >>>
> >>> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >>> Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>> block/blk-mq.c | 15 +++++++++++++--
> >>> block/blk-mq.h | 12 +++++++++---
> >>> 2 files changed, 22 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >>> index 07eb1412760b..0dbe75034f61 100644
> >>> --- a/block/blk-mq.c
> >>> +++ b/block/blk-mq.c
> >>> @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> >>> struct blk_mq_hw_ctx *cur_hctx = NULL;
> >>> struct request *rq;
> >>> u64 now = 0;
> >>> + int active = 0;
> >>>
> >>> if (iob->need_ts)
> >>> now = ktime_get_ns();
> >>> @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> >>> rq_qos_done(rq->q, rq);
> >>>
> >>> if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
> >>> - if (cur_hctx)
> >>> + if (cur_hctx) {
> >>> + if (active)
> >>> + __blk_mq_sub_active_requests(cur_hctx,
> >>> + active);
> >>> blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
> >>> + }
> >>> nr_tags = 0;
> >>> + active = 0;
> >>> cur_hctx = rq->mq_hctx;
> >>> }
> >>> tags[nr_tags++] = rq->tag;
> >>> + if (rq->rq_flags & RQF_MQ_INFLIGHT)
> >>> + active++;
> >>
> >> Are there any cases where either none or all of requests have the
> >> flag set, and hence active == nr_tags?
> >
> > none and BLK_MQ_F_TAG_QUEUE_SHARED, and Shinichiro only observed the
> > issue on two NSs.
>
> Maybe I wasn't clear enough. What I'm saying is that either all of the
> requests will have RQF_MQ_INFLIGHT set, or none of them. Hence active
> should be either 0, or == nr_tags.
Yeah, that is right since BLK_MQ_F_TAG_QUEUE_SHARED is updated after
queue is frozen. Meantime blk_mq_end_request_batch() is only called
for ending successfully completed requests.
Will do that in V2.
Thanks,
Ming
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 15:08 ` Ming Lei
@ 2021-11-02 15:20 ` Jens Axboe
2021-11-02 15:23 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2021-11-02 15:20 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Shinichiro Kawasaki
On 11/2/21 9:08 AM, Ming Lei wrote:
> On Tue, Nov 02, 2021 at 08:57:41AM -0600, Jens Axboe wrote:
>> On 11/2/21 7:57 AM, Ming Lei wrote:
>>> On Tue, Nov 02, 2021 at 07:47:44AM -0600, Jens Axboe wrote:
>>>> On 11/2/21 7:35 AM, Ming Lei wrote:
>>>>> In case of shared tags and none io sched, batched completion still may
>>>>> be run into, and hctx->nr_active is accounted when getting driver tag,
>>>>> so it has to be updated in blk_mq_end_request_batch().
>>>>>
>>>>> Otherwise, hctx->nr_active may become same with queue depth, then
>>>>> hctx_may_queue() always return false, then io hang is caused.
>>>>>
>>>>> Fixes the issue by updating the counter in batched way.
>>>>>
>>>>> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>>>> Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>> block/blk-mq.c | 15 +++++++++++++--
>>>>> block/blk-mq.h | 12 +++++++++---
>>>>> 2 files changed, 22 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index 07eb1412760b..0dbe75034f61 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>>>> struct blk_mq_hw_ctx *cur_hctx = NULL;
>>>>> struct request *rq;
>>>>> u64 now = 0;
>>>>> + int active = 0;
>>>>>
>>>>> if (iob->need_ts)
>>>>> now = ktime_get_ns();
>>>>> @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>>>> rq_qos_done(rq->q, rq);
>>>>>
>>>>> if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
>>>>> - if (cur_hctx)
>>>>> + if (cur_hctx) {
>>>>> + if (active)
>>>>> + __blk_mq_sub_active_requests(cur_hctx,
>>>>> + active);
>>>>> blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
>>>>> + }
>>>>> nr_tags = 0;
>>>>> + active = 0;
>>>>> cur_hctx = rq->mq_hctx;
>>>>> }
>>>>> tags[nr_tags++] = rq->tag;
>>>>> + if (rq->rq_flags & RQF_MQ_INFLIGHT)
>>>>> + active++;
>>>>
>>>> Are there any cases where either none or all of requests have the
>>>> flag set, and hence active == nr_tags?
>>>
>>> none and BLK_MQ_F_TAG_QUEUE_SHARED, and Shinichiro only observed the
>>> issue on two NSs.
>>
>> Maybe I wasn't clear enough. What I'm saying is that either all of the
>> requests will have RQF_MQ_INFLIGHT set, or none of them. Hence active
>> should be either 0, or == nr_tags.
>
> Yeah, that is right since BLK_MQ_F_TAG_QUEUE_SHARED is updated after
> queue is frozen. Meantime blk_mq_end_request_batch() is only called
> for ending successfully completed requests.
>
> Will do that in V2.
Thanks, then it just becomes a single check in blk_mq_flush_tag_batch(),
which is a lot better than per-request.
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch()
2021-11-02 15:20 ` Jens Axboe
@ 2021-11-02 15:23 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2021-11-02 15:23 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Shinichiro Kawasaki
On 11/2/21 9:20 AM, Jens Axboe wrote:
> On 11/2/21 9:08 AM, Ming Lei wrote:
>> On Tue, Nov 02, 2021 at 08:57:41AM -0600, Jens Axboe wrote:
>>> On 11/2/21 7:57 AM, Ming Lei wrote:
>>>> On Tue, Nov 02, 2021 at 07:47:44AM -0600, Jens Axboe wrote:
>>>>> On 11/2/21 7:35 AM, Ming Lei wrote:
>>>>>> In case of shared tags and none io sched, batched completion still may
>>>>>> be run into, and hctx->nr_active is accounted when getting driver tag,
>>>>>> so it has to be updated in blk_mq_end_request_batch().
>>>>>>
>>>>>> Otherwise, hctx->nr_active may become same with queue depth, then
>>>>>> hctx_may_queue() always return false, then io hang is caused.
>>>>>>
>>>>>> Fixes the issue by updating the counter in batched way.
>>>>>>
>>>>>> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
>>>>>> Fixes: f794f3351f26 ("block: add support for blk_mq_end_request_batch()")
>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>>> ---
>>>>>> block/blk-mq.c | 15 +++++++++++++--
>>>>>> block/blk-mq.h | 12 +++++++++---
>>>>>> 2 files changed, 22 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>>> index 07eb1412760b..0dbe75034f61 100644
>>>>>> --- a/block/blk-mq.c
>>>>>> +++ b/block/blk-mq.c
>>>>>> @@ -825,6 +825,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>>>>> struct blk_mq_hw_ctx *cur_hctx = NULL;
>>>>>> struct request *rq;
>>>>>> u64 now = 0;
>>>>>> + int active = 0;
>>>>>>
>>>>>> if (iob->need_ts)
>>>>>> now = ktime_get_ns();
>>>>>> @@ -846,16 +847,26 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
>>>>>> rq_qos_done(rq->q, rq);
>>>>>>
>>>>>> if (nr_tags == TAG_COMP_BATCH || cur_hctx != rq->mq_hctx) {
>>>>>> - if (cur_hctx)
>>>>>> + if (cur_hctx) {
>>>>>> + if (active)
>>>>>> + __blk_mq_sub_active_requests(cur_hctx,
>>>>>> + active);
>>>>>> blk_mq_flush_tag_batch(cur_hctx, tags, nr_tags);
>>>>>> + }
>>>>>> nr_tags = 0;
>>>>>> + active = 0;
>>>>>> cur_hctx = rq->mq_hctx;
>>>>>> }
>>>>>> tags[nr_tags++] = rq->tag;
>>>>>> + if (rq->rq_flags & RQF_MQ_INFLIGHT)
>>>>>> + active++;
>>>>>
>>>>> Are there any cases where either none or all of requests have the
>>>>> flag set, and hence active == nr_tags?
>>>>
>>>> none and BLK_MQ_F_TAG_QUEUE_SHARED, and Shinichiro only observed the
>>>> issue on two NSs.
>>>
>>> Maybe I wasn't clear enough. What I'm saying is that either all of the
>>> requests will have RQF_MQ_INFLIGHT set, or none of them. Hence active
>>> should be either 0, or == nr_tags.
>>
>> Yeah, that is right since BLK_MQ_F_TAG_QUEUE_SHARED is updated after
>> queue is frozen. Meantime blk_mq_end_request_batch() is only called
>> for ending successfully completed requests.
>>
>> Will do that in V2.
>
> Thanks, then it just becomes a single check in blk_mq_flush_tag_batch(),
> which is a lot better than per-request.
Something like this, untested. FWIW, I did apply 1-2 from this series,
so just do a v2 of 3/3 and that should do it.
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 02f70dc06ced..18dee9af4487 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -817,6 +817,8 @@ static inline void blk_mq_flush_tag_batch(struct blk_mq_hw_ctx *hctx,
struct request_queue *q = hctx->queue;
blk_mq_put_tags(hctx->tags, tag_array, nr_tags);
+ if (hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)
+ __blk_mq_sub_active_requests(hctx, nr_tags);
percpu_ref_put_many(&q->q_usage_counter, nr_tags);
}
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 28859fc5faee..cb0b5482ca5e 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -225,12 +225,18 @@ static inline void __blk_mq_inc_active_requests(struct blk_mq_hw_ctx *hctx)
atomic_inc(&hctx->nr_active);
}
-static inline void __blk_mq_dec_active_requests(struct blk_mq_hw_ctx *hctx)
+static inline void __blk_mq_sub_active_requests(struct blk_mq_hw_ctx *hctx,
+ int val)
{
if (blk_mq_is_shared_tags(hctx->flags))
- atomic_dec(&hctx->queue->nr_active_requests_shared_tags);
+ atomic_sub(val, &hctx->queue->nr_active_requests_shared_tags);
else
- atomic_dec(&hctx->nr_active);
+ atomic_sub(val, &hctx->nr_active);
+}
+
+static inline void __blk_mq_dec_active_requests(struct blk_mq_hw_ctx *hctx)
+{
+ __blk_mq_sub_active_requests(hctx, 1);
}
static inline int __blk_mq_active_requests(struct blk_mq_hw_ctx *hctx)
--
Jens Axboe
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-02 15:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-02 13:34 [PATCH 0/3] blk-mq: misc fixes Ming Lei
2021-11-02 13:35 ` [PATCH 1/3] blk-mq: only try to run plug merge if request has same queue with incoming bio Ming Lei
2021-11-02 13:35 ` [PATCH 2/3] blk-mq: add RQF_ELV debug entry Ming Lei
2021-11-02 13:35 ` [PATCH 3/3] blk-mq: update hctx->nr_active in blk_mq_end_request_batch() Ming Lei
2021-11-02 13:47 ` Jens Axboe
2021-11-02 13:57 ` Ming Lei
2021-11-02 14:57 ` Jens Axboe
2021-11-02 15:08 ` Ming Lei
2021-11-02 15:20 ` Jens Axboe
2021-11-02 15:23 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).