[PATCH 1/2] f2fs: fix changing cursegs if recovery fails on zoned device

[PATCH 1/2] f2fs: fix changing cursegs if recovery fails on zoned device

[Sheng Yong]

Fsync data recovery attempts to check and fix write pointer consistency
of cursegs and all other zones. If the write pointers of cursegs are
unaligned, cursegs are changed to new sections.

If recovery fails, zone write pointers are still checked and fixed,
but the latest checkpoint cannot be written back. Additionally, retry-
mount skips recovery and rolls back to reuse the old cursegs whose
zones are already finished. This can lead to unaligned write later.

This patch addresses the issue by leaving writer pointers untouched if
recovery fails. When retry-mount is performed, cursegs and other zones
are checked and fixed after skipping recovery.

Signed-off-by: Song Feng <songfeng@oppo.com>
Signed-off-by: Yongpeng Yang <yangyongpeng1@oppo.com>
Signed-off-by: Sheng Yong <shengyong@oppo.com>
---
 fs/f2fs/recovery.c |  2 +-
 fs/f2fs/super.c    | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index e4d81b8705d1..324f948247ae 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -899,7 +899,7 @@ int f2fs_recover_fsync_data(struct f2fs_sb_info *sbi, bool check_only)
 	 * and the f2fs is not read only, check and fix zoned block devices'
 	 * write pointer consistency.
 	 */
-	if (f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sbi->sb)) {
+	if (!err && f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sbi->sb)) {
 		int err2 = f2fs_fix_curseg_write_pointer(sbi);
 
 		if (!err2)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 87ab5696bd48..42224c71ae20 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4738,13 +4738,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 reset_checkpoint:
 	/*
 	 * If the f2fs is not readonly and fsync data recovery succeeds,
-	 * check zoned block devices' write pointer consistency.
+	 * write pointer consistency of cursegs and other zones are already
+	 * checked and fixed during recovery. However, if recovery fails,
+	 * write pointers are left untouched, and retry-mount should check
+	 * them here.
 	 */
-	if (f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sb)) {
+	if (skip_recovery && f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sb)) {
 		int err2;
 
 		f2fs_notice(sbi, "Checking entire write pointers");
-		err2 = f2fs_check_write_pointer(sbi);
+		err2 = f2fs_fix_curseg_write_pointer(sbi);
+		if (!err2)
+			err2 = f2fs_check_write_pointer(sbi);
 		if (err2)
 			err = err2;
 	}
-- 
2.40.1

[PATCH 2/2] f2fs: clear SBI_POR_DOING before initing inmem curseg

[Sheng Yong]

SBI_POR_DOING can be cleared after recovery is completed, so that
changes made before recovery can be persistent, and subsequent
errors can be recorded into cp/sb.

Signed-off-by: Song Feng <songfeng@oppo.com>
Signed-off-by: Yongpeng Yang <yangyongpeng1@oppo.com>
Signed-off-by: Sheng Yong <shengyong@oppo.com>
---
 fs/f2fs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 42224c71ae20..92e93899c033 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4756,13 +4756,13 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto free_meta;
 
+	/* f2fs_recover_fsync_data() cleared this already */
+	clear_sbi_flag(sbi, SBI_POR_DOING);
+
 	err = f2fs_init_inmem_curseg(sbi);
 	if (err)
 		goto sync_free_meta;
 
-	/* f2fs_recover_fsync_data() cleared this already */
-	clear_sbi_flag(sbi, SBI_POR_DOING);
-
 	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
 		err = f2fs_disable_checkpoint(sbi);
 		if (err)
-- 
2.40.1

Re: [PATCH 2/2] f2fs: clear SBI_POR_DOING before initing inmem curseg

[Chao Yu]

On 2024/11/11 16:50, Sheng Yong wrote:
> SBI_POR_DOING can be cleared after recovery is completed, so that
> changes made before recovery can be persistent, and subsequent
> errors can be recorded into cp/sb.
> 
> Signed-off-by: Song Feng <songfeng@oppo.com>
> Signed-off-by: Yongpeng Yang <yangyongpeng1@oppo.com>
> Signed-off-by: Sheng Yong <shengyong@oppo.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

Re: [PATCH 1/2] f2fs: fix changing cursegs if recovery fails on zoned device

[Chao Yu]

On 2024/11/11 16:50, Sheng Yong wrote:
> Fsync data recovery attempts to check and fix write pointer consistency
> of cursegs and all other zones. If the write pointers of cursegs are
> unaligned, cursegs are changed to new sections.
> 
> If recovery fails, zone write pointers are still checked and fixed,
> but the latest checkpoint cannot be written back. Additionally, retry-
> mount skips recovery and rolls back to reuse the old cursegs whose
> zones are already finished. This can lead to unaligned write later.
> 
> This patch addresses the issue by leaving writer pointers untouched if
> recovery fails. When retry-mount is performed, cursegs and other zones
> are checked and fixed after skipping recovery.
> 
> Signed-off-by: Song Feng <songfeng@oppo.com>
> Signed-off-by: Yongpeng Yang <yangyongpeng1@oppo.com>
> Signed-off-by: Sheng Yong <shengyong@oppo.com>
> ---
>   fs/f2fs/recovery.c |  2 +-
>   fs/f2fs/super.c    | 11 ++++++++---
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index e4d81b8705d1..324f948247ae 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -899,7 +899,7 @@ int f2fs_recover_fsync_data(struct f2fs_sb_info *sbi, bool check_only)
>   	 * and the f2fs is not read only, check and fix zoned block devices'
>   	 * write pointer consistency.
>   	 */
> -	if (f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sbi->sb)) {
> +	if (!err && f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sbi->sb)) {
>   		int err2 = f2fs_fix_curseg_write_pointer(sbi);
>   
>   		if (!err2)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 87ab5696bd48..42224c71ae20 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4738,13 +4738,18 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>   reset_checkpoint:
>   	/*
>   	 * If the f2fs is not readonly and fsync data recovery succeeds,
> -	 * check zoned block devices' write pointer consistency.
> +	 * write pointer consistency of cursegs and other zones are already
> +	 * checked and fixed during recovery. However, if recovery fails,
> +	 * write pointers are left untouched, and retry-mount should check
> +	 * them here.
>   	 */
> -	if (f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sb)) {
> +	if (skip_recovery && f2fs_sb_has_blkzoned(sbi) && !f2fs_readonly(sb)) {
>   		int err2;
>   
>   		f2fs_notice(sbi, "Checking entire write pointers");
> -		err2 = f2fs_check_write_pointer(sbi);
> +		err2 = f2fs_fix_curseg_write_pointer(sbi);
> +		if (!err2)
> +			err2 = f2fs_check_write_pointer(sbi);
>   		if (err2)
>   			err = err2;

What about wrapping above logic into f2fs_fix_and_check_write_pointer() and reuse in
f2fs_fill_super() and f2fs_recover_fsync_data()?

int f2fs_fix_and_check_write_pointer()
{
	int err;

	if (!f2fs_sb_has_blkzoned(sbi) || f2fs_readonly(sbi->sb))
		return 0;

	err = f2fs_fix_curseg_write_pointer(sbi);
	if (!err)
		err = f2fs_check_write_pointer(sbi);

	return err;
}

Thanks,

>   	}