Received-SPF: pass (google.com: domain of linux-kernel+bounces-32310-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <74243f43-c129-4530-970c-4de2afcd307e@kernel.org>
Date: Mon, 22 Jan 2024 11:46:15 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V1] f2fs: fix potentail deadloop issue in do_recover_data
Content-Language: en-US
To: Zhiguo Niu <zhiguo.niu@unisoc.com>, jaegeuk@kernel.org
Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
 niuzhiguo84@gmail.com, ke.wang@unisoc.com
References: <1703502715-11936-1-git-send-email-zhiguo.niu@unisoc.com>
From: Chao Yu <chao@kernel.org>
In-Reply-To: <1703502715-11936-1-git-send-email-zhiguo.niu@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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: