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