Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[James Bottomley]

On Thu, 2015-02-12 at 15:01 -0800, akpm@linux-foundation.org wrote:
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes
> 
> While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded
> to the correct SI range" says that Z and Y are included in preparation for
> 128 bit computers, they just waste .text currently.  If and when we get
> u128, string_get_size needs updating anyway (and ISO needs to come up with
> four more prefixes).

This is rubbish.  It's nothing to do with 128 bits.  This is to do with
disk sizes linux gets attached to.  The current largest device clusters
are Petabytes ... I think we may have some exabyte ones somewhere in the
Academic community, so it's by no means inconcievable we'll have
Zettabyte ones within a few years.  The SCSI standard, with 4k blocks
supports up to 2^76, which is well into Zettabytes.  We obviously run
off the mmap possibilities a lot sooner, because of the byte offsets,
but that's fixable.  Someone will probably start first by passing blocks
into that interface not bytes, so we'd like it not to be based on
assumptions that think 2^64 is the largest possible value.

> Also there's no need to include and test for the NULL sentinel; once we
> reach "E" size is at most 18.  [The test is also wrong; it should be
> units_str[units][i+1]; if we've reached NULL we're already doomed.]

So fix the bug, don't set us up to run off the end of the array.  And
please consult the community which keeps track of this rather than
trying to get it into Linux without review.

James

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[Andrew Morton]

On Thu, 12 Feb 2015 15:25:08 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:

> On Thu, 2015-02-12 at 15:01 -0800, akpm@linux-foundation.org wrote:
> > From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes
> > 
> > While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded
> > to the correct SI range" says that Z and Y are included in preparation for
> > 128 bit computers, they just waste .text currently.  If and when we get
> > u128, string_get_size needs updating anyway (and ISO needs to come up with
> > four more prefixes).
> 
> This is rubbish. It's nothing to do with 128 bits.  This is to do with
> disk sizes linux gets attached to.  The current largest device clusters
> are Petabytes ... I think we may have some exabyte ones somewhere in the
> Academic community, so it's by no means inconcievable we'll have
> Zettabyte ones within a few years.  The SCSI standard, with 4k blocks
> supports up to 2^76, which is well into Zettabytes.  We obviously run
> off the mmap possibilities a lot sooner, because of the byte offsets,
> but that's fixable.  Someone will probably start first by passing blocks
> into that interface not bytes, so we'd like it not to be based on
> assumptions that think 2^64 is the largest possible value.

I don't get it.  As the man says, this is presently dead code and
string_get_size() will need to be changed to work for disks larger than
2^64 bytes.  That change may be to take a u128 or it may be as you
suggest: replace the `u64 size' with `u64 size, u64 units' which is
effectively the same thing.

> > Also there's no need to include and test for the NULL sentinel; once we
> > reach "E" size is at most 18.  [The test is also wrong; it should be
> > units_str[units][i+1]; if we've reached NULL we're already doomed.]
> 
> So fix the bug, don't set us up to run off the end of the array.  And
> please consult the community which keeps track of this rather than
> trying to get it into Linux without review.

That seems a bit harsh - you've been cc'ed on this every step of the way.

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[James Bottomley]

On Thu, 2015-02-12 at 15:40 -0800, Andrew Morton wrote:
> On Thu, 12 Feb 2015 15:25:08 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:
> 
> > On Thu, 2015-02-12 at 15:01 -0800, akpm@linux-foundation.org wrote:
> > > From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > Subject: lib/string_helpers.c:string_get_size(): remove redundant prefixes
> > > 
> > > While 3c9f3681d0b4 "[SCSI] lib: add generic helper to print sizes rounded
> > > to the correct SI range" says that Z and Y are included in preparation for
> > > 128 bit computers, they just waste .text currently.  If and when we get
> > > u128, string_get_size needs updating anyway (and ISO needs to come up with
> > > four more prefixes).
> > 
> > This is rubbish. It's nothing to do with 128 bits.  This is to do with
> > disk sizes linux gets attached to.  The current largest device clusters
> > are Petabytes ... I think we may have some exabyte ones somewhere in the
> > Academic community, so it's by no means inconcievable we'll have
> > Zettabyte ones within a few years.  The SCSI standard, with 4k blocks
> > supports up to 2^76, which is well into Zettabytes.  We obviously run
> > off the mmap possibilities a lot sooner, because of the byte offsets,
> > but that's fixable.  Someone will probably start first by passing blocks
> > into that interface not bytes, so we'd like it not to be based on
> > assumptions that think 2^64 is the largest possible value.
> 
> I don't get it.  As the man says, this is presently dead code and
> string_get_size() will need to be changed to work for disks larger than
> 2^64 bytes.  That change may be to take a u128 or it may be as you
> suggest: replace the `u64 size' with `u64 size, u64 units' which is
> effectively the same thing.

The first thing someone's going to do is pass in blocks, because that's
the way the rest of block functions. If we're lucky the add "ZB" too,
but if not we run off the end in some obscure large cluster somewhere.
Don't set people up to make mistakes.

> > > Also there's no need to include and test for the NULL sentinel; once we
> > > reach "E" size is at most 18.  [The test is also wrong; it should be
> > > units_str[units][i+1]; if we've reached NULL we're already doomed.]
> > 
> > So fix the bug, don't set us up to run off the end of the array.  And
> > please consult the community which keeps track of this rather than
> > trying to get it into Linux without review.
> 
> That seems a bit harsh - you've been cc'ed on this every step of the way.

I think you need to check your scripts.  This is the first time I've
seen this patch, which is why I'm reacting this way.

James

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[Andrew Morton]

On Thu, 12 Feb 2015 15:45:29 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:

> ...
>
> > I don't get it.  As the man says, this is presently dead code and
> > string_get_size() will need to be changed to work for disks larger than
> > 2^64 bytes.  That change may be to take a u128 or it may be as you
> > suggest: replace the `u64 size' with `u64 size, u64 units' which is
> > effectively the same thing.
> 
> The first thing someone's going to do is pass in blocks, because that's
> the way the rest of block functions. If we're lucky the add "ZB" too,
> but if not we run off the end in some obscure large cluster somewhere.
> Don't set people up to make mistakes.

Well maybe.  A little bit.  But it assumes that someone is going to
make a change then not test it.

> > > > Also there's no need to include and test for the NULL sentinel; once we
> > > > reach "E" size is at most 18.  [The test is also wrong; it should be
> > > > units_str[units][i+1]; if we've reached NULL we're already doomed.]
> > > 
> > > So fix the bug, don't set us up to run off the end of the array.  And
> > > please consult the community which keeps track of this rather than
> > > trying to get it into Linux without review.
> > 
> > That seems a bit harsh - you've been cc'ed on this every step of the way.
> 
> I think you need to check your scripts.  This is the first time I've
> seen this patch, which is why I'm reacting this way.

No, James.Bottomley@HansenPartnership.com was cc'ed on the original
email and on the -mm spam.

Perhaps Rasmus should should also have cc'ed linux-scsi - practice
seems to vary a lot.  But he did cc the scsi maintainer and the author
of the patch he was modifying (yourself).


So I think the patch is reasonable and the way Rasmus and I handled it
is also reasonable.  Going nuts at us over it isn't reasonable!

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[James Bottomley]

On Thu, 2015-02-12 at 15:55 -0800, Andrew Morton wrote:
> On Thu, 12 Feb 2015 15:45:29 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:
> 
> > ...
> >
> > > I don't get it.  As the man says, this is presently dead code and
> > > string_get_size() will need to be changed to work for disks larger than
> > > 2^64 bytes.  That change may be to take a u128 or it may be as you
> > > suggest: replace the `u64 size' with `u64 size, u64 units' which is
> > > effectively the same thing.
> > 
> > The first thing someone's going to do is pass in blocks, because that's
> > the way the rest of block functions. If we're lucky the add "ZB" too,
> > but if not we run off the end in some obscure large cluster somewhere.
> > Don't set people up to make mistakes.
> 
> Well maybe.  A little bit.  But it assumes that someone is going to
> make a change then not test it.

That would be SOP for very large devices ... very few developers have
access.  However, since it's easy to fix, I'll do that instead of
complaining about the lack of analysis.

This fixes the routine and all the call sites to pass arguments in in
terms of a known block size.  Given that all the callers were scaling
from blocks to bytes anyway, it fits naturally and fixes the overflow
that would occur with very large devices in the old scheme.

James

---

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cdfbd21..26d2440 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -344,7 +344,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
 	struct request_queue *q = vblk->disk->queue;
 	char cap_str_2[10], cap_str_10[10];
 	char *envp[] = { "RESIZE=1", NULL };
-	u64 capacity, size;
+	u64 capacity;
 
 	/* Host must always specify the capacity. */
 	virtio_cread(vdev, struct virtio_blk_config, capacity, &capacity);
@@ -356,9 +356,10 @@ static void virtblk_config_changed_work(struct work_struct *work)
 		capacity = (sector_t)-1;
 	}
 
-	size = capacity * queue_logical_block_size(q);
-	string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
-	string_get_size(size, STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
+	string_get_size(capacity, queue_logical_block_size(q),
+			STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+	string_get_size(capacity, queue_logical_block_size(q),
+			STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
 
 	dev_notice(&vdev->dev,
 		  "new size: %llu %d-byte logical blocks (%s/%s)\n",
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c69afb5..2fc4269 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2230,7 +2230,7 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
 	part_md->part_type = part_type;
 	list_add(&part_md->part, &md->part);
 
-	string_get_size((u64)get_capacity(part_md->disk) << 9, STRING_UNITS_2,
+	string_get_size((u64)get_capacity(part_md->disk), 512, STRING_UNITS_2,
 			cap_str, sizeof(cap_str));
 	pr_info("%s: %s %s partition %u %s\n",
 	       part_md->disk->disk_name, mmc_card_id(card),
@@ -2436,7 +2436,7 @@ static int mmc_blk_probe(struct device *dev)
 	if (IS_ERR(md))
 		return PTR_ERR(md);
 
-	string_get_size((u64)get_capacity(md->disk) << 9, STRING_UNITS_2,
+	string_get_size((u64)get_capacity(md->disk), 512, STRING_UNITS_2,
 			cap_str, sizeof(cap_str));
 	pr_info("%s: %s %s %s %s\n",
 		md->disk->disk_name, mmc_card_id(card), mmc_card_name(card),
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6b78476..3c7fe4e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2235,11 +2235,11 @@ got_data:
 
 	{
 		char cap_str_2[10], cap_str_10[10];
-		u64 sz = (u64)sdkp->capacity << ilog2(sector_size);
 
-		string_get_size(sz, STRING_UNITS_2, cap_str_2,
-				sizeof(cap_str_2));
-		string_get_size(sz, STRING_UNITS_10, cap_str_10,
+		string_get_size(sdkp->capacity, sector_size,
+				STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+		string_get_size(sdkp->capacity, sector_size,
+				STRING_UNITS_10, cap_str_10,
 				sizeof(cap_str_10));
 
 		if (sdkp->first_scan || old_capacity != sdkp->capacity) {
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 6575718..2633280 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,7 +10,7 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
-void string_get_size(u64 size, enum string_size_units units,
+void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
 #define UNESCAPE_SPACE		0x01
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 8f8c441..bff19f8 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -4,6 +4,7 @@
  * Copyright 31 August 2008 James Bottomley
  * Copyright (C) 2013, Intel Corporation
  */
+#include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/math64.h>
 #include <linux/export.h>
@@ -14,7 +15,8 @@
 
 /**
  * string_get_size - get the size in the specified units
- * @size:	The size to be converted
+ * @size:	The size to be converted in blocks
+ * @blk_size:	Size of the block (use 1 for size in bytes)
  * @units:	units to use (powers of 1000 or 1024)
  * @buf:	buffer to format to
  * @len:	length of buffer
@@ -24,14 +26,14 @@
  * at least 9 bytes and will always be zero terminated.
  *
  */
-void string_get_size(u64 size, const enum string_size_units units,
+void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		     char *buf, int len)
 {
 	static const char *const units_10[] = {
-		"B", "kB", "MB", "GB", "TB", "PB", "EB"
+		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
 	};
 	static const char *const units_2[] = {
-		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
 	};
 	static const char *const *const units_str[] = {
 		[STRING_UNITS_10] = units_10,
@@ -42,31 +44,60 @@ void string_get_size(u64 size, const enum string_size_units units,
 		[STRING_UNITS_2] = 1024,
 	};
 	int i, j;
-	u32 remainder = 0, sf_cap;
+	u32 remainder = 0, sf_cap, exp;
 	char tmp[8];
+	const char *unit;
 
 	tmp[0] = '\0';
 	i = 0;
+	if (!size)
+	  goto out;
+
+	if (blk_size >= divisor[units]) {
+		while (blk_size >= divisor[units]) {
+			remainder = do_div(blk_size, divisor[units]);
+			i++;
+		}
+	}
+	exp = divisor[units];
+	do_div(exp, blk_size);
+	if (size >= exp) {
+		remainder = do_div(size, divisor[units]);
+		remainder *= blk_size;
+		i++;
+	} else {
+		remainder *= size;
+	}
+	size *= blk_size;
+	size += (remainder/divisor[units]);
+	remainder %= divisor[units];
+
 	if (size >= divisor[units]) {
 		while (size >= divisor[units]) {
 			remainder = do_div(size, divisor[units]);
 			i++;
 		}
+	}
 
-		sf_cap = size;
-		for (j = 0; sf_cap*10 < 1000; j++)
-			sf_cap *= 10;
+	sf_cap = size;
+	for (j = 0; sf_cap*10 < 1000; j++)
+		sf_cap *= 10;
 
-		if (j) {
-			remainder *= 1000;
-			remainder /= divisor[units];
-			snprintf(tmp, sizeof(tmp), ".%03u", remainder);
-			tmp[j+1] = '\0';
-		}
+	if (j) {
+		remainder *= 1000;
+		remainder /= divisor[units];
+		snprintf(tmp, sizeof(tmp), ".%03u", remainder);
+		tmp[j+1] = '\0';
 	}
 
+ out:
+	if (i >= ARRAY_SIZE(units_2))
+		unit = "UNK";
+	else
+		unit = units_str[units][i];
+
 	snprintf(buf, len, "%u%s %s", (u32)size,
-		 tmp, units_str[units][i]);
+		 tmp, unit);
 }
 EXPORT_SYMBOL(string_get_size);
 

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[Andrew Morton]

On Fri, 13 Feb 2015 16:05:54 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:

> @@ -42,31 +44,60 @@ void string_get_size(u64 size, const enum string_size_units units,
>  		[STRING_UNITS_2] = 1024,
>  	};
>  	int i, j;
> -	u32 remainder = 0, sf_cap;
> +	u32 remainder = 0, sf_cap, exp;
>  	char tmp[8];
> +	const char *unit;
>  
>  	tmp[0] = '\0';
>  	i = 0;
> +	if (!size)
> +	  goto out;

whitespace wart.

> +	if (blk_size >= divisor[units]) {
> +		while (blk_size >= divisor[units]) {
> +			remainder = do_div(blk_size, divisor[units]);
> +			i++;
> +		}
> +	}

The `if' doesn't do anything.

> +	exp = divisor[units];
> +	do_div(exp, blk_size);
> +	if (size >= exp) {
> +		remainder = do_div(size, divisor[units]);
> +		remainder *= blk_size;
> +		i++;
> +	} else {
> +		remainder *= size;
> +	}
> +	size *= blk_size;
> +	size += (remainder/divisor[units]);
> +	remainder %= divisor[units];
> +
>  	if (size >= divisor[units]) {
>  		while (size >= divisor[units]) {
>  			remainder = do_div(size, divisor[units]);
>  			i++;
>  		}
> +	}

Here too.

Re: [patch 069/104] lib/string_helpers.c:string_get_size(): remove redundant prefixes

[James Bottomley]

On Fri, 2015-02-13 at 17:02 -0800, Andrew Morton wrote:
> On Fri, 13 Feb 2015 16:05:54 -0800 James Bottomley <James.Bottomley@hansenpartnership.com> wrote:
> 
> > @@ -42,31 +44,60 @@ void string_get_size(u64 size, const enum string_size_units units,
> >  		[STRING_UNITS_2] = 1024,
> >  	};
> >  	int i, j;
> > -	u32 remainder = 0, sf_cap;
> > +	u32 remainder = 0, sf_cap, exp;
> >  	char tmp[8];
> > +	const char *unit;
> >  
> >  	tmp[0] = '\0';
> >  	i = 0;
> > +	if (!size)
> > +	  goto out;
> 
> whitespace wart.

Yes, I'll run it through checkpatch before submitting.

> > +	if (blk_size >= divisor[units]) {
> > +		while (blk_size >= divisor[units]) {
> > +			remainder = do_div(blk_size, divisor[units]);
> > +			i++;
> > +		}
> > +	}
> 
> The `if' doesn't do anything.

Right ... I just copied from the original, but it's true we can unwrap
this.

> > +	exp = divisor[units];
> > +	do_div(exp, blk_size);
> > +	if (size >= exp) {
> > +		remainder = do_div(size, divisor[units]);
> > +		remainder *= blk_size;
> > +		i++;
> > +	} else {
> > +		remainder *= size;
> > +	}
> > +	size *= blk_size;
> > +	size += (remainder/divisor[units]);
> > +	remainder %= divisor[units];
> > +
> >  	if (size >= divisor[units]) {
> >  		while (size >= divisor[units]) {
> >  			remainder = do_div(size, divisor[units]);
> >  			i++;
> >  		}
> > +	}
> 
> Here too.

Certainly,

James