Date:   Thu, 10 Nov 2022 14:45:31 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Zhiguo Niu" <niuzhiguo84@...il.com>
Cc:     "kernel test robot" <lkp@...el.com>,
        "zhiguo.niu" <zhiguo.niu@...soc.com>, jaegeuk@...nel.org,
        chao@...nel.org, linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        oe-kbuild-all@...ts.linux.dev
Subject: Re: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform

On Thu, Nov 10, 2022, at 11:20, Zhiguo Niu wrote:
> Arnd Bergmann <arnd@...db.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

Powered by blists - more mailing lists