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

From: Chao Yu
Date: Wed Jan 24 2024 - 09:56:46 EST


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: