Received-SPF: pass (google.com: domain of linux-kernel+bounces-84296-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <1707271264-5551-1-git-send-email-zhiguo.niu@unisoc.com>
 <88459a22-a21c-4c3d-8371-31d7d713b536@kernel.org> <CAHJ8P3KYY27M3v=9Lu-yD2ufxU1fdG-bg=G92AbpnLUx0zLz3g@mail.gmail.com>
 <f36ef5d0-8922-449e-b19c-de009c89e712@kernel.org> <CAHJ8P3KS2YAXm=GND8DknZqvGqTvm38Nv_9z1nEq4cTJjir-sA@mail.gmail.com>
 <Zd03RHMKBqv1f6GF@google.com> <CAHJ8P3Lj_1pa=npkAsMfeStEY3nWgf591tFndKTT_skFYLv+GQ@mail.gmail.com>
 <Zd4ZTokbVPkj5gy4@google.com>
In-Reply-To: <Zd4ZTokbVPkj5gy4@google.com>
From: Zhiguo Niu <niuzhiguo84@gmail.com>
Date: Wed, 28 Feb 2024 09:00:37 +0800
Message-ID: <CAHJ8P3+bpRyghkcOoDaJ_2r1Wh69jF1W-8Rm881y6d3rif8xhQ@mail.gmail.com>
Subject: Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device
To: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Chao Yu <chao@kernel.org>, Zhiguo Niu <zhiguo.niu@unisoc.com>, 
	linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, 
	ke.wang@unisoc.com, hongyu.jin@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 28, 2024 at 1:18=E2=80=AFAM Jaegeuk Kim <jaegeuk@kernel.org> wr=
ote:
>
> On 02/27, Zhiguo Niu wrote:
> > On Tue, Feb 27, 2024 at 9:13=E2=80=AFAM Jaegeuk Kim <jaegeuk@kernel.org=
> wrote:
> > >
> > > On 02/26, Zhiguo Niu wrote:
> > > > Dear Chao,
> > > >
> > > > On Fri, Feb 23, 2024 at 10:38=E2=80=AFAM Chao Yu <chao@kernel.org> =
wrote:
> > > > >
> > > > > On 2024/2/23 10:01, Zhiguo Niu wrote:
> > > > > >
> > > > > >
> > > > > > On Thu, Feb 22, 2024 at 8:30=E2=80=AFPM Chao Yu <chao@kernel.or=
g <mailto:chao@kernel.org>> wrote:
> > > > > >
> > > > > >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> > > > > >      > A panic issue happened in a reboot test in small capacit=
y device
> > > > > >      > as following:
> > > > > >      > 1.The device size is 64MB, and main area has 24 segments=
, and
> > > > > >      > CONFIG_F2FS_CHECK_FS is not enabled.
> > > > > >      > 2.There is no any free segments left shown in free_segma=
p_info,
> > > > > >      > then another write request cause get_new_segment get a o=
ut-of-bound
> > > > > >      > segment with segno 24.
> > > > > >      > 3.panic happen in update_sit_entry because access invali=
d bitmap
> > > > > >      > pointer.
> > > > > >
> > > > > >     Zhiguo,
> > > > > >
> > > > > >     Can you please try below patch to see whether it can fix yo=
ur problem?
> > > > > >
> > > > > >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883=
141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121=
851.883141-3-chao@kernel.org>
> > > > > >
> > > > > >     Thanks,
> > > > > >
> > > > > >
> > > > > > Dear Chao,
> > > > > > I need to coordinate the testing resources. The previous testin=
g has been stopped because it was fixed with the current patch. In addition=
, this requires stability testing to reproduce, so it will take a certain a=
mount of time. If there is any situation, I will tell you in time.
> > > > >
> > > > > Zhiguo, thank you!
> > > >
> > > > We tested this patch  this weekend on the previous version with
> > > > problem, and it can not reproduce panic issues,
> > > > so this patch should fix the original issue.
> > > > thanks=EF=BC=81
> > >
> > Dear Jaegeuk,
> > > Hey, do you guys please point out which patches were tested without w=
hat?
> > This problem occurred during our platform stability testing.
> > it can be fixed by my  this patch set, mainly be fixed by:
> > f2fs: fix panic issue in update_sit_entry & f2fs: enhance judgment
> > conditions of GET_SEGNO
> > and Chao's patch can also fix this problems testing without my patch
> > > IOWs, which patches should I remove and keep Chao's patch?
> > I think chao's patch is more reasonable, it does error handling more co=
mplete.
> > but my patch just do some sanity check for return value of GET_SEGNO
> > Same as other codes(update_segment_mtime)
> > and i think it also needed except this part:
>
> Thanks for confirmation. It seems it'd be better to revert yours and appl=
y
> Chao's patch first. If you think there's something to improve on top of i=
t,
> could you please send another patch afterwards?

OK, I think this two patches still needed
  f2fs: correct counting methods of free_segments in __set_inuse
  f2fs: fix panic issue in update_sit_entry
and I'll reorganize it
thanks
>
> >
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 3bf2ce46fa0907..bb22feeae1cfcb 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct
> > f2fs_sb_info *sbi,
> > (GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & (BLKS_PER_SEG(sbi) - 1))
> > #define GET_SEGNO(sbi, blk_addr) \
> > - ((!__is_valid_data_blkaddr(blk_addr)) ? \
> > + ((!__is_valid_data_blkaddr(blk_addr) || \
> > + !f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ? \
> > NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
> > GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
> > #define CAP_BLKS_PER_SEC(sbi)
> > because Chao's patch let new_addr=3Dnull_addr when  get_new_segment
> > returns NOSPACE,
> > so I think this can be reverted and it also saves code running time.
> > How about Chao's opinions?
> > thanks!
> > >
> > > >
> > > > >
> > > > > BTW, I've tested this patch for a while, and it looks there is no=
 issue w/
> > > > > FAULT_NO_SEGMENT fault injection is on.
> > > > >
> > > > > > btw, Why can=E2=80=99t I see this patch on your branch^^?
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/=
log/?h=3Ddev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/lin=
ux.git/log/?h=3Ddev-test>
> > > > >
> > > > > Too lazy to push patches in time, will do it in this weekend. :P
> > > > >
> > > > > > thanks=EF=BC=81
> > > > > >
> > > > > >
> > > > > >      >
> > > > > >      > More detail shown in following patch sets.
> > > > > >      > The three patches are splited here because the modificat=
ions are
> > > > > >      > relatively independent and more readable.
> > > > > >      >
> > > > > >      > ---
> > > > > >      > Changes of v2: stop checkpoint when get a out-of-bound s=
egment
> > > > > >      > ---
> > > > > >      >
> > > > > >      > Zhiguo Niu (4):
> > > > > >      >    f2fs: correct counting methods of free_segments in __=
set_inuse
> > > > > >      >    f2fs: fix panic issue in update_sit_entry
> > > > > >      >    f2fs: enhance judgment conditions of GET_SEGNO
> > > > > >      >    f2fs: stop checkpoint when get a out-of-bounds segmen=
t
> > > > > >      >
> > > > > >      >   fs/f2fs/file.c          |  7 ++++++-
> > > > > >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> > > > > >      >   fs/f2fs/segment.h       |  7 ++++---
> > > > > >      >   include/linux/f2fs_fs.h |  1 +
> > > > > >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> > > > > >      >
> > > > > >