Message ID | 1667889638-9106-1-git-send-email-zhiguo.niu@unisoc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2526536wru; Mon, 7 Nov 2022 22:50:19 -0800 (PST) X-Google-Smtp-Source: AMsMyM6X72h6Aubv2KIxXZ0zVgrwVVRdy9xnBE6cZiG7PvSeop5YSP/X3n1sQXEKHK67Ld8JshDw X-Received: by 2002:a17:902:e891:b0:186:c544:8b1e with SMTP id w17-20020a170902e89100b00186c5448b1emr53726578plg.163.1667890219042; Mon, 07 Nov 2022 22:50:19 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667890219; cv=none; d=google.com; s=arc-20160816; b=t4tn2vgI2e4AOUiq1URTD+19sRLqN+2bsqb1f7yldGmKZYMdfIF/Nw2RCza7KIVU5a JO7uyRM8h3dkjw7l3WktOwgaJxKDhmNVYyGC405Ehw6tcvKE2sApjAgnNb5KX+8W8O9/ hSZ78zLG7Fz/m+J/R6/glZvCecoU860HDBiaOCNifpcUXlmYwv9qGWsO87/CaLaE+66X DxnllEe60HXTeOozuGWcpZ5AO/3dbxpwNPUhAnyu9Icy2VxjdwdZMQ8n0KXNXKYLBJ4X b054nk52wQzu3hpPMm1pSnqxa5FAFkfNKRIK6mic1othoKypPzNTBIiKQMd4vDyp6/jM alZA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from; bh=xej/uiqvvb0hcKS+hjJM61rEgXeY8fzcIZnGcVhjrks=; b=Jz3Ldv2gsYVCsS1JEy6OJXdqh5KRCJvKD93Y+H7XuDQ8sIXB3DKQ9A7EagQDmIJI6j /E0wDuMmrkqiDw+tuIEtAV3dIds8dTRsl+xOR3G6HTe4XmH4JREOvpc6a8umr8FXogwA qyQBAxrEZhMnUgTqq94m2XSbrSFI+6R7X/uGIpuCYgAiv3h7J/F4K57QmzXf+mDHORWm MYVgYUOlohg2LMk+HQdj7REkMWSKMkpoE+yYPe62hMnJAdrr3mWFgYawiXnL7Jt1v60D i4pliKpEzFTLAK2NuxbB1PZfPkxCDJnkLrorZ7hY0ThWBPYCBwKa2Vxd7L3xVhNlpDmx YH4w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id z9-20020a170903018900b00186f1a1fb83si13430281plg.352.2022.11.07.22.50.06; Mon, 07 Nov 2022 22:50:19 -0800 (PST) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233240AbiKHGly (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others); Tue, 8 Nov 2022 01:41:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46342 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232798AbiKHGlx (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 8 Nov 2022 01:41:53 -0500 Received: from SHSQR01.spreadtrum.com (mx1.unisoc.com [222.66.158.135]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 502101EAD2 for <linux-kernel@vger.kernel.org>; Mon, 7 Nov 2022 22:41:50 -0800 (PST) Received: from SHSend.spreadtrum.com (bjmbx02.spreadtrum.com [10.0.64.8]) by SHSQR01.spreadtrum.com with ESMTPS id 2A86erVb051782 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NO); Tue, 8 Nov 2022 14:40:53 +0800 (CST) (envelope-from Zhiguo.Niu@unisoc.com) Received: from bj08434pcu.spreadtrum.com (10.0.74.109) by BJMBX02.spreadtrum.com (10.0.64.8) with Microsoft SMTP Server (TLS) id 15.0.1497.23; Tue, 8 Nov 2022 14:40:53 +0800 From: "zhiguo.niu" <zhiguo.niu@unisoc.com> To: <jaegeuk@kernel.org>, <chao@kernel.org>, <linux-f2fs-devel@lists.sourceforge.net>, <linux-kernel@vger.kernel.org> CC: <niuzhiguo84@gmail.com>, <zhiguo.niu@unisoc.com> Subject: [PATCH V2] f2fs: fix atgc bug on issue in 32bits platform Date: Tue, 8 Nov 2022 14:40:38 +0800 Message-ID: <1667889638-9106-1-git-send-email-zhiguo.niu@unisoc.com> X-Mailer: git-send-email 1.9.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.0.74.109] X-ClientProxiedBy: SHCAS03.spreadtrum.com (10.0.1.207) To BJMBX02.spreadtrum.com (10.0.64.8) X-MAIL: SHSQR01.spreadtrum.com 2A86erVb051782 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748909654701386245?= X-GMAIL-MSGID: =?utf-8?q?1748909654701386245?= |
Series |
[V2] f2fs: fix atgc bug on issue in 32bits platform
|
|
Commit Message
Zhiguo Niu
Nov. 8, 2022, 6:40 a.m. UTC
From: Zhiguo Niu <zhiguo.niu@unisoc.com> There is bug on issue after atgc feature is enabled in 32bits platform as the following log: F2FS-fs (dm-x): inconsistent rbtree, cur(3470333575168) next(3320009719808) ------------[ cut here ]------------ kernel BUG at fs/f2fs/gc.c:602! Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM PC is at get_victim_by_default+0x13c0/0x1498 LR is at f2fs_check_rb_tree_consistence+0xc4/0xd4 .... [<c04d98b0>] (get_victim_by_default) from [<c04d4f44>] (f2fs_gc+0x220/0x6cc) [<c04d4f44>] (f2fs_gc) from [<c04d4780>] (gc_thread_func+0x2ac/0x708) [<c04d4780>] (gc_thread_func) from [<c015c774>] (kthread+0x1a8/0x1b4) [<c015c774>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) the reason is the 64bits key in struct rb_entry has __packed attibute but has not in struct victim_entry, so the wrong key value got by struct rb_entry in f2fs_check_rb_tree_consistence, the following are the memory layouts of struct rb_entry and struct victim_entry in 32bits platform: struct rb_entry { [0] struct rb_node rb_node; union { struct {...}; [12] unsigned long long key; }; } SIZE: 20 struct victim_entry { [0] struct rb_node rb_node; union { struct {...}; [16] struct victim_info vi; }; [32] struct list_head list; } SIZE: 40 This patch fix the inconsistence layout of 64bits key between struct rb_entry and struct victim_entry. Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection") Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> --- changes of v2: update patch according to Chao's suggestion. --- --- fs/f2fs/gc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi zhiguo.niu", Thank you for the patch! Perhaps something to improve: [auto build test WARNING on jaegeuk-f2fs/dev-test] [also build test WARNING on linus/master v6.1-rc4 next-20221109] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/zhiguo-niu/f2fs-fix-atgc-bug-on-issue-in-32bits-platform/20221108-153745 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 config: x86_64-rhel-8.3-func compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/6181893cb29317549ddd9eac74282f75f2a73f66 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review zhiguo-niu/f2fs-fix-atgc-bug-on-issue-in-32bits-platform/20221108-153745 git checkout 6181893cb29317549ddd9eac74282f75f2a73f66 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from fs/f2fs/segment.c:24: >> fs/f2fs/gc.h:73:1: warning: alignment 1 of 'struct victim_entry' is less than 8 [-Wpacked-not-aligned] 73 | } __packed; | ^ vim +73 fs/f2fs/gc.h 62 63 struct victim_entry { 64 struct rb_node rb_node; /* rb node located in rb-tree */ 65 union { 66 struct { 67 unsigned long long mtime; /* mtime of section */ 68 unsigned int segno; /* segment No. */ 69 }; 70 struct victim_info vi; /* victim info */ 71 }; 72 struct list_head list; > 73 } __packed; 74
Hi zhiguo.niu",
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jaegeuk-f2fs/dev-test]
[also build test WARNING on linus/master v6.1-rc4 next-20221109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/zhiguo-niu/f2fs-fix-atgc-bug-on-issue-in-32bits-platform/20221108-153745
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
config: arm-randconfig-c002-20221109
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 463da45892e2d2a262277b91b96f5f8c05dc25d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/6181893cb29317549ddd9eac74282f75f2a73f66
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review zhiguo-niu/f2fs-fix-atgc-bug-on-issue-in-32bits-platform/20221108-153745
git checkout 6181893cb29317549ddd9eac74282f75f2a73f66
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash fs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
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 {
^
1 warning generated.
vim +65 fs/f2fs/gc.h
093749e296e29a Chao Yu 2020-08-04 62
093749e296e29a Chao Yu 2020-08-04 63 struct victim_entry {
093749e296e29a Chao Yu 2020-08-04 64 struct rb_node rb_node; /* rb node located in rb-tree */
093749e296e29a Chao Yu 2020-08-04 @65 union {
093749e296e29a Chao Yu 2020-08-04 66 struct {
093749e296e29a Chao Yu 2020-08-04 67 unsigned long long mtime; /* mtime of section */
093749e296e29a Chao Yu 2020-08-04 68 unsigned int segno; /* segment No. */
093749e296e29a Chao Yu 2020-08-04 69 };
093749e296e29a Chao Yu 2020-08-04 70 struct victim_info vi; /* victim info */
093749e296e29a Chao Yu 2020-08-04 71 };
093749e296e29a Chao Yu 2020-08-04 72 struct list_head list;
6181893cb29317 Zhiguo Niu 2022-11-08 73 } __packed;
093749e296e29a Chao Yu 2020-08-04 74
On Thu, Nov 10, 2022, at 09:33, kernel test robot wrote: > Hi zhiguo.niu", > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on jaegeuk-f2fs/dev-test] > [also build test WARNING on linus/master v6.1-rc4 next-20221109] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: > https://github.com/intel-lab-lkp/linux/commits/zhiguo-niu/f2fs-fix-atgc-bug-on-issue-in-32bits-platform/20221108-153745 > 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'. Removing that is probably better than adding extra __packed annotation that just lead to less efficient code. Arnd
On Thu, Nov 10, 2022, at 11:20, Zhiguo Niu wrote: > Arnd Bergmann <arnd@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
On Fri, Nov 11, 2022, at 10:40, Zhiguo Niu wrote: > Arnd Bergmann <arnd@arndb.de> 于2022年11月10日周四 21:45写道: > I thinks the gcc complier build warning : > ---------------------------------------------------------------- > In file included from fs/f2fs/segment.c:24: >>> fs/f2fs/gc.h:73:1: warning: alignment 1 of 'struct victim_entry' is > >>> less than 8 [-Wpacked-not-aligned] > > 73 | } __packed; > > | ^ > > --------------------------------------------------------------- > > It is because struct rb_node has the attribute > "__attribute__((aligned(sizeof(long)", it is 8 bytes in 64bits platform. > > struct rb_node { > unsigned long __rb_parent_color; > struct rb_node *rb_right; > struct rb_node *rb_left; > } __attribute__((aligned(sizeof(long)))); > > so I just try to put __packed on union of struct victim_entry and i > also keep consistent with struct rb_entry. No, that attribute has no effect on any architecture other than m68k, which defaults to 16-bit alignment for 32-bit members. I'm fairly sure the alignment attribute on rb_node is entirely unrelated to the problems you are seeing in f2fs that come from having a structure with stricter (4 byte or 8 byte) alignment requirements embedded in a structure with relaxed (single-byte) alignment: > struct rb_entry { > struct rb_node rb_node; /* rb node located in rb-tree */ > union { > struct { > unsigned int ofs; /* start offset of the entry */ > unsigned int len; /* length of the entry */ > }; > unsigned long long key; /* 64-bits key */ > } __packed; > }; This tells the compiler that the anonymous union is entirely unconstrained, but the anonymous struct inside it has the default alignment, which is the contradition that gcc correctly warns about. Since the only thing you need here is to lower the alignment constraint from 8 bytes to 4 bytes, the easiest way is to have the __packed annotation on the 'key' member. This avoids all warnings, as long you do not take the address of the 'key' member and pass it to a function that expects an aligned 'u64' pointer. Arnd
On Mon, Nov 14, 2022, at 10:23, Zhiguo Niu wrote: > Arnd Bergmann <arnd@arndb.de> 于2022年11月11日周五 17:57写道: > so I just modify struct victim_entry as your suggestion: > diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h > index 19b956c2d697..e2f25b8fd865 100644 > --- a/fs/f2fs/gc.h > +++ b/fs/f2fs/gc.h > @@ -56,16 +56,16 @@ struct gc_inode_list { > }; > > struct victim_info { > - unsigned long long mtime; /* mtime of section */ > - unsigned int segno; /* section No. */ > + unsigned long long mtime __packed; /* mtime of section */ > + unsigned int segno; /* section No. */ > }; > > struct victim_entry { > struct rb_node rb_node; /* rb node located in rb-tree */ > union { > struct { > - unsigned long long mtime; /* mtime of section */ > - unsigned int segno; /* segment No. */ > + unsigned long long mtime __packed; /* mtime of section */ > + unsigned int segno; /* segment No. */ > }; > struct victim_info vi; /* victim info */ > }; Right, this should work. I'm still unsure where you need a union inside of victim_entry rather than just having the 'victim_info' portion in there by itself, but that is not a matter of correctness. > There is no problem with functional verification in both 64bit and > 32bit platforms, > sorry I don't have the environment to verify is build warnig reported > by the kernel test robot still there. I had a bit trouble reproducing this as well. It looks like this only happens when -Wunaligned-access is enabled for a config, but that requires two things: - building with CC=clang for a target architecture that does not support unaligned access natively, such as ARMv5. ARMv7 is interesting because it disables the warning, though it only supports unaligned load/store on 32-bit and 16-bit words but not 64-bit words using the ldrd/strd instructions. - Even on architectures with no unaligned load/store, the warning is disabled by default unless you use "make W=1" to enable extra warnings. Alternatively, you can enable the warning manually by passing "CFLAGS_MODULE=-Wunaligned-access" to make, which should trigger the warning on any 32-bit architecture. Arnd
diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h index 19b956c2d697..37e1142edf2c 100644 --- a/fs/f2fs/gc.h +++ b/fs/f2fs/gc.h @@ -70,7 +70,7 @@ struct victim_entry { struct victim_info vi; /* victim info */ }; struct list_head list; -}; +} __packed; /* * inline functions