Received-SPF: pass (google.com: domain of linux-kernel+bounces-37906-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <1703502715-11936-1-git-send-email-zhiguo.niu@unisoc.com>
 <74243f43-c129-4530-970c-4de2afcd307e@kernel.org> <CAHJ8P3KmnN3rc5yXh2ecg21Eu61srUJsJP8=TbPxfSu4dY91EQ@mail.gmail.com>
 <b2a3268a-7087-4ff5-9148-dcbc7c95e2bb@kernel.org>
In-Reply-To: <b2a3268a-7087-4ff5-9148-dcbc7c95e2bb@kernel.org>
From: Zhiguo Niu <niuzhiguo84@gmail.com>
Date: Thu, 25 Jan 2024 11:01:38 +0800
Message-ID: <CAHJ8P3+GeS4TSdwAPJ8dO_R08Vvi1iv_ua+9SU2JY5weuN4hKw@mail.gmail.com>
Subject: Re: [PATCH V1] f2fs: fix potentail deadloop issue in do_recover_data
To: Chao Yu <chao@kernel.org>
Cc: Zhiguo Niu <zhiguo.niu@unisoc.com>, jaegeuk@kernel.org, 
	linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, 
	ke.wang@unisoc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Chao,

On Wed, Jan 24, 2024 at 10:54=E2=80=AFPM Chao Yu <chao@kernel.org> wrote:
>
> Zhiguo,
>m
> Can you please check below version? Is it fine to you?
>
> https://lore.kernel.org/linux-f2fs-devel/20240124144915.19445-1-chao@kern=
el.org
it is ok to me and more reasonable than my  version
thanks~
>
> On 2024/1/22 13:46, Zhiguo Niu wrote:
> > Hi Chao
> >
> > On Mon, Jan 22, 2024 at 11:46=E2=80=AFAM Chao Yu <chao@kernel.org> wrot=
e:
> >>
> >> On 2023/12/25 19:11, Zhiguo Niu wrote:
> >>> There is a potentail deadloop issue in the corner case of
> >>> CONFIG_F2FS_FAULT_INJECTION is enabled and the return value
> >>> of f2fs_reserve_new_block is error but not -ENOSPC, such as
> >>> this error case:
> >>> if (unlikely(is_inode_flag_set(dn->inode, FI_NO_ALLOC)))
> >>>                return -EPERM;
> >>
> >> I don't see any path to trigger this error? am I missing something?
> >>
> >>> besides, the mainly error -ENOSPC has been handled as bug on,
> >>> so other error cases can be proecssed normally without looping.
> >>
> >> commit 975756c41332bc5e523e9f843271ed5ab6aaaaaa
> >> Author: Jaegeuk Kim <jaegeuk@kernel.org>
> >> Date:   Thu May 19 11:57:21 2016 -0700
> >>
> >>       f2fs: avoid ENOSPC fault in the recovery process
> >>
> >>       This patch avoids impossible error injection, ENOSPC, during rec=
overy process.
> >>
> >> Please check above patch, I guess intention of adding such loop is
> >> to avoid mount failure due to fault injection was triggered in
> >> f2fs_reserve_new_block().
> >>
> >> What about change as blew?
> >> - keep the loop to avoid mount failure.
> >> - remove bug_on() to avoid panic due to fault injection error.
> >>
> >> #define DEFAULT_RETRY_COUNT             8
> >>
> >>                  for (loops =3D DEFAULT_RETRY_COUNT; loops > 0; loops-=
-) {
> >>                          err =3D f2fs_reserve_new_block(&dn);
> >>                          if (!err ||
> >>                                  !IS_ENABLED(CONFIG_F2FS_FAULT_INJECTI=
ON))
> >>                                  break;
> >>                  }
> >
> > Thanks for your detailed explanation and I understand.
> > It seems that the original process is also reasonable,
> > so it=E2=80=99s okay to keep it as it is.
> >>
> >> Thanks,
> >>
> >>>
> >>> Fixes: 956fa1ddc132 ("f2fs: fix to check return value of f2fs_reserve=
_new_block()")
> >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> >>> ---
> >>>    fs/f2fs/recovery.c | 26 ++++++++------------------
> >>>    1 file changed, 8 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>> index 21381b7..5d658f6 100644
> >>> --- a/fs/f2fs/recovery.c
> >>> +++ b/fs/f2fs/recovery.c
> >>> @@ -710,15 +710,10 @@ static int do_recover_data(struct f2fs_sb_info =
*sbi, struct inode *inode,
> >>>                 */
> >>>                if (dest =3D=3D NEW_ADDR) {
> >>>                        f2fs_truncate_data_blocks_range(&dn, 1);
> >>> -                     do {
> >>> -                             err =3D f2fs_reserve_new_block(&dn);
> >>> -                             if (err =3D=3D -ENOSPC) {
> >>> -                                     f2fs_bug_on(sbi, 1);
> >>> -                                     break;
> >>> -                             }
> >>> -                     } while (err &&
> >>> -                             IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION)=
);
> >>> -                     if (err)
> >>> +                     err =3D f2fs_reserve_new_block(&dn);
> >>> +                     if (err =3D=3D -ENOSPC)
> >>> +                             f2fs_bug_on(sbi, 1);
> >>> +                     else if (err)
> >>>                                goto err;
> >>>                        continue;
> >>>                }
> >>> @@ -727,15 +722,10 @@ static int do_recover_data(struct f2fs_sb_info =
*sbi, struct inode *inode,
> >>>                if (f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
> >>>
> >>>                        if (src =3D=3D NULL_ADDR) {
> >>> -                             do {
> >>> -                                     err =3D f2fs_reserve_new_block(=
&dn);
> >>> -                                     if (err =3D=3D -ENOSPC) {
> >>> -                                             f2fs_bug_on(sbi, 1);
> >>> -                                             break;
> >>> -                                     }
> >>> -                             } while (err &&
> >>> -                                     IS_ENABLED(CONFIG_F2FS_FAULT_IN=
JECTION));
> >>> -                             if (err)
> >>> +                             err =3D f2fs_reserve_new_block(&dn);
> >>> +                             if (err =3D=3D -ENOSPC)
> >>> +                                     f2fs_bug_on(sbi, 1);
> >>> +                             else if (err)
> >>>                                        goto err;
> >>>                        }
> >>>    retry_prev: