[PATCH] 2.5.46: overflow in disk stats

[PATCH] 2.5.46: overflow in disk stats

[Lev Makhlis]

Hi,

I see that the SARD changes have been merged, but MSEC() still has
the overflow problem.  This takes care of it:

--------------------------------------------------------------------------------------------------

diff -urN linux-2.5.46.orig/drivers/block/genhd.c 
linux-2.5.46/drivers/block/genhd.c
--- linux-2.5.46.orig/drivers/block/genhd.c	Tue Nov  5 15:15:07 2002
+++ linux-2.5.46/drivers/block/genhd.c	Tue Nov  5 16:14:35 2002
@@ -326,7 +326,13 @@
 }
 static inline unsigned MSEC(unsigned x)
 {
-	return x * 1000 / HZ;
+#if 1000 % HZ == 0
+	return x * (1000 / HZ);
+#elif HZ % 1000 == 0
+	return x / (HZ / 1000);
+#else
+	return (x / HZ) * 1000 + (x % HZ) * 1000 / HZ;
+#endif
 }
 static ssize_t disk_stat_read(struct gendisk * disk,
 			      char *page, size_t count, loff_t off)

Re: [PATCH] 2.5.46: overflow in disk stats

[Andrew Morton]

Lev Makhlis wrote:
> 
> Hi,
> 
> I see that the SARD changes have been merged, but MSEC() still has
> the overflow problem.  This takes care of it:
> 
> --------------------------------------------------------------------------------------------------
> 
> diff -urN linux-2.5.46.orig/drivers/block/genhd.c
> linux-2.5.46/drivers/block/genhd.c
> --- linux-2.5.46.orig/drivers/block/genhd.c     Tue Nov  5 15:15:07 2002
> +++ linux-2.5.46/drivers/block/genhd.c  Tue Nov  5 16:14:35 2002
> @@ -326,7 +326,13 @@
>  }
>  static inline unsigned MSEC(unsigned x)
>  {
> -       return x * 1000 / HZ;
> +#if 1000 % HZ == 0
> +       return x * (1000 / HZ);
> +#elif HZ % 1000 == 0
> +       return x / (HZ / 1000);
> +#else
> +       return (x / HZ) * 1000 + (x % HZ) * 1000 / HZ;
> +#endif
>  }

My brain just fell out.   Why don't we just do

	return (x / HZ) * 1000;

?

Yes, it'll return zero for the first second of disk activity.
I don't think that matters?

	return ((x + HZ / 2) / HZ) * 1000;

would be more accurate too, and reduces it to half a second ;)



--- 25/drivers/block/genhd.c~msec-fix	Tue Nov  5 21:59:35 2002
+++ 25-akpm/drivers/block/genhd.c	Tue Nov  5 22:02:17 2002
@@ -324,10 +324,12 @@ static ssize_t disk_size_read(struct gen
 {
 	return off ? 0 : sprintf(page, "%llu\n",(unsigned long long)get_capacity(disk));
 }
-static inline unsigned MSEC(unsigned x)
+
+static unsigned jiffies_to_msec(unsigned jif)
 {
-	return x * 1000 / HZ;
+	return ((jif + HZ / 2) / HZ) * 1000;
 }
+
 static ssize_t disk_stat_read(struct gendisk * disk,
 			      char *page, size_t count, loff_t off)
 {
@@ -338,11 +340,11 @@ static ssize_t disk_stat_read(struct gen
 		"%8u %8u %8u"
 		"\n",
 		disk->reads, disk->read_merges, (u64)disk->read_sectors,
-		MSEC(disk->read_ticks),
+		jiffies_to_msec(disk->read_ticks),
 		disk->writes, disk->write_merges, (u64)disk->write_sectors,
-		MSEC(disk->write_ticks),
-		disk->in_flight, MSEC(disk->io_ticks),
-		MSEC(disk->time_in_queue));
+		jiffies_to_msec(disk->write_ticks),
+		disk->in_flight, jiffies_to_msec(disk->io_ticks),
+		jiffies_to_msec(disk->time_in_queue));
 }
 static struct disk_attribute disk_attr_dev = {
 	.attr = {.name = "dev", .mode = S_IRUGO },

_

Re: [PATCH] 2.5.46: overflow in disk stats

[Andrew Morton]

Andrew Morton wrote:
> 
> Why don't we just do
> 
>         return (x / HZ) * 1000;
> 

Well that was pretty stupid.  Ignore me.  You probably already
have.  Let's do it your way.

Re: [PATCH] 2.5.46: overflow in disk stats

[Rick Lindsley]

    I see that the SARD changes have been merged, but MSEC() still has
    the overflow problem.  This takes care of it:

Thanks.  These patches left my control late in the game and have a couple
of other problems with them too. Watch for at least one more patch to
remove the old statistics counters as well. Inexplicably, we are now
counting statistics twice.

Rick