[PATCH] block: move iostat check into blk_acount_io_start()

[PATCH] block: move iostat check into blk_acount_io_start()

[Jens Axboe]

Rather than have blk_do_io_stat() check for both RQF_IO_STAT and whether
the request is a passthrough requests every time, move both of those
checks into blk_account_io_start(). Then blk_do_io_stat() can be reduced
to just checking for RQF_IO_STAT.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4b2c8e940f59..ee6cde39e52b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -359,8 +359,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 
 	if (data->flags & BLK_MQ_REQ_PM)
 		data->rq_flags |= RQF_PM;
-	if (blk_queue_io_stat(q))
-		data->rq_flags |= RQF_IO_STAT;
 	rq->rq_flags = data->rq_flags;
 
 	if (data->rq_flags & RQF_SCHED_TAGS) {
@@ -1000,24 +998,28 @@ static inline void blk_account_io_start(struct request *req)
 {
 	trace_block_io_start(req);
 
-	if (blk_do_io_stat(req)) {
-		/*
-		 * All non-passthrough requests are created from a bio with one
-		 * exception: when a flush command that is part of a flush sequence
-		 * generated by the state machine in blk-flush.c is cloned onto the
-		 * lower device by dm-multipath we can get here without a bio.
-		 */
-		if (req->bio)
-			req->part = req->bio->bi_bdev;
-		else
-			req->part = req->q->disk->part0;
+	if (!blk_queue_io_stat(req->q))
+		return;
+	if (blk_rq_is_passthrough(req))
+		return;
 
-		part_stat_lock();
-		update_io_ticks(req->part, jiffies, false);
-		part_stat_local_inc(req->part,
-				    in_flight[op_is_write(req_op(req))]);
-		part_stat_unlock();
-	}
+	req->rq_flags |= RQF_IO_STAT;
+
+	/*
+	 * All non-passthrough requests are created from a bio with one
+	 * exception: when a flush command that is part of a flush sequence
+	 * generated by the state machine in blk-flush.c is cloned onto the
+	 * lower device by dm-multipath we can get here without a bio.
+	 */
+	if (req->bio)
+		req->part = req->bio->bi_bdev;
+	else
+		req->part = req->q->disk->part0;
+
+	part_stat_lock();
+	update_io_ticks(req->part, jiffies, false);
+	part_stat_local_inc(req->part, in_flight[op_is_write(req_op(req))]);
+	part_stat_unlock();
 }
 
 static inline void __blk_mq_end_request_acct(struct request *rq, u64 now)
diff --git a/block/blk.h b/block/blk.h
index c718e4291db0..84178e535533 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -413,7 +413,7 @@ int blk_dev_init(void);
  */
 static inline bool blk_do_io_stat(struct request *rq)
 {
-	return (rq->rq_flags & RQF_IO_STAT) && !blk_rq_is_passthrough(rq);
+	return rq->rq_flags & RQF_IO_STAT;
 }
 
 void update_io_ticks(struct block_device *part, unsigned long now, bool end);

-- 
Jens Axboe

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[Keith Busch]

On Wed, Oct 02, 2024 at 01:26:08PM -0600, Jens Axboe wrote:
> Rather than have blk_do_io_stat() check for both RQF_IO_STAT and whether
> the request is a passthrough requests every time, move both of those
> checks into blk_account_io_start(). Then blk_do_io_stat() can be reduced
> to just checking for RQF_IO_STAT.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[Anuj gupta]

Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[Christoph Hellwig]

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

I'd kill this somewhat oddly named wrapper now that it is a single
check.

Otherwise looks good:

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

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[Jens Axboe]

On 10/3/24 7:09 AM, Christoph Hellwig wrote:
>>  static inline bool blk_do_io_stat(struct request *rq)
>>  {
>> +	return rq->rq_flags & RQF_IO_STAT;
>>  }
> 
> I'd kill this somewhat oddly named wrapper now that it is a single
> check.

Yeah I did ponder that, since there's just that one check left. Will
do a followup for that.

-- 
Jens Axboe

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[kernel test robot]

Hello,

kernel test robot noticed "blktests.block/024.fail" on:

commit: 5b0b8be85f1ca1c11566890f5d4564ee97cf2d41 ("[PATCH] block: move iostat check into blk_acount_io_start()")
url: https://github.com/intel-lab-lkp/linux/commits/Jens-Axboe/block-move-iostat-check-into-blk_acount_io_start/20241003-032648
base: https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/all/550fc8a0-3461-49ac-879e-32908870f007@kernel.dk/
patch subject: [PATCH] block: move iostat check into blk_acount_io_start()

in testcase: blktests
version: blktests-x86_64-80430af-1_20240910
with following parameters:

	disk: 1HDD
	test: block-024



compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410062110.512391df-oliver.sang@intel.com

2024-10-06 11:02:24 echo block/024
2024-10-06 11:02:24 ./check block/024
block/024 (do I/O faster than a jiffy and check iostats times)
block/024 (do I/O faster than a jiffy and check iostats times) [failed]
    runtime    ...  2.673s
    --- tests/block/024.out	2024-09-10 23:15:59.000000000 +0000
    +++ /lkp/benchmarks/blktests/results/nodev/block/024.out.bad	2024-10-06 11:02:27.595709543 +0000
    @@ -1,10 +1,10 @@
     Running block/024
     read 0 s
     write 0 s
    -read 1 s
    +read 70000 s
     write 0 s
    -read 1 s
    ...
    (Run 'diff -u tests/block/024.out /lkp/benchmarks/blktests/results/nodev/block/024.out.bad' to see the entire diff)



The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241006/202410062110.512391df-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] block: move iostat check into blk_acount_io_start()

[Jens Axboe]

On 10/6/24 8:07 AM, kernel test robot wrote:
> 
> 
> Hello,
> 
> kernel test robot noticed "blktests.block/024.fail" on:
> 
> commit: 5b0b8be85f1ca1c11566890f5d4564ee97cf2d41 ("[PATCH] block: move iostat check into blk_acount_io_start()")
> url: https://github.com/intel-lab-lkp/linux/commits/Jens-Axboe/block-move-iostat-check-into-blk_acount_io_start/20241003-032648
> base: https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git for-next
> patch link: https://lore.kernel.org/all/550fc8a0-3461-49ac-879e-32908870f007@kernel.dk/
> patch subject: [PATCH] block: move iostat check into blk_acount_io_start()
> 
> in testcase: blktests
> version: blktests-x86_64-80430af-1_20240910
> with following parameters:
> 
> 	disk: 1HDD
> 	test: block-024

This should fix it:

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8e75e3471ea5..9cbc0262ad19 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -331,7 +331,7 @@ EXPORT_SYMBOL(blk_rq_init);
 /* Set start and alloc time when the allocated request is actually used */
 static inline void blk_mq_rq_time_init(struct request *rq, u64 alloc_time_ns)
 {
-	if (blk_mq_need_time_stamp(rq))
+	if (blk_queue_io_stat(rq->q))
 		rq->start_time_ns = blk_time_get_ns();
 	else
 		rq->start_time_ns = 0;

-- 
Jens Axboe