| Message ID | 20171016233226.5488-1-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 16, 2017 at 04:32:26PM -0700, Bart Van Assche wrote: > blk_mq_get_tag() can modify data->ctx. This means that in the > error path of blk_mq_get_request() data->ctx should be passed to > blk_mq_put_ctx() instead of local_ctx. Note: since blk_mq_put_ctx() > ignores its argument, this patch does not change any functionality. > > References: commit 1ad43c0078b7 ("blk-mq: don't leak preempt counter/q_usage_counter when allocating rq failed") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Ming Lei <ming.lei@redhat.com> > --- > Changes compared to v1: > - Removed "Cc: <stable>" > - Removed "bug fix" from the description. > > block/blk-mq.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 097ca3ece716..d9b3b5dc66ab 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -336,12 +336,14 @@ static struct request *blk_mq_get_request(struct request_queue *q, > struct elevator_queue *e = q->elevator; > struct request *rq; > unsigned int tag; > - struct blk_mq_ctx *local_ctx = NULL; > + bool put_ctx_on_error = false; > > blk_queue_enter_live(q); > data->q = q; > - if (likely(!data->ctx)) > - data->ctx = local_ctx = blk_mq_get_ctx(q); > + if (likely(!data->ctx)) { > + data->ctx = blk_mq_get_ctx(q); > + put_ctx_on_error = true; > + } > if (likely(!data->hctx)) > data->hctx = blk_mq_map_queue(q, data->ctx->cpu); > if (op & REQ_NOWAIT) > @@ -360,8 +362,8 @@ static struct request *blk_mq_get_request(struct request_queue *q, > > tag = blk_mq_get_tag(data); > if (tag == BLK_MQ_TAG_FAIL) { > - if (local_ctx) { > - blk_mq_put_ctx(local_ctx); > + if (put_ctx_on_error) { > + blk_mq_put_ctx(data->ctx); > data->ctx = NULL; > } > blk_queue_exit(q); > -- > 2.14.2 > Reviewed-by: Ming Lei <ming.lei@redhat.com>
On 10/17/2017 01:32 AM, Bart Van Assche wrote: > blk_mq_get_tag() can modify data->ctx. This means that in the > error path of blk_mq_get_request() data->ctx should be passed to > blk_mq_put_ctx() instead of local_ctx. Note: since blk_mq_put_ctx() > ignores its argument, this patch does not change any functionality. > > References: commit 1ad43c0078b7 ("blk-mq: don't leak preempt counter/q_usage_counter when allocating rq failed") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Ming Lei <ming.lei@redhat.com> > --- > Changes compared to v1: > - Removed "Cc: <stable>" > - Removed "bug fix" from the description. > > block/blk-mq.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Mon, 2017-10-16 at 16:32 -0700, Bart Van Assche wrote: > blk_mq_get_tag() can modify data->ctx. This means that in the > error path of blk_mq_get_request() data->ctx should be passed to > blk_mq_put_ctx() instead of local_ctx. Note: since blk_mq_put_ctx() > ignores its argument, this patch does not change any functionality. Hello Jens, Would it be possible to share your opinion about this patch? Thanks, Bart.
On 11/03/2017 12:31 PM, Bart Van Assche wrote: > On Mon, 2017-10-16 at 16:32 -0700, Bart Van Assche wrote: >> blk_mq_get_tag() can modify data->ctx. This means that in the >> error path of blk_mq_get_request() data->ctx should be passed to >> blk_mq_put_ctx() instead of local_ctx. Note: since blk_mq_put_ctx() >> ignores its argument, this patch does not change any functionality. > > Hello Jens, > > Would it be possible to share your opinion about this patch? Yeah looks fine, I queued it up.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 097ca3ece716..d9b3b5dc66ab 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -336,12 +336,14 @@ static struct request *blk_mq_get_request(struct request_queue *q, struct elevator_queue *e = q->elevator; struct request *rq; unsigned int tag; - struct blk_mq_ctx *local_ctx = NULL; + bool put_ctx_on_error = false; blk_queue_enter_live(q); data->q = q; - if (likely(!data->ctx)) - data->ctx = local_ctx = blk_mq_get_ctx(q); + if (likely(!data->ctx)) { + data->ctx = blk_mq_get_ctx(q); + put_ctx_on_error = true; + } if (likely(!data->hctx)) data->hctx = blk_mq_map_queue(q, data->ctx->cpu); if (op & REQ_NOWAIT) @@ -360,8 +362,8 @@ static struct request *blk_mq_get_request(struct request_queue *q, tag = blk_mq_get_tag(data); if (tag == BLK_MQ_TAG_FAIL) { - if (local_ctx) { - blk_mq_put_ctx(local_ctx); + if (put_ctx_on_error) { + blk_mq_put_ctx(data->ctx); data->ctx = NULL; } blk_queue_exit(q);
blk_mq_get_tag() can modify data->ctx. This means that in the error path of blk_mq_get_request() data->ctx should be passed to blk_mq_put_ctx() instead of local_ctx. Note: since blk_mq_put_ctx() ignores its argument, this patch does not change any functionality. References: commit 1ad43c0078b7 ("blk-mq: don't leak preempt counter/q_usage_counter when allocating rq failed") Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Ming Lei <ming.lei@redhat.com> --- Changes compared to v1: - Removed "Cc: <stable>" - Removed "bug fix" from the description. block/blk-mq.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)