bcachefs: rename version -> bversion for big endian builds

[PATCH] bcachefs: rename version -> bversion for big endian builds

Posted by Guenter Roeck 1 month, 4 weeks ago

Builds on big endian systems fail as follows.

fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
fs/bcachefs/bkey.h:557:41: error:
	'const struct bkey' has no member named 'bversion'

The original commit only renamed the variable for little endian builds.
Rename it for big endian builds as well to fix the problem.

Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 fs/bcachefs/bcachefs_format.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
index 203ee627cab5..84832c2d4df9 100644
--- a/fs/bcachefs/bcachefs_format.h
+++ b/fs/bcachefs/bcachefs_format.h
@@ -223,7 +223,7 @@ struct bkey {
 #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
 	struct bpos	p;
 	__u32		size;		/* extent size, in sectors */
-	struct bversion	version;
+	struct bversion	bversion;
 
 	__u8		pad[1];
 #endif
-- 
2.45.2

Re: [PATCH] bcachefs: rename version -> bversion for big endian builds

Posted by Geert Uytterhoeven 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> Builds on big endian systems fail as follows.
>
> fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> fs/bcachefs/bkey.h:557:41: error:
>         'const struct bkey' has no member named 'bversion'
>
> The original commit only renamed the variable for little endian builds.
> Rename it for big endian builds as well to fix the problem.
>
> Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

As this fixes the build for me:
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

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] bcachefs: rename version -> bversion for big endian builds

Posted by Geert Uytterhoeven 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> Builds on big endian systems fail as follows.
>
> fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> fs/bcachefs/bkey.h:557:41: error:
>         'const struct bkey' has no member named 'bversion'
>
> The original commit only renamed the variable for little endian builds.
> Rename it for big endian builds as well to fix the problem.
>
> Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")

Which is (again) not found on any mailing list, and has never been in
linux-next before it hit upstream...

> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

> --- a/fs/bcachefs/bcachefs_format.h
> +++ b/fs/bcachefs/bcachefs_format.h
> @@ -223,7 +223,7 @@ struct bkey {
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>         struct bpos     p;
>         __u32           size;           /* extent size, in sectors */
> -       struct bversion version;
> +       struct bversion bversion;
>
>         __u8            pad[1];
>  #endif

BTW, how does this work when accessing a non-native file system?
Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
converted from a bi-endian to a little-endian file system?

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] bcachefs: rename version -> bversion for big endian builds

Posted by Kent Overstreet 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 12:04:42PM GMT, Geert Uytterhoeven wrote:
> On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > Builds on big endian systems fail as follows.
> >
> > fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> > fs/bcachefs/bkey.h:557:41: error:
> >         'const struct bkey' has no member named 'bversion'
> >
> > The original commit only renamed the variable for little endian builds.
> > Rename it for big endian builds as well to fix the problem.
> >
> > Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> 
> Which is (again) not found on any mailing list, and has never been in
> linux-next before it hit upstream...
> 
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> > --- a/fs/bcachefs/bcachefs_format.h
> > +++ b/fs/bcachefs/bcachefs_format.h
> > @@ -223,7 +223,7 @@ struct bkey {
> >  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> >         struct bpos     p;
> >         __u32           size;           /* extent size, in sectors */
> > -       struct bversion version;
> > +       struct bversion bversion;
> >
> >         __u8            pad[1];
> >  #endif
> 
> BTW, how does this work when accessing a non-native file system?
> Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
> converted from a bi-endian to a little-endian file system?

we byte swab if necessary

Re: [PATCH] bcachefs: rename version -> bversion for big endian builds

Posted by Geert Uytterhoeven 1 month, 4 weeks ago

Hi Kent,

On Mon, Sep 30, 2024 at 12:11 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
> On Mon, Sep 30, 2024 at 12:04:42PM GMT, Geert Uytterhoeven wrote:
> > On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > > Builds on big endian systems fail as follows.
> > >
> > > fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> > > fs/bcachefs/bkey.h:557:41: error:
> > >         'const struct bkey' has no member named 'bversion'
> > >
> > > The original commit only renamed the variable for little endian builds.
> > > Rename it for big endian builds as well to fix the problem.
> > >
> > > Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> >
> > Which is (again) not found on any mailing list, and has never been in
> > linux-next before it hit upstream...
> >
> > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >
> > > --- a/fs/bcachefs/bcachefs_format.h
> > > +++ b/fs/bcachefs/bcachefs_format.h
> > > @@ -223,7 +223,7 @@ struct bkey {
> > >  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > >         struct bpos     p;
> > >         __u32           size;           /* extent size, in sectors */
> > > -       struct bversion version;
> > > +       struct bversion bversion;
> > >
> > >         __u8            pad[1];
> > >  #endif
> >
> > BTW, how does this work when accessing a non-native file system?
> > Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
> > converted from a bi-endian to a little-endian file system?
>
> we byte swab if necessary

So you have to test 4 combinations instead of 2 (which you don't do,
obviously ;-)

Ext2 was converted from a bi-endian to a little-endian file system
because it turned out the conditional byte-swapping was more
expensive than unconditional (not) byte-swapping. Given all the
bcache structures are already tagged with __packed anyway, I guess
this is even more true for bcachefs.

The proper way established +25y ago was to settle on one endianness
layout for all on-disk data. That way you do not have to duplicate
data and code for little vs. big endian, keep both paths in sync, and
you can annotate everything with __[bl]eXX attributes to let sparse
help you catch bugs.

Which endianness to pick is up to you. Ext2 settled on little-endian,
XFS on big-endian.

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] bcachefs: rename version -> bversion for big endian builds

Posted by Kent Overstreet 1 month, 4 weeks ago

On Mon, Sep 30, 2024 at 02:08:25PM GMT, Geert Uytterhoeven wrote:
> Hi Kent,
> 
> On Mon, Sep 30, 2024 at 12:11 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> > On Mon, Sep 30, 2024 at 12:04:42PM GMT, Geert Uytterhoeven wrote:
> > > On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > Builds on big endian systems fail as follows.
> > > >
> > > > fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> > > > fs/bcachefs/bkey.h:557:41: error:
> > > >         'const struct bkey' has no member named 'bversion'
> > > >
> > > > The original commit only renamed the variable for little endian builds.
> > > > Rename it for big endian builds as well to fix the problem.
> > > >
> > > > Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> > >
> > > Which is (again) not found on any mailing list, and has never been in
> > > linux-next before it hit upstream...
> > >
> > > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > >
> > > > --- a/fs/bcachefs/bcachefs_format.h
> > > > +++ b/fs/bcachefs/bcachefs_format.h
> > > > @@ -223,7 +223,7 @@ struct bkey {
> > > >  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > > >         struct bpos     p;
> > > >         __u32           size;           /* extent size, in sectors */
> > > > -       struct bversion version;
> > > > +       struct bversion bversion;
> > > >
> > > >         __u8            pad[1];
> > > >  #endif
> > >
> > > BTW, how does this work when accessing a non-native file system?
> > > Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
> > > converted from a bi-endian to a little-endian file system?
> >
> > we byte swab if necessary
> 
> So you have to test 4 combinations instead of 2 (which you don't do,
> obviously ;-)
> 
> Ext2 was converted from a bi-endian to a little-endian file system
> because it turned out the conditional byte-swapping was more
> expensive than unconditional (not) byte-swapping. Given all the
> bcache structures are already tagged with __packed anyway, I guess
> this is even more true for bcachefs.
> 
> The proper way established +25y ago was to settle on one endianness
> layout for all on-disk data. That way you do not have to duplicate
> data and code for little vs. big endian, keep both paths in sync, and
> you can annotate everything with __[bl]eXX attributes to let sparse
> help you catch bugs.
> 
> Which endianness to pick is up to you. Ext2 settled on little-endian,
> XFS on big-endian.

If you peruse that code even slightly, you'll see that what we're doing
is treating the key as a multi word integer, so word order has to match
machine byte order in order for various things in the btree lookup code
to work.

But sure, try and tell me there's something about filesystems I don't
already know...

Re: [PATCH] bcachefs: rename version -> bversion for big endian builds

Posted by Geert Uytterhoeven 1 month, 4 weeks ago

Hi Kent,

On Mon, Sep 30, 2024 at 2:31 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
> On Mon, Sep 30, 2024 at 02:08:25PM GMT, Geert Uytterhoeven wrote:
> > On Mon, Sep 30, 2024 at 12:11 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > > On Mon, Sep 30, 2024 at 12:04:42PM GMT, Geert Uytterhoeven wrote:
> > > > On Mon, Sep 30, 2024 at 2:39 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > > Builds on big endian systems fail as follows.
> > > > >
> > > > > fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> > > > > fs/bcachefs/bkey.h:557:41: error:
> > > > >         'const struct bkey' has no member named 'bversion'
> > > > >
> > > > > The original commit only renamed the variable for little endian builds.
> > > > > Rename it for big endian builds as well to fix the problem.
> > > > >
> > > > > Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")
> > > >
> > > > Which is (again) not found on any mailing list, and has never been in
> > > > linux-next before it hit upstream...
> > > >
> > > > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > >
> > > > > --- a/fs/bcachefs/bcachefs_format.h
> > > > > +++ b/fs/bcachefs/bcachefs_format.h
> > > > > @@ -223,7 +223,7 @@ struct bkey {
> > > > >  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > > > >         struct bpos     p;
> > > > >         __u32           size;           /* extent size, in sectors */
> > > > > -       struct bversion version;
> > > > > +       struct bversion bversion;
> > > > >
> > > > >         __u8            pad[1];
> > > > >  #endif
> > > >
> > > > BTW, how does this work when accessing a non-native file system?
> > > > Didn't we stop doing bi-endian file systems in v2.1.10, when ext2 was
> > > > converted from a bi-endian to a little-endian file system?
> > >
> > > we byte swab if necessary
> >
> > So you have to test 4 combinations instead of 2 (which you don't do,
> > obviously ;-)
> >
> > Ext2 was converted from a bi-endian to a little-endian file system
> > because it turned out the conditional byte-swapping was more
> > expensive than unconditional (not) byte-swapping. Given all the
> > bcache structures are already tagged with __packed anyway, I guess
> > this is even more true for bcachefs.
> >
> > The proper way established +25y ago was to settle on one endianness
> > layout for all on-disk data. That way you do not have to duplicate
> > data and code for little vs. big endian, keep both paths in sync, and
> > you can annotate everything with __[bl]eXX attributes to let sparse
> > help you catch bugs.
> >
> > Which endianness to pick is up to you. Ext2 settled on little-endian,
> > XFS on big-endian.
>
> If you peruse that code even slightly, you'll see that what we're doing
> is treating the key as a multi word integer, so word order has to match
> machine byte order in order for various things in the btree lookup code
> to work.

I have seen the multi-word integers...

> But sure, try and tell me there's something about filesystems I don't
> already know...

Okay...

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] bcachefs: rename version -> bversion for big endian builds

Posted by Kent Overstreet 1 month, 4 weeks ago

On Sun, Sep 29, 2024 at 05:39:02PM GMT, Guenter Roeck wrote:
> Builds on big endian systems fail as follows.
> 
> fs/bcachefs/bkey.h: In function 'bch2_bkey_format_add_key':
> fs/bcachefs/bkey.h:557:41: error:
> 	'const struct bkey' has no member named 'bversion'
> 
> The original commit only renamed the variable for little endian builds.
> Rename it for big endian builds as well to fix the problem.
> 
> Fixes: cf49f8a8c277 ("bcachefs: rename version -> bversion")

Thanks, applied