Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Sat, 12 Jun 2021 10:54:23 +0200 | Subject | Re: [PATCH] xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs() |
| |
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
| |