On Thu, Nov 10, 2022, at 11:20, Zhiguo Niu wrote:
> Arnd Bergmann <a...@arndb.de> 于2022年11月10日周四 17:07写道：
>> On Thu, Nov 10, 2022, at 09:33, kernel test robot wrote:
>> > base:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git
>> > dev-test
>> > patch link:
>> > https://lore.kernel.org/r/1667889638-9106-1-git-send-email-zhiguo.niu%40unisoc.com
>> > patch subject: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform
>> > All warnings (new ones prefixed by >>):
>> >
>> > In file included from fs/f2fs/gc.c:22:
>> >>> fs/f2fs/gc.h:65:2: warning: field within 'struct victim_entry' is less
>> >>> aligned than 'union victim_entry::(anonymous at fs/f2fs/gc.h:65:2)' and
>> >>> is usually due to 'struct victim_entry' being packed, which can lead to
>> >>> unaligned accesses [-Wunaligned-access]
>> > union {
>>
>> It looks like the problem is the extra unqualified __packed annotation
>> inside of 'struct rb_entry'.
> yes, I agree, but this modification is about the following commit:
> f2fs: fix memory alignment to support 32bit
> (48046cb55d208eae67259887b29b3097bcf44caf）
Ah, I see. So in this case, the line
en = (struct extent_node *)f2fs_lookup_rb_tree_ret(&et->root,
requires the second field of 'struct extent_node' to be
in the same place as the corresponding field of 'struct rb_entry'.
This seems harmless then, though I would have put the __packed
annotation on the 'key' member instead of the union to
better document what is going on. Ideally the casts between
structures should not be used at all, but I don't know if
changing f2fs for this would involve a major rewrite of that
code.
> so I think is the following modifiction more better?
>
> @@ -68,7 +68,7 @@ struct victim_entry {
>
> unsigned int segno; /* segment No. */
>
> };
>
> struct victim_info vi; /* victim info */
>
> - };
>
> + } __packed;
So here is the construct with
ve = (struct victim_entry *)re;
that relies on vi->mtime to overlay re->key, right?
I'm not sure why there is a union in victim_entry, it would
be a little easier without that. Clearly both sides
of the union need the same alignment constraints, so
you could annotate the two 'mtime' members as __packed,
which gives the anonymous struct and the struct victim_info
32-bit alignment and avoids the warning. Having the
__packed at the end of the structure or union would
result in only single-byte alignment for structure
and not solve the problem that the compiler warns about.
The other alternative is to revert rb_entry back to
having 64-bit alignment on the key, but then also mark
extent_node as requiring the same alignment on the
'extent_info' member for consistency:
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -580,11 +580,11 @@ struct rb_entry {
unsigned int len; /* length of the entry */
};
unsigned long long key; /* 64-bits key */
- } __packed;
+ };
};
struct extent_info {
- unsigned int fofs; /* start offset in a file */
+ unsigned int fofs __aligned(8); /* start offset in a file */
unsigned int len; /* length of the extent */
u32 blk; /* start block address of the extent */
#ifdef CONFIG_F2FS_FS_COMPRESSION
Arnd
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
