Re: Patch for inconsistent recording of block device statistics

[Jens Axboe <axboe@suse.de>]

On Tue, Mar 22 2005, Mark Seger wrote:
> The read/write statistics for both sectors and merges are calculated at 
> the time requests first enter the request queue but the remainder of the 
> statistics, such as the number of read/writes are calculated at the time 
> the I/O completes.  As a result, one cannot accurately determine the 
> data rates read or written at the actual time the I/O is performed.  
> This behavior is masked with smaller queue sizes but is very real and 
> was very noticeable with earlier 2.6 kenels using the cfq scheduler 
> which had a default queue size of 8192 where the time difference between 
> these sets of counters could exceed 10 seconds for large file writes and 
> small monitoring intervals such as 1 second.  In that environment, one 
> would see extremely high bursts of I/O, sometimes exceeding 500 or even 
> 1000 MB/sec for the first second or two and then drop to 0 for a long 
> time while the 'number of operations' counters accurately reflect what 
> is really happening.
> 
> The attached patch fixes this problem by simply accumulating the 
> read/write sector/merge data in temporary variables stored in the 
> request queue entry, and when the I/O completes copies those values to 
> the disk statistics block.

I don't like this patch, it adds 4 * sizeof(unsigned long) to struct
request when it can be solved without adding anything. The idea is
sound, though, the current way the stats are done isn't very
interesting.

How about accounting merges the way we currently do it, since that piece
of the stats _is_ interesting at queueing time. And then account
completion in __end_that_request_first(). Untested patch attached.

===== drivers/block/ll_rw_blk.c 1.287 vs edited =====
--- 1.287/drivers/block/ll_rw_blk.c	2005-03-11 21:32:27 +01:00
+++ edited/drivers/block/ll_rw_blk.c	2005-03-23 10:10:39 +01:00
@@ -2294,16 +2293,12 @@
 	if (!blk_fs_request(rq) || !rq->rq_disk)
 		return;
 
-	if (rw == READ) {
-		__disk_stat_add(rq->rq_disk, read_sectors, nr_sectors);
-		if (!new_io)
+	if (!new_io) {
+		if (rw == READ)
 			__disk_stat_inc(rq->rq_disk, read_merges);
-	} else if (rw == WRITE) {
-		__disk_stat_add(rq->rq_disk, write_sectors, nr_sectors);
-		if (!new_io)
+		else
 			__disk_stat_inc(rq->rq_disk, write_merges);
-	}
-	if (new_io) {
+	} else {
 		disk_round_stats(rq->rq_disk);
 		rq->rq_disk->in_flight++;
 	}
@@ -3063,6 +3069,13 @@
 				(unsigned long long)req->sector);
 	}
 
+	if (blk_fs_request(req)) {
+		if (rq_data_dir(req) == READ)
+			__disk_stat_add(req->rq_disk, read_sectors, nr_bytes >> 9);
+		else
+			__disk_stat_add(req->rq_disk, write_sectors, nr_bytes >> 9);
+	}
+
 	total_bytes = bio_nbytes = 0;
 	while ((bio = req->bio) != NULL) {
 		int nbytes;

-- 
Jens Axboe