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 <[email protected]>
---
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:
--
1.9.1
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 <[email protected]>
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> > ---
> > 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/[email protected]
On 2024/1/22 13:46, Zhiguo Niu wrote:
> Hi Chao
>
> On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <[email protected]> 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 <[email protected]>
>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]> wrote:
>
> Zhiguo,
>m
> Can you please check below version? Is it fine to you?
>
> https://lore.kernel.org/linux-f2fs-devel/[email protected]
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 <[email protected]> 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 <[email protected]>
> >> 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 <[email protected]>
> >>> ---
> >>> 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: