| Message ID | 20210610110001.2805317-1-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: Fix 64-bit division on 32-bit in xlog_state_switch_iclogs() | expand |
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.
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
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
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.
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
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)
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) {
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(-)