Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1115-g8b801eadce-fm-20221102.001-g8b801ead
Mime-Version: 1.0
Message-Id: <97c529f2-c5a2-4ed6-89eb-c79f020e9d0c@app.fastmail.com>
In-Reply-To: <CAHJ8P3+g7dr3PBZFQCD5HQLZ2b0WHe=b6Jt7ha1o7mqZJ7_-BQ@mail.gmail.com>
References: <1667889638-9106-1-git-send-email-zhiguo.niu@unisoc.com>
 <202211101659.j9z9jLHv-lkp@intel.com>
 <88fce16b-5092-4246-8bbf-23f2c03224f3@app.fastmail.com>
 <CAHJ8P3+g7dr3PBZFQCD5HQLZ2b0WHe=b6Jt7ha1o7mqZJ7_-BQ@mail.gmail.com>
Date:   Thu, 10 Nov 2022 14:45:31 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Zhiguo Niu" <niuzhiguo84@gmail.com>
Cc:     "kernel test robot" <lkp@intel.com>,
        "zhiguo.niu" <zhiguo.niu@unisoc.com>, jaegeuk@kernel.org,
        chao@kernel.org, linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
        oe-kbuild-all@lists.linux.dev
Subject: Re: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 10, 2022, at 11:20, Zhiguo Niu wrote:
> Arnd Bergmann <arnd@arndb.de> =E4=BA=8E2022=E5=B9=B411=E6=9C=8810=E6=97=
=A5=E5=91=A8=E5=9B=9B 17:07=E5=86=99=E9=81=93=EF=BC=9A
>> On Thu, Nov 10, 2022, at 09:33, kernel test robot wrote:
>> > base:  =20
>> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git=20
>> > dev-test
>> > patch link:   =20
>> > https://lore.kernel.org/r/1667889638-9106-1-git-send-email-zhiguo.n=
iu%40unisoc.com
>> > patch subject: [PATCH V2] f2fs: fix atgc bug on issue in 32bits pla=
tform
>> > 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' i=
s 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 l=
ead to unaligned accesses [-Wunaligned-access]
>> >            union {
>>=20
>> It looks like the problem is the extra unqualified __packed annotation
>> inside of 'struct rb_entry'.=20
> yes, I agree, but this modification is about the following commit:
> f2fs: fix memory alignment to support 32bit=20
> (48046cb55d208eae67259887b29b3097bcf44caf=EF=BC=89

Ah, I see. So in this case, the line

        en =3D (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?=20
>
> @@ -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 =3D (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=20
__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;
+       };
 };
=20
 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 ex=
tent */
 #ifdef CONFIG_F2FS_FS_COMPRESSION

      Arnd