[PATCH 1/2] lib/lcm.c: Ensure correct result whenever it fits

[PATCH 1/2] lib/lcm.c: Ensure correct result whenever it fits

[Rasmus Villemoes]

Ensure that lcm(a,b) returns the mathematically correct result,
provided it fits in an unsigned long. The current version returns
garbage if a*b overflows, even if the final result would fit.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
There are of course still plenty of cases where the return value is
wrong.

 lib/lcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/lcm.c b/lib/lcm.c
index b9c8de4..01b3aa9 100644
--- a/lib/lcm.c
+++ b/lib/lcm.c
@@ -7,7 +7,7 @@
 unsigned long lcm(unsigned long a, unsigned long b)
 {
 	if (a && b)
-		return (a * b) / gcd(a, b);
+		return (a / gcd(a, b)) * b;
 	else if (b)
 		return b;
 
-- 
1.8.4.rc3.2.g61bff3f

[PATCH 2/2] lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not n

[Rasmus Villemoes]

Return the mathematically correct answer when an argument is 0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
I don't think there is any instance of lcm(0,n) in the kernel, but at
least this reduces code size a little.

 lib/lcm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/lcm.c b/lib/lcm.c
index 01b3aa9..51cc6b1 100644
--- a/lib/lcm.c
+++ b/lib/lcm.c
@@ -8,9 +8,7 @@ unsigned long lcm(unsigned long a, unsigned long b)
 {
 	if (a && b)
 		return (a / gcd(a, b)) * b;
-	else if (b)
-		return b;
-
-	return a;
+	else
+		return 0;
 }
 EXPORT_SYMBOL_GPL(lcm);
-- 
1.8.4.rc3.2.g61bff3f

Re: [PATCH 1/2] lib/lcm.c: Ensure correct result whenever it fits

[Rasmus Villemoes]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

Ping...

> Ensure that lcm(a,b) returns the mathematically correct result,
> provided it fits in an unsigned long. The current version returns
> garbage if a*b overflows, even if the final result would fit.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> There are of course still plenty of cases where the return value is
> wrong.
>
>  lib/lcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/lcm.c b/lib/lcm.c
> index b9c8de4..01b3aa9 100644
> --- a/lib/lcm.c
> +++ b/lib/lcm.c
> @@ -7,7 +7,7 @@
>  unsigned long lcm(unsigned long a, unsigned long b)
>  {
>  	if (a && b)
> -		return (a * b) / gcd(a, b);
> +		return (a / gcd(a, b)) * b;
>  	else if (b)
>  		return b;

Re: [PATCH 2/2] lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not n

[Rasmus Villemoes]

On Sun, Mar 29 2015, Mike Snitzer <snitzer@redhat.com> wrote:

> On Tue, Dec 9, 2014 at 4:03 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Return the mathematically correct answer when an argument is 0.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> This change is the source of 3.19 regression for stacking device
> limits, via commit 69c953c ("lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not
> n").
>
>
> Rasmus, mathematical purity of lcm() aside, it'd have been nice if you
> looked at the lcm() callers to determine whether you'd be breaking
> them.

I'm sorry about this. I thought I did check the callers, but evidently
not well enough.

Rasmus

Re: [PATCH 2/2] lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not n

[Rasmus Villemoes]

On Sun, Mar 29 2015, Mike Snitzer <snitzer@redhat.com> wrote:

> On Tue, Dec 9, 2014 at 4:03 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> Return the mathematically correct answer when an argument is 0.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> This change is the source of 3.19 regression for stacking device
> limits, via commit 69c953c ("lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not
> n").
>
>
> Rasmus, mathematical purity of lcm() aside, it'd have been nice if you
> looked at the lcm() callers to determine whether you'd be breaking
> them.

I'm sorry about this. I thought I did check the callers, but evidently
not well enough.

Rasmus

Re: [PATCH 2/2] lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not n

[Mike Snitzer]

On Tue, Dec 9, 2014 at 4:03 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Return the mathematically correct answer when an argument is 0.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/lcm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/lcm.c b/lib/lcm.c
> index 01b3aa922dda..51cc6b13cd52 100644
> --- a/lib/lcm.c
> +++ b/lib/lcm.c
> @@ -8,9 +8,7 @@ unsigned long lcm(unsigned long a, unsigned long b)
>  {
>         if (a && b)
>                 return (a / gcd(a, b)) * b;
> -       else if (b)
> -               return b;
> -
> -       return a;
> +       else
> +               return 0;
>  }
>  EXPORT_SYMBOL_GPL(lcm);

This change is the source of 3.19 regression for stacking device
limits, via commit 69c953c ("lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not
n").

Test:
# modprobe scsi_debug dev_size_mb=10 num_tgts=1 opt_blks=1536
# cat /sys/block/sde/queue/optimal_io_size
786432
# dmsetup create node --table "0 100 linear /dev/sde 0"

Before commit 69c953c:
# cat /sys/block/dm-5/queue/optimal_io_size
786432

After commit 69c953c:
# cat /sys/block/dm-5/queue/optimal_io_size
0

Rasmus, mathematical purity of lcm() aside, it'd have been nice if you
looked at the lcm() callers to determine whether you'd be breaking
them.

It looks like we need a new lcm_not_zero() and blk_stack_limits()
should be changed to use it, and the patch needs to cc stable.  Martin
and/or Jens, what do you think?

Mike

[PATCH 2/2] lib/lcm.c: lcm(n,0)=lcm(0,n) is 0, not n

[Rasmus Villemoes]

Return the mathematically correct answer when an argument is 0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/lcm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/lcm.c b/lib/lcm.c
index 01b3aa922dda..51cc6b13cd52 100644
--- a/lib/lcm.c
+++ b/lib/lcm.c
@@ -8,9 +8,7 @@ unsigned long lcm(unsigned long a, unsigned long b)
 {
 	if (a && b)
 		return (a / gcd(a, b)) * b;
-	else if (b)
-		return b;
-
-	return a;
+	else
+		return 0;
 }
 EXPORT_SYMBOL_GPL(lcm);
-- 
2.1.3