Issue in blk_mq_alloc_request_hctx()

Commit Message

John Garry Oct. 21, 2022, 11:16 a.m. UTC

Hi guys,

I find that a call to blk_mq_alloc_request_hctx() sometimes has rq->bio 
set when returned, when I didn't think it should. I notice that we 
explicitly zero this field (and data len and sector) in 
blk_mq_alloc_request().

This trips up scsi_setup_scsi_cmd() for me, which checks these fields.

This is what I thought needs changing:

---8<----

 From 396d0b64b73cc6fb5193189d9da2c6107dbeff83 Mon Sep 17 00:00:00 2001
From: John Garry <[e-mail]>
Date: Fri, 21 Oct 2022 11:56:11 +0100
Subject: [PATCH] blk-mq: Properly init rq and fix queue enter/exit in
  blk_mq_alloc_request_hctx()


  	int ret;
@@ -660,8 +661,15 @@ struct request *blk_mq_alloc_request_hctx(struct 
request_queue *q,
  	tag = blk_mq_get_tag(&data);
  	if (tag == BLK_MQ_NO_TAG)
  		goto out_queue_exit;
-	return blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
+	rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
  					alloc_time_ns);
+	if (!rq)
+		goto out_queue_exit;
+
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	return rq;


--->8---


What that, now my code under dev looks ok. Is that change correct? Seems 
strange to be missed..

Cheers,
John

Comments

Bart Van Assche Oct. 21, 2022, 2:54 p.m. UTC | #1

On 10/21/22 04:16, John Garry wrote:
> -    return blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
> +    rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
>                       alloc_time_ns);
> +    if (!rq)
> +        goto out_queue_exit;
> +
> +    rq->__data_len = 0;
> +    rq->__sector = (sector_t) -1;
> +    rq->bio = rq->biotail = NULL;
> +    return rq;

Hi John,

Shouldn't the new struct request member initializations be moved into 
blk_mq_rq_ctx_init() such that all blk_mq_rq_ctx_init() callers are fixed?

Thanks,

Bart.

John Garry Oct. 21, 2022, 3:26 p.m. UTC | #2

On 21/10/2022 15:54, Bart Van Assche wrote:
> On 10/21/22 04:16, John Garry wrote:
>> -    return blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
>> +    rq = blk_mq_rq_ctx_init(&data, blk_mq_tags_from_data(&data), tag,
>>                       alloc_time_ns);
>> +    if (!rq)
>> +        goto out_queue_exit;
>> +
>> +    rq->__data_len = 0;
>> +    rq->__sector = (sector_t) -1;
>> +    rq->bio = rq->biotail = NULL;
>> +    return rq;
> 
> Hi John,
> 
> Shouldn't the new struct request member initializations be moved into 
> blk_mq_rq_ctx_init() such that all blk_mq_rq_ctx_init() callers are fixed?

That would seem reasonable. I just wonder why it was not there in the 
first place.

Anyway, I'll look to make that change.

thanks,
John

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f33ea455dd72..cce5cda3c442 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -611,6 +611,7 @@  struct request *blk_mq_alloc_request_hctx(struct 
request_queue *q,
  		.nr_tags	= 1,
  	};
  	u64 alloc_time_ns = 0;
+	struct request *rq;
  	unsigned int cpu;
  	unsigned int tag;