Re: Patch for inconsistent recording of block device statistics

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.
Actually I wasn't all that excited about using the extra variable myself. However, I wasn't entirely sure what was going on and this at least allowed me to test the concept without doing anything harmful.
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.
I also agree with your suggestion about keeping the merged counts where they are and am copying the author of iostat to suggest the man page be updated to reflect the fact that merges are counts for requests queued rather than 'issued to the device' as it currently states.

re: your patch - I did try it on both an Operton and Xeon box. It worked find on the Opeteron and reported 0 for all the sectors on the Xeon. If nothing immediately jumps to your mind could it have been something I did wrong? I'll try another build after I send this along, but I don't see how that will help as I did the first one from a brand new source kit.

The one thing that still jumps out at me about this patch is that the sectors are being counted in one routine and the number of I/Os in another. If the best place to update the sector counts is indeed where you suggest doing it, is there any reason not to move the update code for all the disk stats from end_that_request_last() to that same place as well for consistency and for better assurances that they are updated as close to the same point in time as possible?

-mark

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux