[PATCH] bcachefs: omit alignment attribute on big endian struct bkey

[PATCH] bcachefs: omit alignment attribute on big endian struct bkey

[Thomas Bertschinger]

This is needed for building Rust bindings on big endian architectures
like s390x. Currently this is only done in userspace, but it might
happen in-kernel in the future. When creating a Rust binding for struct
bkey, the "packed" attribute is needed to get a type with the correct
member offsets. However, rustc does not allow types to have both a
"packed" and "align" attribute. Thus, in order to get a Rust type
compatible with the C type, on big endian systems we must omit the
"aligned" attribute in C.

This does not affect the struct's size or member offsets, only its
toplevel alignment, which should be an acceptable impact.

Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 fs/bcachefs/bcachefs_format.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
index 1bb24aa73528..fc12efe8f8cd 100644
--- a/fs/bcachefs/bcachefs_format.h
+++ b/fs/bcachefs/bcachefs_format.h
@@ -222,7 +222,11 @@ struct bkey {
 
 	__u8		pad[1];
 #endif
-} __packed __aligned(8);
+} __packed
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+__aligned(8)
+#endif
+;
 
 struct bkey_packed {
 	__u64		_data[0];
-- 
2.43.0

Re: [PATCH] bcachefs: omit alignment attribute on big endian struct bkey

[Brian Foster]

On Thu, Feb 15, 2024 at 09:19:57AM -0700, Thomas Bertschinger wrote:
> This is needed for building Rust bindings on big endian architectures
> like s390x. Currently this is only done in userspace, but it might
> happen in-kernel in the future. When creating a Rust binding for struct
> bkey, the "packed" attribute is needed to get a type with the correct
> member offsets. However, rustc does not allow types to have both a
> "packed" and "align" attribute. Thus, in order to get a Rust type
> compatible with the C type, on big endian systems we must omit the
> "aligned" attribute in C.
> 
> This does not affect the struct's size or member offsets, only its
> toplevel alignment, which should be an acceptable impact.
> 
> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
> ---
>  fs/bcachefs/bcachefs_format.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
> index 1bb24aa73528..fc12efe8f8cd 100644
> --- a/fs/bcachefs/bcachefs_format.h
> +++ b/fs/bcachefs/bcachefs_format.h
> @@ -222,7 +222,11 @@ struct bkey {
>  
>  	__u8		pad[1];
>  #endif
> -} __packed __aligned(8);
> +} __packed

I can't really comment on the Rust context, but could you add a comment
to explain why the ifdef is here? I.e., even something as simple as
"Rust disallows packed and aligned on ..." is useful.

Also out of curiosity, is there some reason these two attributes
together presumably isn't a problem for LE?

Brian

> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +__aligned(8)
> +#endif
> +;
>  
>  struct bkey_packed {
>  	__u64		_data[0];
> -- 
> 2.43.0
> 

Re: [PATCH] bcachefs: omit alignment attribute on big endian struct bkey

[Thomas Bertschinger]

On Thu, Feb 15, 2024 at 12:23:12PM -0500, Brian Foster wrote:
> I can't really comment on the Rust context, but could you add a comment
> to explain why the ifdef is here? I.e., even something as simple as
> "Rust disallows packed and aligned on ..." is useful.
> 
> Brian

I'll work on a v2 with an explanatory comment.

> Also out of curiosity, is there some reason these two attributes
> together presumably isn't a problem for LE?

The root of the problem is that rustc won't compile types with both
packed and align attributes. However, rust-bindgen tries to be helpful
and when generating Rust bindings for C types with both attrs, will
avoid placing one or the other attribute if it can do so without
changing the type's layout.

For LE, the "packed" attr is unneeded because the struct is "naturally
packed", but the "align(8)" attr is needed because the struct's natural
alignment is 4.

For BE, the "packed" attr is needed because there are some members that
are not aligned (e.g., `size` has as offset of 23 but wants to be at
24), so rust-bindgen cannot omit it. Thus we have to remove "align(8)"
to get a type that rustc can compile.

- Thomas Bertschinger

Re: [PATCH] bcachefs: omit alignment attribute on big endian struct bkey

[Kent Overstreet]

On Thu, Feb 15, 2024 at 11:02:22AM -0700, Thomas Bertschinger wrote:
> On Thu, Feb 15, 2024 at 12:23:12PM -0500, Brian Foster wrote:
> > I can't really comment on the Rust context, but could you add a comment
> > to explain why the ifdef is here? I.e., even something as simple as
> > "Rust disallows packed and aligned on ..." is useful.
> > 
> > Brian
> 
> I'll work on a v2 with an explanatory comment.
> 
> > Also out of curiosity, is there some reason these two attributes
> > together presumably isn't a problem for LE?
> 
> The root of the problem is that rustc won't compile types with both
> packed and align attributes. However, rust-bindgen tries to be helpful
> and when generating Rust bindings for C types with both attrs, will
> avoid placing one or the other attribute if it can do so without
> changing the type's layout.
> 
> For LE, the "packed" attr is unneeded because the struct is "naturally
> packed", but the "align(8)" attr is needed because the struct's natural
> alignment is 4.
> 
> For BE, the "packed" attr is needed because there are some members that
> are not aligned (e.g., `size` has as offset of 23 but wants to be at
> 24), so rust-bindgen cannot omit it. Thus we have to remove "align(8)"
> to get a type that rustc can compile.

Specifically, when i was designing bkey, I wanted the header to be no
bigger than necessary so that bkey_packed could use the rest. That means
that decently offten extent keys will fit into only 8 bytes, instead of
spilling over to 16.

But packed_bkey treats the part after the header - the packed section -
as a single multi word, variable length integer. And bkey, the unpacked
version, is just a special case version of a bkey_packed; all the packed
bkey code will work on keys in any packed format, the in-memory
representation of an unpacked key also is just one type of packed key...

So that constrains the key part of a bkig endian bkey to start right
after the header...

So if we ever do a bkey_v2 and need to expand the hedaer by another byte
for some reason, that would clean up that wart :)