2002-11-06 05:01:38

by Lev Makhlis

Subject: [PATCH] 2.5.46: overflow in disk stats

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)

2002-11-06 06:00:09

by Andrew Morton

Subject: Re: [PATCH] 2.5.46: overflow in disk stats

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 },

_

2002-11-06 06:14:18

by Andrew Morton

Subject: Re: [PATCH] 2.5.46: overflow in disk stats

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.

2002-11-06 07:14:14

by Rick Lindsley

Subject: Re: [PATCH] 2.5.46: overflow in disk stats

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