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

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

[Gao Xiang]

I'm unsure why they aren't 2 bytes in size only in arm-linux-gnueabi.

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;
 
 /* 32-byte reduced form of an ondisk inode */
 struct erofs_inode_compact {
@@ -369,9 +369,9 @@ struct z_erofs_map_header {
 			 * bit 7   : pack the whole file into packed inode
 			 */
 			__u8	h_clusterbits;
-		};
+		} __packed;
 		__le16 h_extents_hi;	/* extent count MSB */
-	};
+	} __packed;
 };
 
 enum {
-- 
2.43.5

[PATCH 2/2] erofs: fix encoded extents handling

[Gao Xiang]

 - The MSB 32 bits of `z_fragmentoff` are available only in extent
records of size >= 8B.

 - Use round_down() to calculate `lstart` as well as increase `pos`
   correspondingly for extent records of size == 8B.

Fixes: 1d191b4ca51d ("erofs: implement encoded extent metadata")
Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
---
 fs/erofs/zmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/erofs/zmap.c b/fs/erofs/zmap.c
index 8de50df05dfe..14ea47f954f5 100644
--- a/fs/erofs/zmap.c
+++ b/fs/erofs/zmap.c
@@ -559,7 +559,8 @@ static int z_erofs_map_blocks_ext(struct inode *inode,
 			pos += sizeof(__le64);
 			lstart = 0;
 		} else {
-			lstart = map->m_la >> vi->z_lclusterbits;
+			lstart = round_down(map->m_la, 1 << vi->z_lclusterbits);
+			pos += (lstart >> vi->z_lclusterbits) * recsz;
 			pa = EROFS_NULL_ADDR;
 		}
 
@@ -614,7 +615,7 @@ static int z_erofs_map_blocks_ext(struct inode *inode,
 		if (last && (vi->z_advise & Z_EROFS_ADVISE_FRAGMENT_PCLUSTER)) {
 			map->m_flags |= EROFS_MAP_MAPPED | EROFS_MAP_FRAGMENT;
 			vi->z_fragmentoff = map->m_plen;
-			if (recsz >= offsetof(struct z_erofs_extent, pstart_lo))
+			if (recsz > offsetof(struct z_erofs_extent, pstart_lo))
 				vi->z_fragmentoff |= map->m_pa << 32;
 		} else if (map->m_plen) {
 			map->m_flags |= EROFS_MAP_MAPPED |
-- 
2.43.5

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

[David Laight]

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.

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

I'd add a compile assert (of some form) on the size of the structure.

	David

>  
>  /* 32-byte reduced form of an ondisk inode */
>  struct erofs_inode_compact {
> @@ -369,9 +369,9 @@ struct z_erofs_map_header {
>  			 * bit 7   : pack the whole file into packed inode
>  			 */
>  			__u8	h_clusterbits;
> -		};
> +		} __packed;
>  		__le16 h_extents_hi;	/* extent count MSB */
> -	};
> +	} __packed;

Ditto

>  };
>  
>  enum {

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

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

[David Laight]

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

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

[Gao Xiang]

On 2025/4/11 04:53, David Laight wrote:
> 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!

Sigh, I'm not sure, it's really a mess but I don't have
more time to look into that.

> 
>>
>>>    
>>>>


..

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

Currently it works fine with `__packed` on the union definition,

do you suggest adding both `__packed` and `__aligned(2)`, may
I ask what's the benefit of `__aligned(2)`?

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

.. sigh.. I have no more interest on this now due to lack
of time (my current employer doesn't allow me), I think
if there is no better ideas, let's keep the original patch
way to resolve arm compile issues...

Thanks,
Gao Xiang