[PATCH] 2.5.46: duplicate statistics being gathered

[PATCH] 2.5.46: duplicate statistics being gathered

[Rick Lindsley]

In 2.5.46, there are now disk statistics being collected twice: once
for gendisk/hd_struct, and once for dkstat.  They are collecting the
same thing.  This patch removes dkstat, which also had the disadvantage
of being limited by DK_MAX_MAJOR and DK_MAX_DISK. (Those #defines are
removed too.)

In addition, this patch removes disk statistics from /proc/stat since
they are now available via sysfs and there seems to have been a general
preference in previous discussions to "clean up" /proc/stat.  Too many
disks being reported in /proc/stat also caused buffer overflows when
trying to print out the data.

The code in led.c from the parisc architecture has not apparently been
recompiled under recent versions of 2.5, since it references
kstat.dk_drive which doesn't exist in later versions.  Accordingly,
I've added an #ifdef 0 and a comment to that code so that it may at
least compile, albeit without one feature -- a step up from its state
now.  If it is preferable to keep the broken code in, that patch may
easily be excised from below.

Rick

diff -ru stat-2.5.46/drivers/block/ll_rw_blk.c stat-2.5.46-rl1/drivers/block/ll_rw_blk.c
--- stat-2.5.46/drivers/block/ll_rw_blk.c	Mon Nov  4 14:30:06 2002
+++ stat-2.5.46-rl1/drivers/block/ll_rw_blk.c	Thu Nov  7 16:52:12 2002
@@ -28,11 +28,6 @@
 #include <linux/slab.h>
 
 /*
- * Disk stats 
- */
-struct disk_stat dkstat;
-
-/*
  * For the allocated request tables
  */
 static kmem_cache_t *request_cachep;
@@ -1433,19 +1428,6 @@
 
 	major = rq->rq_disk->major;
 	index = rq->rq_disk->first_minor >> rq->rq_disk->minor_shift;
-
-	if ((index >= DK_MAX_DISK) || (major >= DK_MAX_MAJOR))
-		return;
-
-	dkstat.drive[major][index] += new_io;
-	if (rw == READ) {
-		dkstat.drive_rio[major][index] += new_io;
-		dkstat.drive_rblk[major][index] += nr_sectors;
-	} else if (rw == WRITE) {
-		dkstat.drive_wio[major][index] += new_io;
-		dkstat.drive_wblk[major][index] += nr_sectors;
-	} else
-		printk(KERN_ERR "drive_stat_acct: cmd not R/W?\n");
 }
 
 /*
diff -ru stat-2.5.46/drivers/parisc/led.c stat-2.5.46-rl1/drivers/parisc/led.c
--- stat-2.5.46/drivers/parisc/led.c	Mon Nov  4 14:30:17 2002
+++ stat-2.5.46-rl1/drivers/parisc/led.c	Thu Nov  7 17:29:09 2002
@@ -403,6 +403,14 @@
 	int major, disk, total;
 	
 	total = 0;
+#ifdef 0
+	/*
+	 * this section will no longer work in 2.5, as we no longer
+	 * have either kstat.dk_drive nor DK_MAX_*.  It can probably
+	 * be rewritten to use the per-disk statistics now kept in the
+	 * gendisk, but since I have no HP machines to test it on, I'm
+	 * not really up to that.  ricklind@us.ibm.com 11/7/02
+	 */
 	for (major = 0; major < DK_MAX_MAJOR; major++) {
 	    for (disk = 0; disk < DK_MAX_DISK; disk++)
 		total += kstat.dk_drive[major][disk];
@@ -416,6 +424,7 @@
 	    } else
 		led_diskio_counter += CALC_ADD(total, diskio_max, addvalue);
 	}
+#endif
 	
 	diskio_total_last += total; 
 }
diff -ru stat-2.5.46/fs/proc/proc_misc.c stat-2.5.46-rl1/fs/proc/proc_misc.c
--- stat-2.5.46/fs/proc/proc_misc.c	Mon Nov  4 14:30:07 2002
+++ stat-2.5.46-rl1/fs/proc/proc_misc.c	Thu Nov  7 16:52:38 2002
@@ -381,26 +381,6 @@
 		len += sprintf(page + len, " %u", kstat_irqs(i));
 #endif
 
-	len += sprintf(page + len, "\ndisk_io: ");
-
-	for (major = 0; major < DK_MAX_MAJOR; major++) {
-		for (disk = 0; disk < DK_MAX_DISK; disk++) {
-			int active = dkstat.drive[major][disk] +
-				dkstat.drive_rblk[major][disk] +
-				dkstat.drive_wblk[major][disk];
-			if (active)
-				len += sprintf(page + len,
-					"(%u,%u):(%u,%u,%u,%u,%u) ",
-					major, disk,
-					dkstat.drive[major][disk],
-					dkstat.drive_rio[major][disk],
-					dkstat.drive_rblk[major][disk],
-					dkstat.drive_wio[major][disk],
-					dkstat.drive_wblk[major][disk]
-			);
-		}
-	}
-
 	len += sprintf(page + len,
 		"\nctxt %lu\n"
 		"btime %lu\n"
diff -ru stat-2.5.46/include/linux/blkdev.h stat-2.5.46-rl1/include/linux/blkdev.h
--- stat-2.5.46/include/linux/blkdev.h	Mon Nov  4 14:30:14 2002
+++ stat-2.5.46-rl1/include/linux/blkdev.h	Thu Nov  7 16:54:42 2002
@@ -11,22 +11,6 @@
 
 #include <asm/scatterlist.h>
 
-/*
- * Disk stats ...
- */
-
-#define DK_MAX_MAJOR 16
-#define DK_MAX_DISK 16
- 
-struct disk_stat {
-        unsigned int drive[DK_MAX_MAJOR][DK_MAX_DISK];
-        unsigned int drive_rio[DK_MAX_MAJOR][DK_MAX_DISK];
-        unsigned int drive_wio[DK_MAX_MAJOR][DK_MAX_DISK];
-        unsigned int drive_rblk[DK_MAX_MAJOR][DK_MAX_DISK];
-        unsigned int drive_wblk[DK_MAX_MAJOR][DK_MAX_DISK];
-};
-extern struct disk_stat dkstat;
-
 struct request_queue;
 typedef struct request_queue request_queue_t;
 struct elevator_s;

Re: [PATCH] 2.5.46: duplicate statistics being gathered

[Andrew Morton]

Rick Lindsley wrote:
> 
> In addition, this patch removes disk statistics from /proc/stat since
> they are now available via sysfs and there seems to have been a general
> preference in previous discussions to "clean up" /proc/stat.

We really should do this.  It's a question of timing and phasing.

The <unknown quantity of> applications out there which are reading
the disk info from /proc/stat need to be taught to go fishing in
/name-of-the-day-fs.

And that's hard.  /driverfs? /sys? /sysfs? /kernfs?  AFAIK we don't
even have a recommended mountpoint for the thing, do we?  One way
to resolve that is for the monitoring application to locate the
mountpoint by consulting /proc/mounts on startup.

So what do people think?  Do we just peremptorily nuke it and let
the world catch up, or is a more organised migration possible?

Rick, I believe you've hunted down some other apps which are using this
info. sysstat, sar, mpstat, various flavours of iostat.  Where do
we stand on getting those updated?

Thanks.

Re: [PATCH] 2.5.46: duplicate statistics being gathered

[Greg KH]

On Thu, Nov 07, 2002 at 09:27:08PM -0800, Andrew Morton wrote:
> 
> The <unknown quantity of> applications out there which are reading
> the disk info from /proc/stat need to be taught to go fishing in
> /name-of-the-day-fs.
> 
> And that's hard.  /driverfs? /sys? /sysfs? /kernfs?  AFAIK we don't
> even have a recommended mountpoint for the thing, do we?  One way
> to resolve that is for the monitoring application to locate the
> mountpoint by consulting /proc/mounts on startup.

It's now called sysfs and should be mounted at /sys.  I do not think the
name will change anymore (famous last words...)

Unless you want to be different and mount it somewhere else :)

And yes, any application relying on data within it, should be able to
determine its location (through /proc/mounts, or some other such
format.)

thanks,

greg k-h

Re: [PATCH] 2.5.46: duplicate statistics being gathered

[Rick Lindsley]

    The <unknown quantity of> applications out there which are reading
    the disk info from /proc/stat need to be taught to go fishing in
    /name-of-the-day-fs.

As we determine the quantity, I'm (so far) willing to assist in porting.

    So what do people think?  Do we just peremptorily nuke it and let
    the world catch up, or is a more organised migration possible?

We should set the new world order in place before 2.6 emerges.  That
allows distros to collect or create tools for their stable releases
which take advantage of this.

    Rick, I believe you've hunted down some other apps which are
    using this info. sysstat, sar, mpstat, various flavours of iostat.
    Where do we stand on getting those updated?

iostat.c from linux.inet.hr is almost completely ported now.  It will
search around in /proc/partitions, /proc/diskstat, and <sysfs>/block
to try to find the location of the statistics, meaning it should work
on 2.4 + patches, 2.5 + patches, now 2.5.46, and eventually 2.4, 2.5,
and 2.6 without patches.  IT'S NOT THERE YET -- still testing and hope
to pass it on to the site owner this weekend.

iostat.c and sar from the sysstat set of tools at
http://perso.wanadoo.fr/sebastien.godard/ has been gathered, perused,
but not yet ported.  The iostat.c source is similar but different from
the source mentioned above.  The sysstat tools will be next, and the
changes again passed back to the site maintainer.

If there are other tools that utilize disk statistics, now would be a
very good time to let me know about them.  I feel strongly enough about
getting this right to help port them as needed .. for a while anyway :)

In the kernel, there are some designated maintainers for many areas.
Is it similar in user space?

Rick

Re: [PATCH] 2.5.46: duplicate statistics being gathered

[Richard B. Johnson]

On Fri, 8 Nov 2002, Rick Lindsley wrote:

>     The <unknown quantity of> applications out there which are reading
>     the disk info from /proc/stat need to be taught to go fishing in
>     /name-of-the-day-fs.
> 

There is /proc/version, which could have been used to make
`ps` and `top` compatable over, at least previously known
versions.

The last version of procps I have a copy of is 1.2. I don't see
anybody opening that file anywhere. Maybe it would be a good
idea to start.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
   Bush : The Fourth Reich of America

Re: [PATCH] 2.5.46: duplicate statistics being gathered

[Zwane Mwaikambo]

On Fri, 8 Nov 2002, Richard B. Johnson wrote:

> On Fri, 8 Nov 2002, Rick Lindsley wrote:
> 
> >     The <unknown quantity of> applications out there which are reading
> >     the disk info from /proc/stat need to be taught to go fishing in
> >     /name-of-the-day-fs.
> > 
> 
> There is /proc/version, which could have been used to make
> `ps` and `top` compatable over, at least previously known
> versions.
> 
> The last version of procps I have a copy of is 1.2. I don't see
> anybody opening that file anywhere. Maybe it would be a good
> idea to start.

...
munmap(0x40013000, 113429)              = 0
uname({sys="Linux", node="montezuma.mastecende.com", ...}) = 0
open("/proc/uptime", O_RDONLY)          = 3
lseek(3, 0, SEEK_SET)                   = 0
..

? Or is this not what you had in mind.

	Zwane
-- 
function.linuxpower.ca