Re: [PATCH V1] f2fs: fix potentail deadloop issue in do_recover_data

[Chao Yu]

Zhiguo,

Can you please check below version? Is it fine to you?

https://lore.kernel.org/linux-f2fs-devel/20240124144915.19445-1-chao@xxxxxxxxxx

On 2024/1/22 13:46, Zhiguo Niu wrote:
> Hi Chao
>
> On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <chao@xxxxxxxxxx> wrote:
> > 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@xxxxxxxxxx>
> > 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 recovery 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 = DEFAULT_RETRY_COUNT; loops > 0; loops--) {
> >                          err = f2fs_reserve_new_block(&dn);
> >                          if (!err ||
> >                                  !IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION))
> >                                  break;
> >                  }
>
> Thanks for your detailed explanation and I understand.
> It seems that the original process is also reasonable,
> so it’s 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@xxxxxxxxxx>
> > > ---
> > >    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 == NEW_ADDR) {
> > >                        f2fs_truncate_data_blocks_range(&dn, 1);
> > > -                     do {
> > > -                             err = f2fs_reserve_new_block(&dn);
> > > -                             if (err == -ENOSPC) {
> > > -                                     f2fs_bug_on(sbi, 1);
> > > -                                     break;
> > > -                             }
> > > -                     } while (err &&
> > > -                             IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION));
> > > -                     if (err)
> > > +                     err = f2fs_reserve_new_block(&dn);
> > > +                     if (err == -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 == NULL_ADDR) {
> > > -                             do {
> > > -                                     err = f2fs_reserve_new_block(&dn);
> > > -                                     if (err == -ENOSPC) {
> > > -                                             f2fs_bug_on(sbi, 1);
> > > -                                             break;
> > > -                                     }
> > > -                             } while (err &&
> > > -                                     IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION));
> > > -                             if (err)
> > > +                             err = f2fs_reserve_new_block(&dn);
> > > +                             if (err == -ENOSPC)
> > > +                                     f2fs_bug_on(sbi, 1);
> > > +                             else if (err)
> > >                                        goto err;
> > >                        }
> > >    retry_prev: