[Fwd: Re: Patch for inconsistent recording of block device statistics]
PATCH 1/1 [Fwd: Re: Patch for inconsistent recording of block device statistics]
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
Quoted 15 lines · Mark Seger
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.
Quoted 4 lines
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 =====
drivers/block/ll_rw_blk.c +11 -8
@@ -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;
Signature · 1 line
Jens Axboe
Quoted 5 lines · Mark Seger
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?
Signature · 1 line
Jens Axboe
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
Quoted 13 lines · Jens Axboe
Quoted 3 lines · Mark Seger
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?
Quoted 8 lines · 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.
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!
Signature · 1 line
Jens Axboe