* [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-20 14:22 [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user Rasmus Villemoes
@ 2025-10-20 14:22 ` Rasmus Villemoes
2025-10-22 16:15 ` Nathan Chancellor
2025-10-20 14:22 ` [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path Rasmus Villemoes
2025-10-22 5:30 ` [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user David Sterba
2 siblings, 1 reply; 13+ messages in thread
From: Rasmus Villemoes @ 2025-10-20 14:22 UTC (permalink / raw)
To: Linus Torvalds, David Sterba
Cc: linux-btrfs, Nathan Chancellor, Nick Desaulniers, linux-kbuild,
Rasmus Villemoes
Once in a while, it turns out that enabling -fms-extensions could
allow some slightly prettier code. But every time it has come up, the
code that had to be used instead has been deemed "not too awful" and
not worth introducing another compiler flag for.
That's probably true for each individual case, but then it's somewhat
of a chicken/egg situation.
If we just "bite the bullet" as Linus says and enable it once and for
all, it is available whenever a use case turns up, and no individual
case has to justify it.
A lore.kernel.org search provides these examples:
- https://lore.kernel.org/lkml/200706301813.58435.agruen@suse.de/
- https://lore.kernel.org/lkml/20180419152817.GD25406@bombadil.infradead.org/
- https://lore.kernel.org/lkml/170622208395.21664.2510213291504081000@noble.neil.brown.name/
- https://lore.kernel.org/lkml/87h6475w9q.fsf@prevas.dk/
- https://lore.kernel.org/lkml/CAHk-=wjeZwww6Zswn6F_iZTpUihTSNKYppLqj36iQDDhfntuEw@mail.gmail.com/
Undoubtedly, there are more places in the code where this could also
be used but where -fms-extensions just didn't come up in any
discussion.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
Makefile | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Makefile b/Makefile
index d14824792227..ef6f23ee8e7f 100644
--- a/Makefile
+++ b/Makefile
@@ -1061,6 +1061,15 @@ NOSTDINC_FLAGS += -nostdinc
# perform bounds checking.
KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)
+# Allow including a tagged struct or union anonymously in another struct/union.
+KBUILD_CFLAGS += -fms-extensions
+
+# For clang, the -fms-extensions flag is apparently not enough to
+# express one's intention to make use of those extensions.
+ifdef CONFIG_CC_IS_CLANG
+KBUILD_CFLAGS += -Wno-microsoft-anon-tag
+endif
+
# disable invalid "can't wrap" optimizations for signed / pointers
KBUILD_CFLAGS += -fno-strict-overflow
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-20 14:22 ` [PATCH 1/2] Kbuild: enable -fms-extensions Rasmus Villemoes
@ 2025-10-22 16:15 ` Nathan Chancellor
2025-10-22 20:35 ` Rasmus Villemoes
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2025-10-22 16:15 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Linus Torvalds, David Sterba, linux-btrfs, Nick Desaulniers,
linux-kbuild
On Mon, Oct 20, 2025 at 04:22:27PM +0200, Rasmus Villemoes wrote:
> Once in a while, it turns out that enabling -fms-extensions could
> allow some slightly prettier code. But every time it has come up, the
> code that had to be used instead has been deemed "not too awful" and
> not worth introducing another compiler flag for.
>
> That's probably true for each individual case, but then it's somewhat
> of a chicken/egg situation.
>
> If we just "bite the bullet" as Linus says and enable it once and for
> all, it is available whenever a use case turns up, and no individual
> case has to justify it.
>
> A lore.kernel.org search provides these examples:
>
> - https://lore.kernel.org/lkml/200706301813.58435.agruen@suse.de/
> - https://lore.kernel.org/lkml/20180419152817.GD25406@bombadil.infradead.org/
> - https://lore.kernel.org/lkml/170622208395.21664.2510213291504081000@noble.neil.brown.name/
> - https://lore.kernel.org/lkml/87h6475w9q.fsf@prevas.dk/
> - https://lore.kernel.org/lkml/CAHk-=wjeZwww6Zswn6F_iZTpUihTSNKYppLqj36iQDDhfntuEw@mail.gmail.com/
>
> Undoubtedly, there are more places in the code where this could also
> be used but where -fms-extensions just didn't come up in any
> discussion.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> Makefile | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index d14824792227..ef6f23ee8e7f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1061,6 +1061,15 @@ NOSTDINC_FLAGS += -nostdinc
> # perform bounds checking.
> KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)
>
> +# Allow including a tagged struct or union anonymously in another struct/union.
> +KBUILD_CFLAGS += -fms-extensions
> +
> +# For clang, the -fms-extensions flag is apparently not enough to
> +# express one's intention to make use of those extensions.
> +ifdef CONFIG_CC_IS_CLANG
> +KBUILD_CFLAGS += -Wno-microsoft-anon-tag
> +endif
I think this should go in the first 'ifdef CONFIG_CC_IS_CLANG' block in
scripts/Makefile.extrawarn below '-Wno-gnu' with a comment that is
similar in nature, which could even be combined like
# The kernel builds with '-std-gnu11' and '-fms-extensions' so the use
# of GNU and Microsoft extensions is acceptable.
Other than that, this seems fine to me.
> # disable invalid "can't wrap" optimizations for signed / pointers
> KBUILD_CFLAGS += -fno-strict-overflow
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-22 16:15 ` Nathan Chancellor
@ 2025-10-22 20:35 ` Rasmus Villemoes
2025-10-22 21:11 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Rasmus Villemoes @ 2025-10-22 20:35 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Linus Torvalds, David Sterba, linux-btrfs, Nick Desaulniers,
linux-kbuild
On Wed, 22 Oct 2025 at 18:15, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Oct 20, 2025 at 04:22:27PM +0200, Rasmus Villemoes wrote:
> > +# Allow including a tagged struct or union anonymously in another struct/union.
> > +KBUILD_CFLAGS += -fms-extensions
> > +
> > +# For clang, the -fms-extensions flag is apparently not enough to
> > +# express one's intention to make use of those extensions.
> > +ifdef CONFIG_CC_IS_CLANG
> > +KBUILD_CFLAGS += -Wno-microsoft-anon-tag
> > +endif
>
> I think this should go in the first 'ifdef CONFIG_CC_IS_CLANG' block in
> scripts/Makefile.extrawarn below '-Wno-gnu' with a comment that is
> similar in nature, which could even be combined like
>
> # The kernel builds with '-std-gnu11' and '-fms-extensions' so the use
> # of GNU and Microsoft extensions is acceptable.
>
> Other than that, this seems fine to me.
I honestly had no idea where it was best to put these, and your
suggestion sounds quite reasonable. I didn't think to look in that
Makefile.extrawarn as the name suggested that was only about what
happens with W=1 and higher.
Feel free to tweak when applying.
Rasmus
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-22 20:35 ` Rasmus Villemoes
@ 2025-10-22 21:11 ` Nathan Chancellor
2025-10-23 12:40 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2025-10-22 21:11 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Linus Torvalds, David Sterba, linux-btrfs, Nick Desaulniers,
linux-kbuild
On Wed, Oct 22, 2025 at 10:35:33PM +0200, Rasmus Villemoes wrote:
> On Wed, 22 Oct 2025 at 18:15, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Mon, Oct 20, 2025 at 04:22:27PM +0200, Rasmus Villemoes wrote:
>
> > > +# Allow including a tagged struct or union anonymously in another struct/union.
> > > +KBUILD_CFLAGS += -fms-extensions
> > > +
> > > +# For clang, the -fms-extensions flag is apparently not enough to
> > > +# express one's intention to make use of those extensions.
> > > +ifdef CONFIG_CC_IS_CLANG
> > > +KBUILD_CFLAGS += -Wno-microsoft-anon-tag
> > > +endif
> >
> > I think this should go in the first 'ifdef CONFIG_CC_IS_CLANG' block in
> > scripts/Makefile.extrawarn below '-Wno-gnu' with a comment that is
> > similar in nature, which could even be combined like
> >
> > # The kernel builds with '-std-gnu11' and '-fms-extensions' so the use
> > # of GNU and Microsoft extensions is acceptable.
> >
> > Other than that, this seems fine to me.
>
> I honestly had no idea where it was best to put these, and your
> suggestion sounds quite reasonable. I didn't think to look in that
> Makefile.extrawarn as the name suggested that was only about what
> happens with W=1 and higher.
Yeah, we may want to rename that to just Makefile.warn since it
encompasses all warnings since commit e88ca24319e4 ("kbuild: consolidate
warning flags in scripts/Makefile.extrawarn"), which is actually
feedback I gave on the original change:
https://lore.kernel.org/20230811141943.GB3948268@dev-arch.thelio-3990X/
I'll send a patch for that soon.
> Feel free to tweak when applying.
I have tentatively applied this to kbuild-next so that it can spend most
of the cycle in -next to try and catch all potential problems.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-22 21:11 ` Nathan Chancellor
@ 2025-10-23 12:40 ` Nathan Chancellor
2025-10-23 14:17 ` Dave Kleikamp
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Chancellor @ 2025-10-23 12:40 UTC (permalink / raw)
To: Rasmus Villemoes, Linus Torvalds, Dave Kleikamp
Cc: David Sterba, linux-btrfs, Nick Desaulniers, linux-kbuild,
jfs-discussion
On Wed, Oct 22, 2025 at 11:11:38PM +0200, Nathan Chancellor wrote:
...
> > > > +# Allow including a tagged struct or union anonymously in another struct/union.
> > > > +KBUILD_CFLAGS += -fms-extensions
...
> I have tentatively applied this to kbuild-next so that it can spend most
> of the cycle in -next to try and catch all potential problems.
One side effect that has been found in my testing so far is clang's
'-fms-extensions' turns '_inline' into a keyword, which breaks fs/jfs:
In file included from fs/jfs/jfs_unicode.c:8:
fs/jfs/jfs_incore.h:86:13: error: type name does not allow function specifier to be specified
86 | unchar _inline[128];
| ^
fs/jfs/jfs_incore.h:86:20: error: expected member name or ';' after declaration specifiers
86 | unchar _inline[128];
| ~~~~~~~~~~~~~~^
There appear to be other similar keywords (ones with KEYMS in the linke
below) but my personal distribution configuration does not show any
instances in the build where they matter (I did not test allmodconfig
yet).
https://github.com/llvm/llvm-project/blob/249883d0c5883996bed038cd82a8999f342994c9/clang/include/clang/Basic/TokenKinds.def#L744-L794
Something like this is all it takes to resolve the issue, so I will send
a patch for formal review/acking but I wanted to bring it up ahead of
time in case this is unpalpable and we should throw these changes out of
-next instead of forward fixing.
Cheers,
Nathan
diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
index 10934f9a11be..5aaafedb8fbc 100644
--- a/fs/jfs/jfs_incore.h
+++ b/fs/jfs/jfs_incore.h
@@ -76,14 +76,14 @@ struct jfs_inode_info {
struct {
unchar _unused[16]; /* 16: */
dxd_t _dxd; /* 16: */
- /* _inline may overflow into _inline_ea when needed */
+ /* _inline_sym may overflow into _inline_ea when needed */
/* _inline_ea may overlay the last part of
* file._xtroot if maxentry = XTROOTINITSLOT
*/
union {
struct {
/* 128: inline symlink */
- unchar _inline[128];
+ unchar _inline_sym[128];
/* 128: inline extended attr */
unchar _inline_ea[128];
};
@@ -101,7 +101,7 @@ struct jfs_inode_info {
#define i_imap u.file._imap
#define i_dirtable u.dir._table
#define i_dtroot u.dir._dtroot
-#define i_inline u.link._inline
+#define i_inline u.link._inline_sym
#define i_inline_ea u.link._inline_ea
#define i_inline_all u.link._inline_all
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-23 12:40 ` Nathan Chancellor
@ 2025-10-23 14:17 ` Dave Kleikamp
2025-10-23 16:45 ` Nathan Chancellor
0 siblings, 1 reply; 13+ messages in thread
From: Dave Kleikamp @ 2025-10-23 14:17 UTC (permalink / raw)
To: Nathan Chancellor, Rasmus Villemoes, Linus Torvalds
Cc: David Sterba, linux-btrfs, Nick Desaulniers, linux-kbuild,
jfs-discussion
On 10/23/25 7:40AM, Nathan Chancellor wrote:
> On Wed, Oct 22, 2025 at 11:11:38PM +0200, Nathan Chancellor wrote:
> ...
>>>>> +# Allow including a tagged struct or union anonymously in another struct/union.
>>>>> +KBUILD_CFLAGS += -fms-extensions
> ...
>> I have tentatively applied this to kbuild-next so that it can spend most
>> of the cycle in -next to try and catch all potential problems.
>
> One side effect that has been found in my testing so far is clang's
> '-fms-extensions' turns '_inline' into a keyword, which breaks fs/jfs:
>
> In file included from fs/jfs/jfs_unicode.c:8:
> fs/jfs/jfs_incore.h:86:13: error: type name does not allow function specifier to be specified
> 86 | unchar _inline[128];
> | ^
> fs/jfs/jfs_incore.h:86:20: error: expected member name or ';' after declaration specifiers
> 86 | unchar _inline[128];
> | ~~~~~~~~~~~~~~^
>
> There appear to be other similar keywords (ones with KEYMS in the linke
> below) but my personal distribution configuration does not show any
> instances in the build where they matter (I did not test allmodconfig
> yet).
>
> https://github.com/llvm/llvm-project/blob/249883d0c5883996bed038cd82a8999f342994c9/clang/include/clang/Basic/TokenKinds.def#L744-L794
>
> Something like this is all it takes to resolve the issue, so I will send
> a patch for formal review/acking but I wanted to bring it up ahead of
> time in case this is unpalpable and we should throw these changes out of
> -next instead of forward fixing.
I'm on vacation now, so I may be slow to respond to a future patch, so
I'll go ahead and give you my ack to this.
Acked-by: Dave Kleikamp <dave.kleikamp@oracle.com>
>
> Cheers,
> Nathan
>
> diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
> index 10934f9a11be..5aaafedb8fbc 100644
> --- a/fs/jfs/jfs_incore.h
> +++ b/fs/jfs/jfs_incore.h
> @@ -76,14 +76,14 @@ struct jfs_inode_info {
> struct {
> unchar _unused[16]; /* 16: */
> dxd_t _dxd; /* 16: */
> - /* _inline may overflow into _inline_ea when needed */
> + /* _inline_sym may overflow into _inline_ea when needed */
> /* _inline_ea may overlay the last part of
> * file._xtroot if maxentry = XTROOTINITSLOT
> */
> union {
> struct {
> /* 128: inline symlink */
> - unchar _inline[128];
> + unchar _inline_sym[128];
> /* 128: inline extended attr */
> unchar _inline_ea[128];
> };
> @@ -101,7 +101,7 @@ struct jfs_inode_info {
> #define i_imap u.file._imap
> #define i_dirtable u.dir._table
> #define i_dtroot u.dir._dtroot
> -#define i_inline u.link._inline
> +#define i_inline u.link._inline_sym
> #define i_inline_ea u.link._inline_ea
> #define i_inline_all u.link._inline_all
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/2] Kbuild: enable -fms-extensions
2025-10-23 14:17 ` Dave Kleikamp
@ 2025-10-23 16:45 ` Nathan Chancellor
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2025-10-23 16:45 UTC (permalink / raw)
To: Dave Kleikamp
Cc: Rasmus Villemoes, Linus Torvalds, David Sterba, linux-btrfs,
Nick Desaulniers, linux-kbuild, jfs-discussion
On Thu, Oct 23, 2025 at 09:17:47AM -0500, Dave Kleikamp wrote:
> On 10/23/25 7:40AM, Nathan Chancellor wrote:
> > Something like this is all it takes to resolve the issue, so I will send
> > a patch for formal review/acking but I wanted to bring it up ahead of
> > time in case this is unpalpable and we should throw these changes out of
> > -next instead of forward fixing.
>
> I'm on vacation now, so I may be slow to respond to a future patch, so I'll
> go ahead and give you my ack to this.
>
> Acked-by: Dave Kleikamp <dave.kleikamp@oracle.com>
Great, thanks for the tag and taking a look quickly!
> > diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
> > index 10934f9a11be..5aaafedb8fbc 100644
> > --- a/fs/jfs/jfs_incore.h
> > +++ b/fs/jfs/jfs_incore.h
> > @@ -76,14 +76,14 @@ struct jfs_inode_info {
> > struct {
> > unchar _unused[16]; /* 16: */
> > dxd_t _dxd; /* 16: */
> > - /* _inline may overflow into _inline_ea when needed */
> > + /* _inline_sym may overflow into _inline_ea when needed */
> > /* _inline_ea may overlay the last part of
> > * file._xtroot if maxentry = XTROOTINITSLOT
> > */
> > union {
> > struct {
> > /* 128: inline symlink */
> > - unchar _inline[128];
> > + unchar _inline_sym[128];
> > /* 128: inline extended attr */
> > unchar _inline_ea[128];
> > };
> > @@ -101,7 +101,7 @@ struct jfs_inode_info {
> > #define i_imap u.file._imap
> > #define i_dirtable u.dir._table
> > #define i_dtroot u.dir._dtroot
> > -#define i_inline u.link._inline
> > +#define i_inline u.link._inline_sym
> > #define i_inline_ea u.link._inline_ea
> > #define i_inline_all u.link._inline_all
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path
2025-10-20 14:22 [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user Rasmus Villemoes
2025-10-20 14:22 ` [PATCH 1/2] Kbuild: enable -fms-extensions Rasmus Villemoes
@ 2025-10-20 14:22 ` Rasmus Villemoes
2025-10-20 19:48 ` Linus Torvalds
2025-10-22 5:30 ` [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user David Sterba
2 siblings, 1 reply; 13+ messages in thread
From: Rasmus Villemoes @ 2025-10-20 14:22 UTC (permalink / raw)
To: Linus Torvalds, David Sterba
Cc: linux-btrfs, Nathan Chancellor, Nick Desaulniers, linux-kbuild,
Rasmus Villemoes
The newly introduced -fms-extensions compiler flag allows defining
struct fs_path in such a way that inline_buf becomes a proper array
with a size known to the compiler.
This also makes the problem fixed by 8aec9dbf2db2 ("btrfs: send: fix
-Wflex-array-member-not-at-end warning in struct send_ctx") go
away. Whether cur_inode_path should be put back to its original place
in struct send_ctx I don't know, but at least the comment no longer
applies.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
fs/btrfs/send.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 6144e66661f5..1fe4a06e6850 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -47,28 +47,30 @@
* It allows fast adding of path elements on the right side (normal path) and
* fast adding to the left side (reversed path). A reversed path can also be
* unreversed if needed.
+ *
+ * The definition of struct fs_path relies on -fms-extensions to allow
+ * including a tagged struct as an anonymous member.
*/
+struct __fs_path {
+ char *start;
+ char *end;
+
+ char *buf;
+ unsigned short buf_len:15;
+ unsigned short reversed:1;
+};
+static_assert(sizeof(struct __fs_path) < 256);
struct fs_path {
- union {
- struct {
- char *start;
- char *end;
-
- char *buf;
- unsigned short buf_len:15;
- unsigned short reversed:1;
- char inline_buf[];
- };
- /*
- * Average path length does not exceed 200 bytes, we'll have
- * better packing in the slab and higher chance to satisfy
- * an allocation later during send.
- */
- char pad[256];
- };
+ struct __fs_path;
+ /*
+ * Average path length does not exceed 200 bytes, we'll have
+ * better packing in the slab and higher chance to satisfy
+ * an allocation later during send.
+ */
+ char inline_buf[256 - sizeof(struct __fs_path)];
};
#define FS_PATH_INLINE_SIZE \
- (sizeof(struct fs_path) - offsetof(struct fs_path, inline_buf))
+ sizeof_field(struct fs_path, inline_buf)
/* reused for each extent */
@@ -305,7 +307,6 @@ struct send_ctx {
struct btrfs_lru_cache dir_created_cache;
struct btrfs_lru_cache dir_utimes_cache;
- /* Must be last as it ends in a flexible-array member. */
struct fs_path cur_inode_path;
};
--
2.51.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path
2025-10-20 14:22 ` [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path Rasmus Villemoes
@ 2025-10-20 19:48 ` Linus Torvalds
2025-10-22 5:24 ` David Sterba
0 siblings, 1 reply; 13+ messages in thread
From: Linus Torvalds @ 2025-10-20 19:48 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: David Sterba, linux-btrfs, Nathan Chancellor, Nick Desaulniers,
linux-kbuild
On Mon, 20 Oct 2025 at 04:22, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> +struct __fs_path {
> + char *start;
> + char *end;
> +
> + char *buf;
> + unsigned short buf_len:15;
> + unsigned short reversed:1;
> +};
> +static_assert(sizeof(struct __fs_path) < 256);
> struct fs_path {
> + struct __fs_path;
> + /*
> + * Average path length does not exceed 200 bytes, we'll have
> + * better packing in the slab and higher chance to satisfy
> + * an allocation later during send.
> + */
> + char inline_buf[256 - sizeof(struct __fs_path)];
> };
It strikes me that this won't pack as well as it used to before the change.
On 64-bit architectrures, 'struct __fs_path' will be 8-byte aligned
due to the pointers in it, and that means that the size of it will
also be aligned: it will be 32 bytes in size.
So you'll get 256-32 bytes of inline_buf.
And it *used* to be that 'inline_buf[]' was packed righ after the
16-bit buf_len / reversed bits, so it used to get an extra six bytes.
I think it could be fixed with a "__packed" thing on that inner
struct, but that also worries me a bit because we'd certainly never
want the compiler to generate the code for unaligned accesses (on the
broken architectures that would do that). You'd then have to mark the
containing structure as being aligned to make compilers generate good
code.
So either you lose some inline buffer space, or you end up having to
add extra packing stuff. Either way is a bit of a bother.
Linus
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path
2025-10-20 19:48 ` Linus Torvalds
@ 2025-10-22 5:24 ` David Sterba
0 siblings, 0 replies; 13+ messages in thread
From: David Sterba @ 2025-10-22 5:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: Rasmus Villemoes, David Sterba, linux-btrfs, Nathan Chancellor,
Nick Desaulniers, linux-kbuild
On Mon, Oct 20, 2025 at 09:48:25AM -1000, Linus Torvalds wrote:
> On Mon, 20 Oct 2025 at 04:22, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >
> > +struct __fs_path {
> > + char *start;
> > + char *end;
> > +
> > + char *buf;
> > + unsigned short buf_len:15;
> > + unsigned short reversed:1;
> > +};
> > +static_assert(sizeof(struct __fs_path) < 256);
> > struct fs_path {
> > + struct __fs_path;
> > + /*
> > + * Average path length does not exceed 200 bytes, we'll have
> > + * better packing in the slab and higher chance to satisfy
> > + * an allocation later during send.
> > + */
> > + char inline_buf[256 - sizeof(struct __fs_path)];
> > };
>
> It strikes me that this won't pack as well as it used to before the change.
>
> On 64-bit architectrures, 'struct __fs_path' will be 8-byte aligned
> due to the pointers in it, and that means that the size of it will
> also be aligned: it will be 32 bytes in size.
>
> So you'll get 256-32 bytes of inline_buf.
>
> And it *used* to be that 'inline_buf[]' was packed righ after the
> 16-bit buf_len / reversed bits, so it used to get an extra six bytes.
>
> I think it could be fixed with a "__packed" thing on that inner
> struct, but that also worries me a bit because we'd certainly never
> want the compiler to generate the code for unaligned accesses (on the
> broken architectures that would do that). You'd then have to mark the
> containing structure as being aligned to make compilers generate good
> code.
>
> So either you lose some inline buffer space, or you end up having to
> add extra packing stuff. Either way is a bit of a bother.
For the inline path buffer losing 6 bytes is acceptable, I did some
stats on my system and full paths under /.snapshots from snapper are all
uner 200 bytes, lengths on another random data partition goes up to 380.
Users can of course have path of any length but there's a fallback and
the allocations are done in the steps of slab bucket sizes so it's
reasonably effective.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user
2025-10-20 14:22 [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user Rasmus Villemoes
2025-10-20 14:22 ` [PATCH 1/2] Kbuild: enable -fms-extensions Rasmus Villemoes
2025-10-20 14:22 ` [PATCH 2/2] btrfs: send: make use of -fms-extensions for defining struct fs_path Rasmus Villemoes
@ 2025-10-22 5:30 ` David Sterba
2025-10-22 16:17 ` Nathan Chancellor
2 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2025-10-22 5:30 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Linus Torvalds, David Sterba, linux-btrfs, Nathan Chancellor,
Nick Desaulniers, linux-kbuild
On Mon, Oct 20, 2025 at 04:22:26PM +0200, Rasmus Villemoes wrote:
> Since -fms-extensions once again came up as potentially useful, Linus
> suggested that we bite the bullet and enable it.
>
> https://lore.kernel.org/lkml/CAHk-=wjeZwww6Zswn6F_iZTpUihTSNKYppLqj36iQDDhfntuEw@mail.gmail.com/
>
> So that's what patch 1 does, and patch 2 puts it to use in the btrfs
> case.
>
> Compile-tested only, with gcc (15.2.1) and clang (20.1.8).
>
> Rasmus Villemoes (2):
> Kbuild: enable -fms-extensions
> btrfs: send: make use of -fms-extensions for defining struct fs_path
For the btrfs part
Acked-by: David Sterba <dsterba@suse.com>
I think it makes more sense to take the patches via the kbuild tree so
it's in linux-next for build coverage and eventual tweaks to the Kbuild
files. Or I can take the patches into btrfs for-next.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user
2025-10-22 5:30 ` [PATCH 0/2] Kbuild: enable -fms-extensions, make btrfs the first user David Sterba
@ 2025-10-22 16:17 ` Nathan Chancellor
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2025-10-22 16:17 UTC (permalink / raw)
To: David Sterba
Cc: Rasmus Villemoes, Linus Torvalds, David Sterba, linux-btrfs,
Nick Desaulniers, linux-kbuild, Nicolas Schier
+ Nicolas for kbuild
On Wed, Oct 22, 2025 at 07:30:42AM +0200, David Sterba wrote:
> On Mon, Oct 20, 2025 at 04:22:26PM +0200, Rasmus Villemoes wrote:
> > Since -fms-extensions once again came up as potentially useful, Linus
> > suggested that we bite the bullet and enable it.
> >
> > https://lore.kernel.org/lkml/CAHk-=wjeZwww6Zswn6F_iZTpUihTSNKYppLqj36iQDDhfntuEw@mail.gmail.com/
> >
> > So that's what patch 1 does, and patch 2 puts it to use in the btrfs
> > case.
> >
> > Compile-tested only, with gcc (15.2.1) and clang (20.1.8).
> >
> > Rasmus Villemoes (2):
> > Kbuild: enable -fms-extensions
> > btrfs: send: make use of -fms-extensions for defining struct fs_path
>
> For the btrfs part
>
> Acked-by: David Sterba <dsterba@suse.com>
>
> I think it makes more sense to take the patches via the kbuild tree so
> it's in linux-next for build coverage and eventual tweaks to the Kbuild
> files. Or I can take the patches into btrfs for-next.
Yeah, we should be able to take these into Kbuild. I had just one small
nit on patch 1 that we could probably fix up at application time unless
folks disagree with it. I hope there will be no follow up fixes needed
but it would make more sense in our tree than yours.
Cheers,
Nathan
^ permalink raw reply [flat|nested] 13+ messages in thread