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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

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.

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

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