Skip to contentSkip to conversation

2.5.46: overflow in disk stats

PATCH In review Lev Makhlis 3 participants 4 messages Nov 6, 2002 Block layer

lore
Share & tools
lore

Lev Makhlis

author
#0 lore raw

PATCH 1/1 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

drivers/block/genhd.c +7 -1
@@ -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)
#1 lore raw
Quoted 15 lines · 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

drivers/block/genhd.c Tue Nov 5 16:14:35 2002 renamed from drivers/block/genhd.c Tue Nov 5 15:15:07 2002 +0 -0
@@ -326,7 +326,13 @@
 }
 static inline unsigned MSEC(unsigned x)

drivers/block/genhd.c Tue Nov 5 16:14:35 2002 line 336

Quoted 10 lines
drivers/block/genhd.c Tue Nov 5 16:14:35 2002 renamed from drivers/block/genhd.c Tue Nov 5 15:15:07 2002 +7 -1
 {
-       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 ;)

drivers/block/genhd.c renamed from drivers/block/genhd.c~msec-fix +8 -6
@@ -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 },

_

#2 lore raw
Quoted 5 lines · Andrew Morton

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.

#3 lore raw
In reply to Lev Makhlis

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