diff mbox series

[v9,10/12] block: don't check blk_rq_is_passthrough() in blk_do_io_stat()

Message ID 20191009192530.13079-12-logang@deltatee.com (mailing list archive)
State New, archived
Headers show
Series nvmet: add target passthru commands support | expand

Commit Message

Logan Gunthorpe Oct. 9, 2019, 7:25 p.m. UTC 
Instead of checking blk_rq_is_passthruough() for every call to
blk_do_io_stat(), don't set RQF_IO_STAT for passthrough requests.
This should be equivalent, and opens the possibility of passthrough
requests specifically requesting statistics tracking.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-mq.c | 2 +-
 block/blk.h    | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Oct. 10, 2019, 10:05 a.m. UTC | #1 
> @@ -319,7 +319,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>  	rq->cmd_flags = op;
>  	if (data->flags & BLK_MQ_REQ_PREEMPT)
>  		rq->rq_flags |= RQF_PREEMPT;
> -	if (blk_queue_io_stat(data->q))
> +	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
>  		rq->rq_flags |= RQF_IO_STAT;

This needs a comment why we don't account passthrough requests by
default.  And I'm really curious about the answer, because I don't
know it myself.

>   *	a) it's attached to a gendisk, and
>   *	b) the queue had IO stats enabled when this request was started, and
> - *	c) it's a file system request
> + *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)

c) should just go away now based on your changes.

>  static inline bool blk_do_io_stat(struct request *rq)
>  {
>  	return rq->rq_disk &&
> -	       (rq->rq_flags & RQF_IO_STAT) &&
> -		!blk_rq_is_passthrough(rq);
> +	       (rq->rq_flags & RQF_IO_STAT);

The check can be collapsed onto a single line now.
Logan Gunthorpe Oct. 10, 2019, 5:56 p.m. UTC | #2 
Hey,

Thanks for the thorough review, lots here to go through. I'll address it
all as I have time and try to get the prep work done first, as soon as I
can.

On 2019-10-10 4:05 a.m., Christoph Hellwig wrote:
>> @@ -319,7 +319,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>>  	rq->cmd_flags = op;
>>  	if (data->flags & BLK_MQ_REQ_PREEMPT)
>>  		rq->rq_flags |= RQF_PREEMPT;
>> -	if (blk_queue_io_stat(data->q))
>> +	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
>>  		rq->rq_flags |= RQF_IO_STAT;
> 
> This needs a comment why we don't account passthrough requests by
> default.  And I'm really curious about the answer, because I don't
> know it myself.

Yes, sadly, I don't know this answer either but the comment made it
appear that someone did it deliberately. Digging into git blame suggests
that it just evolved that way. The check was originally added in 2005
here with blk_fs_request():

commit d72d904a5367 ("[BLOCK] Update read/write block io statistics at
completion time")

blk_fs_request() evolved to become blk_rq_is_passthrough() but I suspect
no one ever considered whether we want to account the passthru requests.

So, I'll leave this restriction out and see if anyone complains.

Logan

>>   *	a) it's attached to a gendisk, and
>>   *	b) the queue had IO stats enabled when this request was started, and
>> - *	c) it's a file system request
>> + *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)
> 
> c) should just go away now based on your changes.
> 
>>  static inline bool blk_do_io_stat(struct request *rq)
>>  {
>>  	return rq->rq_disk &&
>> -	       (rq->rq_flags & RQF_IO_STAT) &&
>> -		!blk_rq_is_passthrough(rq);
>> +	       (rq->rq_flags & RQF_IO_STAT);
> 
> The check can be collapsed onto a single line now.
>
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ec791156e9cc..618814fcc8a7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -319,7 +319,7 @@  static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->cmd_flags = op;
 	if (data->flags & BLK_MQ_REQ_PREEMPT)
 		rq->rq_flags |= RQF_PREEMPT;
-	if (blk_queue_io_stat(data->q))
+	if (blk_queue_io_stat(data->q) && !blk_rq_is_passthrough(rq))
 		rq->rq_flags |= RQF_IO_STAT;
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_HLIST_NODE(&rq->hash);
diff --git a/block/blk.h b/block/blk.h
index 47fba9362e60..6b7ebc2e61b8 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -243,13 +243,12 @@  int blk_dev_init(void);
  *
  *	a) it's attached to a gendisk, and
  *	b) the queue had IO stats enabled when this request was started, and
- *	c) it's a file system request
+ *	c) it's a file system request (RQF_IO_STAT will not be set otherwise)
  */
 static inline bool blk_do_io_stat(struct request *rq)
 {
 	return rq->rq_disk &&
-	       (rq->rq_flags & RQF_IO_STAT) &&
-		!blk_rq_is_passthrough(rq);
+	       (rq->rq_flags & RQF_IO_STAT);
 }
 
 static inline void req_set_nomerge(struct request_queue *q, struct request *req)