Re: [PATCH 1/2] erofs: add __packed annotation to union(__le16..)

[David Laight <david.laight.linux@gmail.com>]

On Thu, 10 Apr 2025 07:56:45 +0800
Gao Xiang <hsiangkao@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 <hsiangkao@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 <lkp@intel.com>
> >> Closes: https://lore.kernel.org/r/202504051202.DS7QIknJ-lkp@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 <hsiangkao@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