| Message ID | 20180802182944.14442-9-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Enable runtime power management | expand |
On Thu, Aug 02, 2018 at 11:29:43AM -0700, Bart Van Assche wrote: > Make sure that blk_pm_add_request() is called exactly once before > a request is added to a software queue, to the scheduler and also > before .queue_rq() is called directly. Call blk_pm_put_request() > after a request has finished. > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Jianchao Wang <jianchao.w.wang@oracle.com> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > --- > block/blk-mq-sched.c | 13 +++++++++++-- > block/blk-mq.c | 8 ++++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c > index cf9c66c6d35a..d87839b31d56 100644 > --- a/block/blk-mq-sched.c > +++ b/block/blk-mq-sched.c > @@ -14,6 +14,7 @@ > #include "blk-mq-debugfs.h" > #include "blk-mq-sched.h" > #include "blk-mq-tag.h" > +#include "blk-pm.h" > #include "blk-wbt.h" > > void blk_mq_sched_free_hctx_data(struct request_queue *q, > @@ -349,6 +350,8 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx, > { > /* dispatch flush rq directly */ > if (rq->rq_flags & RQF_FLUSH_SEQ) { > + blk_pm_add_request(rq->q, rq); > + > spin_lock(&hctx->lock); > list_add(&rq->queuelist, &hctx->dispatch); > spin_unlock(&hctx->lock); > @@ -380,6 +383,8 @@ void blk_mq_sched_insert_request(struct request *rq, bool at_head, > if (blk_mq_sched_bypass_insert(hctx, !!e, rq)) > goto run; > > + blk_pm_add_request(q, rq); > + > if (e && e->type->ops.mq.insert_requests) { > LIST_HEAD(list); > > @@ -402,10 +407,14 @@ void blk_mq_sched_insert_requests(struct request_queue *q, > { > struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu); > struct elevator_queue *e = hctx->queue->elevator; > + struct request *rq; > + > + if (e && e->type->ops.mq.insert_requests) { > + list_for_each_entry(rq, list, queuelist) > + blk_pm_add_request(q, rq); > > - if (e && e->type->ops.mq.insert_requests) > e->type->ops.mq.insert_requests(hctx, list, false); > - else { > + } else { > /* > * try to issue requests directly if the hw queue isn't > * busy in case of 'none' scheduler, and this way may save > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 8d845872ea02..592d81c37b07 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -36,6 +36,7 @@ > #include "blk-mq-tag.h" > #include "blk-stat.h" > #include "blk-mq-sched.h" > +#include "blk-pm.h" > #include "blk-rq-qos.h" > > static bool blk_mq_poll(struct request_queue *q, blk_qc_t cookie); > @@ -476,6 +477,8 @@ static void __blk_mq_free_request(struct request *rq) > struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu); > const int sched_tag = rq->internal_tag; > > + blk_pm_put_request(rq); > + > if (rq->tag != -1) > blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag); > if (sched_tag != -1) > @@ -1565,6 +1568,8 @@ void blk_mq_request_bypass_insert(struct request *rq, bool run_queue) > struct blk_mq_ctx *ctx = rq->mq_ctx; > struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu); > > + blk_pm_add_request(rq->q, rq); > + > spin_lock(&hctx->lock); > list_add_tail(&rq->queuelist, &hctx->dispatch); > spin_unlock(&hctx->lock); > @@ -1586,6 +1591,7 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx, > list_for_each_entry(rq, list, queuelist) { > BUG_ON(rq->mq_ctx != ctx); > trace_block_rq_insert(hctx->queue, rq); > + blk_pm_add_request(rq->q, rq); > } > > spin_lock(&ctx->lock); > @@ -1682,6 +1688,8 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, > blk_qc_t new_cookie; > blk_status_t ret; > > + blk_pm_add_request(q, rq); > + > new_cookie = request_to_qc_t(hctx, rq); > blk_pm_add_request() calls pm_request_resume() for waking up device, but it is wrong because it is async request, which can't guarantee device will be ready before calling .queue_rq(). Thanks, Ming
On Fri, 2018-08-03 at 07:53 +0800, Ming Lei wrote: > blk_pm_add_request() calls pm_request_resume() for waking up device, but > it is wrong because it is async request, which can't guarantee device > will be ready before calling .queue_rq(). That's a longstanding issue and not something that has been introduced by this patch series. Hence patch 6/9 that issues a warning if pm_request_resume() would be called. Bart.
On Fri, Aug 03, 2018 at 12:08:54AM +0000, Bart Van Assche wrote: > On Fri, 2018-08-03 at 07:53 +0800, Ming Lei wrote: > > blk_pm_add_request() calls pm_request_resume() for waking up device, but > > it is wrong because it is async request, which can't guarantee device > > will be ready before calling .queue_rq(). > > That's a longstanding issue and not something that has been introduced by this > patch series. Hence patch 6/9 that issues a warning if pm_request_resume() would > be called. It is introduced by your patch, please see blk_pm_allow_request(). Thanks, Ming
On Fri, 2018-08-03 at 08:11 +0800, Ming Lei wrote: > On Fri, Aug 03, 2018 at 12:08:54AM +0000, Bart Van Assche wrote: > > On Fri, 2018-08-03 at 07:53 +0800, Ming Lei wrote: > > > blk_pm_add_request() calls pm_request_resume() for waking up device, but > > > it is wrong because it is async request, which can't guarantee device > > > will be ready before calling .queue_rq(). > > > > That's a longstanding issue and not something that has been introduced by this > > patch series. Hence patch 6/9 that issues a warning if pm_request_resume() would > > be called. > > It is introduced by your patch, please see blk_pm_allow_request(). Did you perhaps misread my patch? Please have a look at commit c8158819d506 ("block: implement runtime pm strategy"). I think that commit from 2013 introduced the pm_request_resume() call in blk_pm_add_request(). By the way, if you would like to see that call removed, I'm fine with removing that call. Thanks, Bart.
On Fri, Aug 03, 2018 at 01:03:36AM +0000, Bart Van Assche wrote: > On Fri, 2018-08-03 at 08:11 +0800, Ming Lei wrote: > > On Fri, Aug 03, 2018 at 12:08:54AM +0000, Bart Van Assche wrote: > > > On Fri, 2018-08-03 at 07:53 +0800, Ming Lei wrote: > > > > blk_pm_add_request() calls pm_request_resume() for waking up device, but > > > > it is wrong because it is async request, which can't guarantee device > > > > will be ready before calling .queue_rq(). > > > > > > That's a longstanding issue and not something that has been introduced by this > > > patch series. Hence patch 6/9 that issues a warning if pm_request_resume() would > > > be called. > > > > It is introduced by your patch, please see blk_pm_allow_request(). > > Did you perhaps misread my patch? Please have a look at commit c8158819d506 > ("block: implement runtime pm strategy"). I think that commit from 2013 > introduced the pm_request_resume() call in blk_pm_add_request(). > > By the way, if you would like to see that call removed, I'm fine with > removing that call. pm_request_resume() isn't used wrong now, and the issue is introduced by removing blk_pm_allow_request(), which is called in elv_next_request() for avoiding this issue, so that requests can be kept in scheduler queue if device isn't active. Thanks, Ming
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index cf9c66c6d35a..d87839b31d56 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -14,6 +14,7 @@ #include "blk-mq-debugfs.h" #include "blk-mq-sched.h" #include "blk-mq-tag.h" +#include "blk-pm.h" #include "blk-wbt.h" void blk_mq_sched_free_hctx_data(struct request_queue *q, @@ -349,6 +350,8 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx, { /* dispatch flush rq directly */ if (rq->rq_flags & RQF_FLUSH_SEQ) { + blk_pm_add_request(rq->q, rq); + spin_lock(&hctx->lock); list_add(&rq->queuelist, &hctx->dispatch); spin_unlock(&hctx->lock); @@ -380,6 +383,8 @@ void blk_mq_sched_insert_request(struct request *rq, bool at_head, if (blk_mq_sched_bypass_insert(hctx, !!e, rq)) goto run; + blk_pm_add_request(q, rq); + if (e && e->type->ops.mq.insert_requests) { LIST_HEAD(list); @@ -402,10 +407,14 @@ void blk_mq_sched_insert_requests(struct request_queue *q, { struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu); struct elevator_queue *e = hctx->queue->elevator; + struct request *rq; + + if (e && e->type->ops.mq.insert_requests) { + list_for_each_entry(rq, list, queuelist) + blk_pm_add_request(q, rq); - if (e && e->type->ops.mq.insert_requests) e->type->ops.mq.insert_requests(hctx, list, false); - else { + } else { /* * try to issue requests directly if the hw queue isn't * busy in case of 'none' scheduler, and this way may save diff --git a/block/blk-mq.c b/block/blk-mq.c index 8d845872ea02..592d81c37b07 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -36,6 +36,7 @@ #include "blk-mq-tag.h" #include "blk-stat.h" #include "blk-mq-sched.h" +#include "blk-pm.h" #include "blk-rq-qos.h" static bool blk_mq_poll(struct request_queue *q, blk_qc_t cookie); @@ -476,6 +477,8 @@ static void __blk_mq_free_request(struct request *rq) struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu); const int sched_tag = rq->internal_tag; + blk_pm_put_request(rq); + if (rq->tag != -1) blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag); if (sched_tag != -1) @@ -1565,6 +1568,8 @@ void blk_mq_request_bypass_insert(struct request *rq, bool run_queue) struct blk_mq_ctx *ctx = rq->mq_ctx; struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(rq->q, ctx->cpu); + blk_pm_add_request(rq->q, rq); + spin_lock(&hctx->lock); list_add_tail(&rq->queuelist, &hctx->dispatch); spin_unlock(&hctx->lock); @@ -1586,6 +1591,7 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx, list_for_each_entry(rq, list, queuelist) { BUG_ON(rq->mq_ctx != ctx); trace_block_rq_insert(hctx->queue, rq); + blk_pm_add_request(rq->q, rq); } spin_lock(&ctx->lock); @@ -1682,6 +1688,8 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, blk_qc_t new_cookie; blk_status_t ret; + blk_pm_add_request(q, rq); + new_cookie = request_to_qc_t(hctx, rq); /*
Make sure that blk_pm_add_request() is called exactly once before a request is added to a software queue, to the scheduler and also before .queue_rq() is called directly. Call blk_pm_put_request() after a request has finished. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Jianchao Wang <jianchao.w.wang@oracle.com> Cc: Ming Lei <ming.lei@redhat.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Johannes Thumshirn <jthumshirn@suse.de> --- block/blk-mq-sched.c | 13 +++++++++++-- block/blk-mq.c | 8 ++++++++ 2 files changed, 19 insertions(+), 2 deletions(-)