| Message ID | 20210301021444.4134047-3-yuyufen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: improve blk_mq_tag_to_rq() | expand |
On 2021/03/01 11:12, Yufen Yu wrote: > We just set hctx->tags->rqs[tag] when get driver tag, but will > not set hctx->sched_tags->rqs[tag] when get sched tag. > So, blk_mq_tag_to_rq() always return NULL for sched_tags. > > Signed-off-by: Yufen Yu <yuyufen@huawei.com> > --- > block/blk-mq.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 5362a7958b74..424afe112b58 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -846,6 +846,7 @@ static int blk_mq_test_tag_bit(struct blk_mq_tags *tags, unsigned int tag) > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > { > + /* if tags is hctx->sched_tags, it always return NULL */ > if (tag < tags->nr_tags && blk_mq_test_tag_bit(tags, tag)) { > prefetch(tags->rqs[tag]); > return tags->rqs[tag]; > @@ -3845,17 +3846,8 @@ static bool blk_mq_poll_hybrid(struct request_queue *q, > > if (!blk_qc_t_is_internal(cookie)) > rq = blk_mq_tag_to_rq(hctx->tags, blk_qc_t_to_tag(cookie)); > - else { > - rq = blk_mq_tag_to_rq(hctx->sched_tags, blk_qc_t_to_tag(cookie)); > - /* > - * With scheduling, if the request has completed, we'll > - * get a NULL return here, as we clear the sched tag when > - * that happens. The request still remains valid, like always, > - * so we should be safe with just the NULL check. > - */ > - if (!rq) > - return false; > - } > + else > + return false; Reverse the if condition to avoid the "else". That will nicely cleanup the code. > > return blk_mq_poll_hybrid_sleep(q, rq); > } >
On Sun, Feb 28, 2021 at 09:14:44PM -0500, Yufen Yu wrote: > We just set hctx->tags->rqs[tag] when get driver tag, but will > not set hctx->sched_tags->rqs[tag] when get sched tag. > So, blk_mq_tag_to_rq() always return NULL for sched_tags. True, also blk_mq_tag_to_rq() seems an awkward API, and it needs 'struct blk_mq_tags *', but which is a block layer internal definition. > > Signed-off-by: Yufen Yu <yuyufen@huawei.com> > --- > block/blk-mq.c | 14 +++----------- > 1 file changed, 3 insertions(+), 11 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 5362a7958b74..424afe112b58 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -846,6 +846,7 @@ static int blk_mq_test_tag_bit(struct blk_mq_tags *tags, unsigned int tag) > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > { > + /* if tags is hctx->sched_tags, it always return NULL */ > if (tag < tags->nr_tags && blk_mq_test_tag_bit(tags, tag)) { > prefetch(tags->rqs[tag]); > return tags->rqs[tag]; > @@ -3845,17 +3846,8 @@ static bool blk_mq_poll_hybrid(struct request_queue *q, > > if (!blk_qc_t_is_internal(cookie)) > rq = blk_mq_tag_to_rq(hctx->tags, blk_qc_t_to_tag(cookie)); > - else { > - rq = blk_mq_tag_to_rq(hctx->sched_tags, blk_qc_t_to_tag(cookie)); > - /* > - * With scheduling, if the request has completed, we'll > - * get a NULL return here, as we clear the sched tag when > - * that happens. The request still remains valid, like always, > - * so we should be safe with just the NULL check. > - */ > - if (!rq) > - return false; > - } > + else > + return false; > I think the correct fix is to retrieve the request via: hctx->sched_tags->static_rqs[blk_qc_t_to_tag(cookie)] since it is nice to run blk_mq_poll_hybrid_sleep() for one non-started request in case of real scheduler.
On 2021/3/1 14:50, Ming Lei wrote: > On Sun, Feb 28, 2021 at 09:14:44PM -0500, Yufen Yu wrote: >> We just set hctx->tags->rqs[tag] when get driver tag, but will >> not set hctx->sched_tags->rqs[tag] when get sched tag. >> So, blk_mq_tag_to_rq() always return NULL for sched_tags. > > True, also blk_mq_tag_to_rq() seems an awkward API, and it needs > 'struct blk_mq_tags *', but which is a block layer internal definition. > >> >> Signed-off-by: Yufen Yu <yuyufen@huawei.com> >> --- >> block/blk-mq.c | 14 +++----------- >> 1 file changed, 3 insertions(+), 11 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 5362a7958b74..424afe112b58 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -846,6 +846,7 @@ static int blk_mq_test_tag_bit(struct blk_mq_tags *tags, unsigned int tag) >> >> struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) >> { >> + /* if tags is hctx->sched_tags, it always return NULL */ >> if (tag < tags->nr_tags && blk_mq_test_tag_bit(tags, tag)) { >> prefetch(tags->rqs[tag]); >> return tags->rqs[tag]; >> @@ -3845,17 +3846,8 @@ static bool blk_mq_poll_hybrid(struct request_queue *q, >> >> if (!blk_qc_t_is_internal(cookie)) >> rq = blk_mq_tag_to_rq(hctx->tags, blk_qc_t_to_tag(cookie)); >> - else { >> - rq = blk_mq_tag_to_rq(hctx->sched_tags, blk_qc_t_to_tag(cookie)); >> - /* >> - * With scheduling, if the request has completed, we'll >> - * get a NULL return here, as we clear the sched tag when >> - * that happens. The request still remains valid, like always, >> - * so we should be safe with just the NULL check. >> - */ >> - if (!rq) >> - return false; >> - } >> + else >> + return false; >> > > I think the correct fix is to retrieve the request via: > > hctx->sched_tags->static_rqs[blk_qc_t_to_tag(cookie)] > > since it is nice to run blk_mq_poll_hybrid_sleep() for one > non-started request in case of real scheduler. > Yes, do blk_mq_poll_hybrid_sleep() should be more reasonable here. I will modify it in next version. Thanks, Yufen
diff --git a/block/blk-mq.c b/block/blk-mq.c index 5362a7958b74..424afe112b58 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -846,6 +846,7 @@ static int blk_mq_test_tag_bit(struct blk_mq_tags *tags, unsigned int tag) struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) { + /* if tags is hctx->sched_tags, it always return NULL */ if (tag < tags->nr_tags && blk_mq_test_tag_bit(tags, tag)) { prefetch(tags->rqs[tag]); return tags->rqs[tag]; @@ -3845,17 +3846,8 @@ static bool blk_mq_poll_hybrid(struct request_queue *q, if (!blk_qc_t_is_internal(cookie)) rq = blk_mq_tag_to_rq(hctx->tags, blk_qc_t_to_tag(cookie)); - else { - rq = blk_mq_tag_to_rq(hctx->sched_tags, blk_qc_t_to_tag(cookie)); - /* - * With scheduling, if the request has completed, we'll - * get a NULL return here, as we clear the sched tag when - * that happens. The request still remains valid, like always, - * so we should be safe with just the NULL check. - */ - if (!rq) - return false; - } + else + return false; return blk_mq_poll_hybrid_sleep(q, rq); }
We just set hctx->tags->rqs[tag] when get driver tag, but will not set hctx->sched_tags->rqs[tag] when get sched tag. So, blk_mq_tag_to_rq() always return NULL for sched_tags. Signed-off-by: Yufen Yu <yuyufen@huawei.com> --- block/blk-mq.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)