[PATCH v2] block: make the io_ticks counter more accurate

[PATCH v2] block: make the io_ticks counter more accurate

[Wen Yang]

Instead of the jiffies, we should update the io_ticks counter
with the passed in parameter 'now'.

Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
v2->v1: Use the same clock source for io_ticks and other statistics in the diskstats

 block/blk-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 379f6f5..da7de9f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1365,7 +1365,7 @@ void blk_account_io_done(struct request *req, u64 now)
 		part_stat_lock();
 		part = req->part;
 
-		update_io_ticks(part, jiffies);
+		update_io_ticks(part, nsecs_to_jiffies(now));
 		part_stat_inc(part, ios[sgrp]);
 		part_stat_add(part, nsecs[sgrp], now - req->start_time_ns);
 		part_stat_add(part, time_in_queue, nsecs_to_jiffies64(now - req->start_time_ns));
@@ -1407,7 +1407,7 @@ void blk_account_io_start(struct request *rq, bool new_io)
 		rq->part = part;
 	}
 
-	update_io_ticks(part, jiffies);
+	update_io_ticks(part, nsecs_to_jiffies(ktime_get_ns()));
 
 	part_stat_unlock();
 }
-- 
1.8.3.1

Re: [PATCH v2] block: make the io_ticks counter more accurate

[Jens Axboe]

On 12/25/19 8:10 PM, Wen Yang wrote:
> Instead of the jiffies, we should update the io_ticks counter
> with the passed in parameter 'now'.

I'm still missing some justification for this. What exactly is this
patch trying to solve or improve? Your commit message says "we should",
but why?

-- 
Jens Axboe

Re: [PATCH v2] block: make the io_ticks counter more accurate

[Wen Yang]

On 2019/12/26 11:39 上午, Jens Axboe wrote:
> On 12/25/19 8:10 PM, Wen Yang wrote:
>> Instead of the jiffies, we should update the io_ticks counter
>> with the passed in parameter 'now'.
> 
> I'm still missing some justification for this. What exactly is this
> patch trying to solve or improve? Your commit message says "we should",
> but why?
> 

Hi Jens,

Thank you for your comments.
We observed in the document that:

io_ticks
========

This value counts the number of milliseconds during which the device has
had I/O requests queued.

And the iostat command uses io_ticks count to calculate %util:
https://github.com/sysstat/sysstat/blob/master/rd_stats.c#L372

eg：
Device:         rrqm/s   wrqm/s     r/s     w/s    rkB/s    wkB/s 
avgrq-sz avgqu-sz   await r_await w_await  svctm  %util


So we need to unify the time windows of these statistics（io_ticks, 
rd_tick, time_in_queue, etc）.
However, the current code uses jiffies to count io_ticks.
Jiffies is different from the passed in parameter 'now',
so these statistics will be inconsistent：

void blk_account_io_done(struct request *req, u64 now)
{
…
     update_io_ticks(part, jiffies);
     part_stat_inc(part, ios[sgrp]);
     part_stat_add(part, nsecs[sgrp], now - req->start_time_ns);
     part_stat_add(part, time_in_queue, nsecs_to_jiffies64(now - 
req->start_time_ns));
…
}

In addition, we also found another issue:
the update_io_tick() function only adds one to io_ticks at a time,
which will result in the calculated %util lower than the real one.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bio.c#n1713


We will try our best to improve it.
please kindly help with some suggestions.
Thanks.

-- 
Best Regards,
Wen