Hi Chao
On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <c...@kernel.org> 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 <jaeg...@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 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....@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 == 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:
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
