[Fwd: Re: Patch for inconsistent recording of block device statistics]

[Fwd: Re: Patch for inconsistent recording of block device statistics]

[Mark Seger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

This patch was discussed back in march, and I still haven't seen it show 
up in the source pool.  I was wondering if it just feel through the 
cracks or if it was planned for a specific future release.  If the 
attached doesn't provide enough context for you to remember what this is 
all about, just let me know...
-mark


[-- Attachment #2: Re: Patch for inconsistent recording of block device statistics --]
[-- Type: message/rfc822, Size: 4998 bytes --]

From: Jens Axboe <axboe@suse.de>
To: Mark Seger <Mark.Seger@hp.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: Patch for inconsistent recording of block device statistics
Date: Wed, 23 Mar 2005 10:19:16 +0100
Message-ID: <20050323091916.GO24105@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

Re: [Fwd: Re: Patch for inconsistent recording of block device statistics]

[Jens Axboe]

On Mon, Oct 24 2005, Mark Seger wrote:
> This patch was discussed back in march, and I still haven't seen it show 
> up in the source pool.  I was wondering if it just feel through the 
> cracks or if it was planned for a specific future release.  If the 
> attached doesn't provide enough context for you to remember what this is 
> all about, just let me know...

Refresh my memory on where the discussion went after this email, I don't
recall. Did the patch work for you?


-- 
Jens Axboe

Re: [Fwd: Re: Patch for inconsistent recording of block device statistics]

[Mark Seger]

yes, the patch worked.  The general discussion was that the byte counter 
gets incremented when requests are queued, not when they're acted upon 
as is the case with the count of I/Os.  As a result, the disk write 
numbers don't make any sense reporting impossibly high numbers (>100MB 
and as high as 450!) during some times and at other reporting zeros.  
The entire time, the I/O counts are happily showing what appear to be 
correct numbers.  Here's a snapshot taken during a portion of a 2GB file 
file to /tmp.

# DISK SUMMARY (/sec)
#         Reads  R-Merged  R-KBytes   Writes  W-Merged  W-KBytes
14:26:38      0         0         0        0         0         0
14:26:39      0         0         0       90      4391     18368
14:26:40      0         0         0      577     12603     52696
14:26:41      0         0         0      563    107835    446728
14:26:42      0         0         0      445         0         0
14:26:43      0         0         0      442         0         0
14:26:44      0         0         0      445         0         0
14:26:45      0         0         0      354         0         0
14:26:46      0         0         0      442         0         0
14:26:47      0         0         0      443         0         0
14:26:48      0         0         0      408         0         0
14:26:49      0         0         4      439       782      3280
14:26:50      1         0         0      462     12230     51160
14:26:51      0         0         0      574     88342    366116
14:26:52      0         0         0      477     32881    136604
14:26:53      0         0         0      443      9101     37656
14:26:54      0         0         0      442     11779     48736
14:26:55      0         0         0      373         0         0
14:26:56      0         0         0      415         0         0

-mark

Jens Axboe wrote:

>On Mon, Oct 24 2005, Mark Seger wrote:
>  
>
>>This patch was discussed back in march, and I still haven't seen it show 
>>up in the source pool.  I was wondering if it just feel through the 
>>cracks or if it was planned for a specific future release.  If the 
>>attached doesn't provide enough context for you to remember what this is 
>>all about, just let me know...
>>    
>>
>
>Refresh my memory on where the discussion went after this email, I don't
>recall. Did the patch work for you?
>
>
>  
>

Re: [Fwd: Re: Patch for inconsistent recording of block device statistics]

[Jens Axboe]

On Tue, Oct 25 2005, Mark Seger wrote:
> yes, the patch worked.  The general discussion was that the byte counter 
> gets incremented when requests are queued, not when they're acted upon 
> as is the case with the count of I/Os.  As a result, the disk write 
> numbers don't make any sense reporting impossibly high numbers (>100MB 
> and as high as 450!) during some times and at other reporting zeros.  
> The entire time, the I/O counts are happily showing what appear to be 
> correct numbers.  Here's a snapshot taken during a portion of a 2GB file 
> file to /tmp.

I've applied my path to the for-linus git branch, I will push it for
2.6.15 as well. Thanks for reminding me!

-- 
Jens Axboe