[PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Bart Van Assche]

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

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Ming Lei]

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>


-- 
Ming

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Bart Van Assche]

T24gTW9uLCAyMDE3LTEwLTE2IGF0IDE2OjMyIC0wNzAwLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6
DQo+IGJsa19tcV9nZXRfdGFnKCkgY2FuIG1vZGlmeSBkYXRhLT5jdHguIFRoaXMgbWVhbnMgdGhh
dCBpbiB0aGUNCj4gZXJyb3IgcGF0aCBvZiBibGtfbXFfZ2V0X3JlcXVlc3QoKSBkYXRhLT5jdHgg
c2hvdWxkIGJlIHBhc3NlZCB0bw0KPiBibGtfbXFfcHV0X2N0eCgpIGluc3RlYWQgb2YgbG9jYWxf
Y3R4LiBOb3RlOiBzaW5jZSBibGtfbXFfcHV0X2N0eCgpDQo+IGlnbm9yZXMgaXRzIGFyZ3VtZW50
LCB0aGlzIHBhdGNoIGRvZXMgbm90IGNoYW5nZSBhbnkgZnVuY3Rpb25hbGl0eS4NCg0KSGVsbG8g
SmVucywNCg0KV291bGQgaXQgYmUgcG9zc2libGUgdG8gc2hhcmUgeW91ciBvcGluaW9uIGFib3V0
IHRoaXMgcGF0Y2g/DQoNClRoYW5rcywNCg0KQmFydC4=

Re: [PATCH v2] blk-mq: Make blk_mq_get_request() error path less confusing

[Jens Axboe]

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.

-- 
Jens Axboe