On Thu, 10 Apr 2025 07:56:45 +0800 Gao Xiang <hsiang...@linux.alibaba.com> wrote:
> Hi David, > > On 2025/4/10 02:52, David Laight wrote: > > On Tue, 8 Apr 2025 19:44:47 +0800 > > Gao Xiang <hsiang...@linux.alibaba.com> wrote: > > > >> I'm unsure why they aren't 2 bytes in size only in arm-linux-gnueabi. > > > > IIRC one of the arm ABI aligns structures on 32 bit boundaries. > > Thanks for your reply, but I'm not sure if it's the issue. Twas a guess, the fragment in the patch doesn't look as though it will add padding. All tests I've tried generate a 2 byte union. But there might be something odd about the definition of __le16. Or the compiler is actually broken! > > > > >> > >> Reported-by: kernel test robot <l...@intel.com> > >> Closes: https://lore.kernel.org/r/202504051202.ds7qiknj-...@intel.com > >> Fixes: 61ba89b57905 ("erofs: add 48-bit block addressing on-disk support") > >> Fixes: efb2aef569b3 ("erofs: add encoded extent on-disk definition") > >> Signed-off-by: Gao Xiang <hsiang...@linux.alibaba.com> > >> --- > >> fs/erofs/erofs_fs.h | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > >> index 61a5ee11f187..94bf636776b0 100644 > >> --- a/fs/erofs/erofs_fs.h > >> +++ b/fs/erofs/erofs_fs.h > >> @@ -56,7 +56,7 @@ struct erofs_super_block { > >> union { > >> __le16 rootnid_2b; /* nid of root directory */ > >> __le16 blocks_hi; /* (48BIT on) blocks count MSB */ > >> - } rb; > >> + } __packed rb; > >> __le64 inos; /* total valid ino # (== f_files - f_favail) */ > >> __le64 epoch; /* base seconds used for compact inodes */ > >> __le32 fixed_nsec; /* fixed nanoseconds for compact inodes */ > >> @@ -148,7 +148,7 @@ union erofs_inode_i_nb { > >> __le16 nlink; /* if EROFS_I_NLINK_1_BIT is unset */ > >> __le16 blocks_hi; /* total blocks count MSB */ > >> __le16 startblk_hi; /* starting block number MSB */ > >> -}; > >> +} __packed; > > > > That shouldn't be necessary and will kill performance on some systems. > > The 'packed' on the member should be enough to reduce the size. > > It cannot be resolved by the following diff: > > diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h > index 94bf636776b0..1f6233dfdcb0 100644 > --- a/fs/erofs/erofs_fs.h > +++ b/fs/erofs/erofs_fs.h > @@ -148,14 +148,14 @@ union erofs_inode_i_nb { > __le16 nlink; /* if EROFS_I_NLINK_1_BIT is unset */ > __le16 blocks_hi; /* total blocks count MSB */ > __le16 startblk_hi; /* starting block number MSB */ > -} __packed; > +}; > > /* 32-byte reduced form of an ondisk inode */ > struct erofs_inode_compact { > __le16 i_format; /* inode format hints */ > __le16 i_xattr_icount; > __le16 i_mode; > - union erofs_inode_i_nb i_nb; > + union erofs_inode_i_nb i_nb __packed; > __le32 i_size; > __le32 i_mtime; > union erofs_inode_i_u i_u; > @@ -171,7 +171,7 @@ struct erofs_inode_extended { > __le16 i_format; /* inode format hints */ > __le16 i_xattr_icount; > __le16 i_mode; > - union erofs_inode_i_nb i_nb; > + union erofs_inode_i_nb i_nb __packed; > __le64 i_size; > union erofs_inode_i_u i_u; > > I doesn't work and will report > > In file included from <command-line>: > In function 'erofs_check_ondisk_layout_definitions', > inlined from 'erofs_module_init' at ../fs/erofs/super.c:817:2: > ./../include/linux/compiler_types.h:542:38: error: call to > '__compiletime_assert_332' declared with attribute error: BUILD_BUG_ON > failed: sizeof(struct erofs_inode_compact) != 32 > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | Try with just __packed __aligned(2) on the union definition. That should overcome whatever brain-damage is causing the larger alignment, > > > > > I'd add a compile assert (of some form) on the size of the structure. > > you mean > > @@ -435,6 +435,7 @@ static inline void > erofs_check_ondisk_layout_definitions(void) > }; > > BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128); > + BUILD_BUG_ON(sizeof(union erofs_inode_i_nb) != 2); > BUILD_BUG_ON(sizeof(struct erofs_inode_compact) != 32); I'm sure there is one that you can put in the .h file itself. Might have to be Static_assert(). > > ? > > > ./../include/linux/compiler_types.h:542:38: error: call to > '__compiletime_assert_332' declared with attribute error: BUILD_BUG_ON > failed: sizeof(union erofs_inode_i_nb) != 2 > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | ^ > > That doesn't work too. That it the root of the problem. I'd check with just a 'short' rather than the __le16. David > > Thanks, > Gao Xiang
