[V2] f2fs: fix atgc bug on issue in 32bits platform

  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(-)
  

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