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