| Message ID | 1703502715-11936-1-git-send-email-zhiguo.niu@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,V1] f2fs: fix potentail deadloop issue in do_recover_data | expand |
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 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, > > 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 == 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:
Hi Chao On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <chao@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 <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 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@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:
Zhiguo, Can you please check below version? Is it fine to you? https://lore.kernel.org/linux-f2fs-devel/20240124144915.19445-1-chao@kernel.org On 2024/1/22 13:46, Zhiguo Niu wrote: > Hi Chao > > On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <chao@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 <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 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@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:
Hi Chao, On Wed, Jan 24, 2024 at 10:54 PM 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@kernel.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 AM Chao Yu <chao@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 <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 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@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:
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:
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; besides, the mainly error -ENOSPC has been handled as bug on, so other error cases can be proecssed normally without looping. 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(-)