[PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Geert Uytterhoeven]

On 32-bit (e.g. m68k):

    ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!

Fix this by using a uint32_t intermediate, like before.

Reported-by: noreply@ellerman.id.au
Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Compile-tested only.
---
 fs/xfs/xfs_log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0e563ff8cd3be4aa..0c91da5defee6b9f 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3143,8 +3143,8 @@ xlog_state_switch_iclogs(
 
 	/* Round up to next log-sunit */
 	if (log->l_iclog_roundoff > BBSIZE) {
-		log->l_curr_block = roundup(log->l_curr_block,
-						BTOBB(log->l_iclog_roundoff));
+		uint32_t sunit_bb = BTOBB(log->l_iclog_roundoff);
+		log->l_curr_block = roundup(log->l_curr_block, sunit_bb);
 	}
 
 	if (log->l_curr_block >= log->l_logBBsize) {
-- 
2.25.1

Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Dave Chinner]

On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
> On 32-bit (e.g. m68k):
> 
>     ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!
> 
> Fix this by using a uint32_t intermediate, like before.
> 
> Reported-by: noreply@ellerman.id.au
> Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Compile-tested only.
> ---
>  fs/xfs/xfs_log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

<sigh>

64 bit division on 32 bit platforms is still a problem in this day
and age?

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Maybe we should just put "requires 64 bit kernel" on XFS these days...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Darrick J. Wong]

On Fri, Jun 11, 2021 at 08:01:55AM +1000, Dave Chinner wrote:
> On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
> > On 32-bit (e.g. m68k):
> > 
> >     ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!
> > 
> > Fix this by using a uint32_t intermediate, like before.
> > 
> > Reported-by: noreply@ellerman.id.au
> > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Compile-tested only.
> > ---
> >  fs/xfs/xfs_log.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> <sigh>
> 
> 64 bit division on 32 bit platforms is still a problem in this day
> and age?
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Maybe we should just put "requires 64 bit kernel" on XFS these days...

But then how will I recover my 100TB XFS using my TV?  It only has so
much framebuffer that I can abuse for swap memory...  >:O

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Geert Uytterhoeven]

Hi Dave,

On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
> > On 32-bit (e.g. m68k):
> >
> >     ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!
> >
> > Fix this by using a uint32_t intermediate, like before.
> >
> > Reported-by: noreply@ellerman.id.au
> > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Compile-tested only.
> > ---
> >  fs/xfs/xfs_log.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> <sigh>
>
> 64 bit division on 32 bit platforms is still a problem in this day
> and age?

They're not a problem.  But you should use the right operations from
<linux/math64.h>, iff you really need these expensive operations.

> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Dave Chinner]

On Fri, Jun 11, 2021 at 08:55:24AM +0200, Geert Uytterhoeven wrote:
> Hi Dave,
> 
> On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
> > > On 32-bit (e.g. m68k):
> > >
> > >     ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!
> > >
> > > Fix this by using a uint32_t intermediate, like before.
> > >
> > > Reported-by: noreply@ellerman.id.au
> > > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > ---
> > > Compile-tested only.
> > > ---
> > >  fs/xfs/xfs_log.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > <sigh>
> >
> > 64 bit division on 32 bit platforms is still a problem in this day
> > and age?
> 
> They're not a problem.  But you should use the right operations from
> <linux/math64.h>, iff you really need these expensive operations.

See, that's the whole problem. This *isn't* obviously a 64 bit
division - BBTOB is shifting the variable down by 9 (bytes to blocks)
and then using that as the divisor.

The problem is that BBTOB has an internal cast to a 64 bit size,
and roundup() just blindly takes it and hence we get non-obvious
compile errors only on 32 bit platforms.

We have type checking macros for all sorts of generic functionality
- why haven't these generic macros that do division also have type
checking to catch this? i.e. so that when people build kernels on
64 bit machines find out that they've unwittingly broken 32 bit
builds the moment they use roundup() and/or friends incorrectly?

That would save a lot of extra work having fix crap like this up
after the fact...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[Geert Uytterhoeven]

Hi Dave,

On Sat, Jun 12, 2021 at 12:46 AM Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Jun 11, 2021 at 08:55:24AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@fromorbit.com> wrote:
> > > On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
> > > > On 32-bit (e.g. m68k):
> > > >
> > > >     ERROR: modpost: "__udivdi3" [fs/xfs/xfs.ko] undefined!
> > > >
> > > > Fix this by using a uint32_t intermediate, like before.
> > > >
> > > > Reported-by: noreply@ellerman.id.au
> > > > Fixes: 7660a5b48fbef958 ("xfs: log stripe roundoff is a property of the log")
> > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > ---
> > > > Compile-tested only.
> > > > ---
> > > >  fs/xfs/xfs_log.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > <sigh>
> > >
> > > 64 bit division on 32 bit platforms is still a problem in this day
> > > and age?
> >
> > They're not a problem.  But you should use the right operations from
> > <linux/math64.h>, iff you really need these expensive operations.
>
> See, that's the whole problem. This *isn't* obviously a 64 bit
> division - BBTOB is shifting the variable down by 9 (bytes to blocks)
> and then using that as the divisor.

BTOBB, not BBTOB ;-)

> The problem is that BBTOB has an internal cast to a 64 bit size,
> and roundup() just blindly takes it and hence we get non-obvious
> compile errors only on 32 bit platforms.

Indeed. Perhaps the macros should be fixed?

    #define BBSHIFT         9
    #define BBSIZE          (1<<BBSHIFT)
    #define BBMASK          (BBSIZE-1)
    #define BTOBB(bytes)    (((__u64)(bytes) + BBSIZE - 1) >> BBSHIFT)
    #define BTOBBT(bytes)   ((__u64)(bytes) >> BBSHIFT)

Why are these two casting bytes to u64? The result will be smaller
due to the shift.
if the type holding bytes was too small, you're screwed anyway.

    #define BBTOB(bbs)      ((bbs) << BBSHIFT)

Why does this one lack the cast? If the passed bbs is ever 32-bit,
it may overflow due to the shift.

> We have type checking macros for all sorts of generic functionality
> - why haven't these generic macros that do division also have type
> checking to catch this? i.e. so that when people build kernels on
> 64 bit machines find out that they've unwittingly broken 32 bit
> builds the moment they use roundup() and/or friends incorrectly?
>
> That would save a lot of extra work having fix crap like this up
> after the fact...

While adding checks would work for e.g. roundup(), it wouldn't work
for plain divisions not involving rounding, as we don't have a way to
catch this for "a / b", except for the link error on 32-bit platforms.

Perhaps the build bots are not monitoring linux-xfs?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs()

[David Laight]

From: Geert Uytterhoeven
> Sent: 11 June 2021 07:55
> Hi Dave,
> 
> On Fri, Jun 11, 2021 at 12:02 AM Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Jun 10, 2021 at 01:00:01PM +0200, Geert Uytterhoeven wrote:
...
> > 64 bit division on 32 bit platforms is still a problem in this day
> > and age?
> 
> They're not a problem.  But you should use the right operations from
> <linux/math64.h>, iff you really need these expensive operations.

(64bit) division isn't exactly cheap on 64bit cpus.

Some timing tables for x86 give latencies of well over 1 bit/clock
for Intel cpus, AMD ryzen manage 2 bits/clock.
Signed divide is also significantly more expensive than
unsigned divide.

Integer divide performance is clearly not important enough
to throw silicon at.
The same tables show fdiv having a latency of 16 clocks.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)