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

[Gao Xiang]

Hi David,

On 2025/4/10 02:52, David Laight wrote:
> On Tue,  8 Apr 2025 19:44:47 +0800
> Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> 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.

> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Closes: https://lore.kernel.org/r/202504051202.DS7QIknJ-lkp@xxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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__)
      |

> 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);

?


./../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.

Thanks,
Gao Xiang