[PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Zhiguo Niu]

A panic issue happened in a reboot test in small capacity device
as following:
1.The device size is 64MB, and main area has 24 segments, and
CONFIG_F2FS_CHECK_FS is not enabled.
2.There is no any free segments left shown in free_segmap_info,
then another write request cause get_new_segment get a out-of-bound
segment with segno 24.
3.panic happen in update_sit_entry because access invalid bitmap
pointer.

More detail shown in following patch sets.
The three patches are splited here because the modifications are
relatively independent and more readable.

---
Changes of v2: stop checkpoint when get a out-of-bound segment
---

Zhiguo Niu (4):
  f2fs: correct counting methods of free_segments in __set_inuse
  f2fs: fix panic issue in update_sit_entry
  f2fs: enhance judgment conditions of GET_SEGNO
  f2fs: stop checkpoint when get a out-of-bounds segment

 fs/f2fs/file.c          |  7 ++++++-
 fs/f2fs/segment.c       | 21 ++++++++++++++++-----
 fs/f2fs/segment.h       |  7 ++++---
 include/linux/f2fs_fs.h |  1 +
 4 files changed, 27 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] f2fs: correct counting methods of free_segments in __set_inuse

[Zhiguo Niu]

There is a corner scenario on a small-capacity partition with 64MB size:
1. The main area has a total of 24 segments, and there are no free
segments left shown from the free_segmap bitmap and free_secmap in
free_segmap_info.
---------------------------------------------------------------------
bitmap value: ffffffffffffffff
---------------------------------------------------------------------
2. When doing gc, an out-of-bounds segment with segno=24 is allocated.
Because CONFIG_F2FS_CHECK_FS is not enabled, f2fs_bug_on in get_new_segment
just print warning log but the subsequent process continues to run.
---------------------------------------------------------------------
got_it:
    /* set it as dirty segment in free segmap */
    f2fs_bug_on(sbi, test_bit(segno, free_i->free_segmap));
    __set_inuse(sbi, segno);
----------------------------------------------------------------------
3. __set_inuse directly sets free_i->free_segments--,
As a result, free_i->free_segments=-1, as shown in the following
coredump information:
----------------------------------------------------------------------
  crash_arm64> struct free_segmap_info 0xffffff8084d9a000 -x
  struct free_segmap_info {
  start_segno = 0x7,
  free_segments = 0xffffffff,
  free_sections = 0x0,
----------------------------------------------------------------------
This is unreasonable and will cause free_segments and free_sections
counts mismatch if there are segments released as free.

So same counting methods like free_sections should be used to
free_segments.

Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
 fs/f2fs/segment.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 8129be7..f2847f1 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -463,8 +463,8 @@ static inline void __set_inuse(struct f2fs_sb_info *sbi,
 	struct free_segmap_info *free_i = FREE_I(sbi);
 	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
 
-	set_bit(segno, free_i->free_segmap);
-	free_i->free_segments--;
+	if (!test_and_set_bit(segno, free_i->free_segmap))
+		free_i->free_segments--;
 	if (!test_and_set_bit(secno, free_i->free_secmap))
 		free_i->free_sections--;
 }
-- 
1.9.1

[PATCH v2 2/4] f2fs: fix panic issue in update_sit_entry

[Zhiguo Niu]

When CONFIG_F2FS_CHECK_FS is not enabled, f2fs_bug_on just printing
warning, get_new_segment may get an out-of-bounds segment when there
is no free segments. Then a block is allocated from this invalid
segment, update_sit_entry will access the invalid bitmap address,
cause system panic. Just as below call stack:

f2fs_allocate_data_block get a block address with 0x4000 and
partition size is 64MB

[   13.401997] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[   13.402003] Mem abort info:
[   13.402006]   ESR = 0x96000005
[   13.402009]   EC = 0x25: DABT (current EL), IL = 32 bits
[   13.402015]   SET = 0, FnV = 0
[   13.402018]   EA = 0, S1PTW = 0
[   13.402021]   FSC = 0x05: level 1 translation fault
[   13.402025] Data abort info:
[   13.402027]   ISV = 0, ISS = 0x00000005
[   13.402030]   CM = 0, WnR = 0
[   13.402034] user pgtable: 4k pages, 39-bit VAs, pgdp=00000001066ab000
[   13.402038] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[   13.402052] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[   13.489854] pc : update_sit_entry+0x128/0x420
[   13.490497] lr : f2fs_allocate_data_block+0x6b0/0xc2c
[   13.491218] sp : ffffffc00e023440
[   13.501530] Call trace:
[   13.501930]  update_sit_entry+0x128/0x420
[   13.502523]  f2fs_allocate_data_block+0x6b0/0xc2c
[   13.503203]  do_write_page+0xf0/0x1d4
[   13.503752]  f2fs_outplace_write_data+0x68/0xfc
[   13.504408]  f2fs_do_write_data_page+0x3a8/0x65c
[   13.505076]  move_data_page+0x294/0x7a8
[   13.505647]  gc_data_segment+0x4b8/0x800
[   13.506229]  do_garbage_collect+0x354/0x674
[   13.506843]  f2fs_gc+0x280/0x68c
[   13.507340]  f2fs_balance_fs+0x104/0x144
[   13.507921]  f2fs_create+0x310/0x3d8
[   13.508458]  path_openat+0x53c/0xc28
[   13.508997]  do_filp_open+0xbc/0x16c
[   13.509535]  do_sys_openat2+0xa0/0x2a0

So sanity check should be add in update_sit_entry.
Also remove some redundant judgment code.

Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
 fs/f2fs/segment.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ad6511f..f373ff7 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2399,6 +2399,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 #endif
 
 	segno = GET_SEGNO(sbi, blkaddr);
+	if (segno == NULL_SEGNO)
+		return;
 
 	se = get_seg_entry(sbi, segno);
 	new_vblocks = se->valid_blocks + del;
@@ -3464,8 +3466,7 @@ void f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 	 * since SSR needs latest valid block information.
 	 */
 	update_sit_entry(sbi, *new_blkaddr, 1);
-	if (GET_SEGNO(sbi, old_blkaddr) != NULL_SEGNO)
-		update_sit_entry(sbi, old_blkaddr, -1);
+	update_sit_entry(sbi, old_blkaddr, -1);
 
 	/*
 	 * If the current segment is full, flush it out and replace it with a
-- 
1.9.1

[PATCH v2 3/4] f2fs: enhance judgment conditions of GET_SEGNO

[Zhiguo Niu]

NULL_SEGNO should also be returned when the blk_addr value is
out-of-bound main area even __is_valid_data_blkaddr return true.

For example, a 64MB partition with total 24 main segments has no
any free segments left, then a new wrtie request use get_new_segment
may get a out-of-bound segno 24 if CONFIG_F2FS_CHECK_FS is not enabled.
GET_SEGNO should also return NULL_SEGNO in this case rather than treating
is as valid segment.

Besides, if the caller of GET_SEGNO does not ensure blk_addr pass to
GET_SEGNO is valid, it should do sanity check about return value of
GET_SEGNO, avoid causing some unexpected problems later.

Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
 fs/f2fs/file.c    | 7 ++++++-
 fs/f2fs/segment.c | 4 +++-
 fs/f2fs/segment.h | 3 ++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 23cd6a1..2cd3cd9 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2985,9 +2985,14 @@ static int f2fs_ioc_flush_device(struct file *filp, unsigned long arg)
 	if (ret)
 		return ret;
 
-	if (range.dev_num != 0)
+	if (range.dev_num != 0) {
 		dev_start_segno = GET_SEGNO(sbi, FDEV(range.dev_num).start_blk);
+		if (dev_start_segno == NULL_SEGNO)
+			return -EINVAL;
+	}
 	dev_end_segno = GET_SEGNO(sbi, FDEV(range.dev_num).end_blk);
+	if (dev_end_segno == NULL_SEGNO)
+		return -EINVAL;
 
 	start_segno = sm->last_victim[FLUSH_DEVICE];
 	if (start_segno < dev_start_segno || start_segno >= dev_end_segno)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index f373ff7..6772ad4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2496,7 +2496,7 @@ void f2fs_invalidate_blocks(struct f2fs_sb_info *sbi, block_t addr)
 	struct sit_info *sit_i = SIT_I(sbi);
 
 	f2fs_bug_on(sbi, addr == NULL_ADDR);
-	if (addr == NEW_ADDR || addr == COMPRESS_ADDR)
+	if (segno == NULL_SEGNO)
 		return;
 
 	f2fs_invalidate_internal_cache(sbi, addr);
@@ -3708,6 +3708,8 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 	unsigned char old_alloc_type;
 
 	segno = GET_SEGNO(sbi, new_blkaddr);
+	if (segno == NULL_SEGNO)
+		return;
 	se = get_seg_entry(sbi, segno);
 	type = se->type;
 
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index f2847f1..b0ea315 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct f2fs_sb_info *sbi,
 	(GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & ((sbi)->blocks_per_seg - 1))
 
 #define GET_SEGNO(sbi, blk_addr)					\
-	((!__is_valid_data_blkaddr(blk_addr)) ?			\
+	((!__is_valid_data_blkaddr(blk_addr) ||			\
+	!f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ?	\
 	NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi),			\
 		GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
 #define BLKS_PER_SEC(sbi)					\
-- 
1.9.1

[PATCH v2 4/4] f2fs: stop checkpoint when get a out-of-bounds segment

[Zhiguo Niu]

There is low probability that an out-of-bounds segment will be got
on a small-capacity device. In order to prevent subsequent write requests
allocating block address from this invalid segment, which may cause
unexpected issue, stop checkpoint should be performed.

Also introduce a new stop cp reason:  STOP_CP_REASON_OUTOF_RAGNE.

Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
 fs/f2fs/segment.c       | 12 ++++++++++--
 include/linux/f2fs_fs.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 6772ad4..6fe2baf 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2666,7 +2666,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
 		if (dir == ALLOC_RIGHT) {
 			secno = find_first_zero_bit(free_i->free_secmap,
 							MAIN_SECS(sbi));
-			f2fs_bug_on(sbi, secno >= MAIN_SECS(sbi));
+			if (secno >= MAIN_SECS(sbi)) {
+				f2fs_stop_checkpoint(sbi, false,
+						STOP_CP_REASON_OUTOF_RAGNE);
+				f2fs_bug_on(sbi, 1);
+			}
 		} else {
 			go_left = 1;
 			left_start = hint - 1;
@@ -2682,7 +2686,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
 		}
 		left_start = find_first_zero_bit(free_i->free_secmap,
 							MAIN_SECS(sbi));
-		f2fs_bug_on(sbi, left_start >= MAIN_SECS(sbi));
+		if (left_start >= MAIN_SECS(sbi)) {
+			f2fs_stop_checkpoint(sbi, false,
+					STOP_CP_REASON_OUTOF_RAGNE);
+			f2fs_bug_on(sbi, 1);
+		}
 		break;
 	}
 	secno = left_start;
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 053137a0..72c6782 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -81,6 +81,7 @@ enum stop_cp_reason {
 	STOP_CP_REASON_CORRUPTED_SUMMARY,
 	STOP_CP_REASON_UPDATE_INODE,
 	STOP_CP_REASON_FLUSH_FAIL,
+	STOP_CP_REASON_OUTOF_RAGNE,
 	STOP_CP_REASON_MAX,
 };
 
-- 
1.9.1

Re: [PATCH v2 4/4] f2fs: stop checkpoint when get a out-of-bounds segment

[Jaegeuk Kim]

On 02/07, Zhiguo Niu wrote:
> There is low probability that an out-of-bounds segment will be got
> on a small-capacity device. In order to prevent subsequent write requests
> allocating block address from this invalid segment, which may cause
> unexpected issue, stop checkpoint should be performed.
> 
> Also introduce a new stop cp reason:  STOP_CP_REASON_OUTOF_RAGNE.

OUT_OF_RANGE?

> 
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> ---
>  fs/f2fs/segment.c       | 12 ++++++++++--
>  include/linux/f2fs_fs.h |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 6772ad4..6fe2baf 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2666,7 +2666,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
>  		if (dir == ALLOC_RIGHT) {
>  			secno = find_first_zero_bit(free_i->free_secmap,
>  							MAIN_SECS(sbi));
> -			f2fs_bug_on(sbi, secno >= MAIN_SECS(sbi));
> +			if (secno >= MAIN_SECS(sbi)) {
> +				f2fs_stop_checkpoint(sbi, false,
> +						STOP_CP_REASON_OUTOF_RAGNE);
> +				f2fs_bug_on(sbi, 1);
> +			}
>  		} else {
>  			go_left = 1;
>  			left_start = hint - 1;
> @@ -2682,7 +2686,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
>  		}
>  		left_start = find_first_zero_bit(free_i->free_secmap,
>  							MAIN_SECS(sbi));
> -		f2fs_bug_on(sbi, left_start >= MAIN_SECS(sbi));
> +		if (left_start >= MAIN_SECS(sbi)) {
> +			f2fs_stop_checkpoint(sbi, false,
> +					STOP_CP_REASON_OUTOF_RAGNE);
> +			f2fs_bug_on(sbi, 1);
> +		}
>  		break;
>  	}
>  	secno = left_start;
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index 053137a0..72c6782 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -81,6 +81,7 @@ enum stop_cp_reason {
>  	STOP_CP_REASON_CORRUPTED_SUMMARY,
>  	STOP_CP_REASON_UPDATE_INODE,
>  	STOP_CP_REASON_FLUSH_FAIL,
> +	STOP_CP_REASON_OUTOF_RAGNE,
>  	STOP_CP_REASON_MAX,
>  };
>  
> -- 
> 1.9.1

Re: [PATCH v2 4/4] f2fs: stop checkpoint when get a out-of-bounds segment

[Chao Yu]

On 2024/2/8 8:16, Jaegeuk Kim wrote:
> On 02/07, Zhiguo Niu wrote:
>> There is low probability that an out-of-bounds segment will be got
>> on a small-capacity device. In order to prevent subsequent write requests
>> allocating block address from this invalid segment, which may cause
>> unexpected issue, stop checkpoint should be performed.
>>
>> Also introduce a new stop cp reason:  STOP_CP_REASON_OUTOF_RAGNE.
> 
> OUT_OF_RANGE?

Maybe STOP_CP_REASON_NO_SEGMENT will be more explicit?

Thanks,

> 
>>
>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
>> ---
>>   fs/f2fs/segment.c       | 12 ++++++++++--
>>   include/linux/f2fs_fs.h |  1 +
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 6772ad4..6fe2baf 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -2666,7 +2666,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
>>   		if (dir == ALLOC_RIGHT) {
>>   			secno = find_first_zero_bit(free_i->free_secmap,
>>   							MAIN_SECS(sbi));
>> -			f2fs_bug_on(sbi, secno >= MAIN_SECS(sbi));
>> +			if (secno >= MAIN_SECS(sbi)) {
>> +				f2fs_stop_checkpoint(sbi, false,
>> +						STOP_CP_REASON_OUTOF_RAGNE);
>> +				f2fs_bug_on(sbi, 1);
>> +			}
>>   		} else {
>>   			go_left = 1;
>>   			left_start = hint - 1;
>> @@ -2682,7 +2686,11 @@ static void get_new_segment(struct f2fs_sb_info *sbi,
>>   		}
>>   		left_start = find_first_zero_bit(free_i->free_secmap,
>>   							MAIN_SECS(sbi));
>> -		f2fs_bug_on(sbi, left_start >= MAIN_SECS(sbi));
>> +		if (left_start >= MAIN_SECS(sbi)) {
>> +			f2fs_stop_checkpoint(sbi, false,
>> +					STOP_CP_REASON_OUTOF_RAGNE);
>> +			f2fs_bug_on(sbi, 1);
>> +		}
>>   		break;
>>   	}
>>   	secno = left_start;
>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
>> index 053137a0..72c6782 100644
>> --- a/include/linux/f2fs_fs.h
>> +++ b/include/linux/f2fs_fs.h
>> @@ -81,6 +81,7 @@ enum stop_cp_reason {
>>   	STOP_CP_REASON_CORRUPTED_SUMMARY,
>>   	STOP_CP_REASON_UPDATE_INODE,
>>   	STOP_CP_REASON_FLUSH_FAIL,
>> +	STOP_CP_REASON_OUTOF_RAGNE,
>>   	STOP_CP_REASON_MAX,
>>   };
>>   
>> -- 
>> 1.9.1

Re: [f2fs-dev] [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[patchwork-bot+f2fs]

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Wed, 7 Feb 2024 10:01:00 +0800 you wrote:
> A panic issue happened in a reboot test in small capacity device
> as following:
> 1.The device size is 64MB, and main area has 24 segments, and
> CONFIG_F2FS_CHECK_FS is not enabled.
> 2.There is no any free segments left shown in free_segmap_info,
> then another write request cause get_new_segment get a out-of-bound
> segment with segno 24.
> 3.panic happen in update_sit_entry because access invalid bitmap
> pointer.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,v2,1/4] f2fs: correct counting methods of free_segments in __set_inuse
    https://git.kernel.org/jaegeuk/f2fs/c/8bac4167fd14
  - [f2fs-dev,v2,2/4] f2fs: fix panic issue in update_sit_entry
    https://git.kernel.org/jaegeuk/f2fs/c/4acac2bf18d6
  - [f2fs-dev,v2,3/4] f2fs: enhance judgment conditions of GET_SEGNO
    (no matching commit)
  - [f2fs-dev,v2,4/4] f2fs: stop checkpoint when get a out-of-bounds segment
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Chao Yu]

On 2024/2/7 10:01, Zhiguo Niu wrote:
> A panic issue happened in a reboot test in small capacity device
> as following:
> 1.The device size is 64MB, and main area has 24 segments, and
> CONFIG_F2FS_CHECK_FS is not enabled.
> 2.There is no any free segments left shown in free_segmap_info,
> then another write request cause get_new_segment get a out-of-bound
> segment with segno 24.
> 3.panic happen in update_sit_entry because access invalid bitmap
> pointer.

Zhiguo,

Can you please try below patch to see whether it can fix your problem?

https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org

Thanks,

> 
> More detail shown in following patch sets.
> The three patches are splited here because the modifications are
> relatively independent and more readable.
> 
> ---
> Changes of v2: stop checkpoint when get a out-of-bound segment
> ---
> 
> Zhiguo Niu (4):
>    f2fs: correct counting methods of free_segments in __set_inuse
>    f2fs: fix panic issue in update_sit_entry
>    f2fs: enhance judgment conditions of GET_SEGNO
>    f2fs: stop checkpoint when get a out-of-bounds segment
> 
>   fs/f2fs/file.c          |  7 ++++++-
>   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
>   fs/f2fs/segment.h       |  7 ++++---
>   include/linux/f2fs_fs.h |  1 +
>   4 files changed, 27 insertions(+), 9 deletions(-)
> 

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Chao Yu]

On 2024/2/23 10:01, Zhiguo Niu wrote:
> 
> 
> On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> 
>     On 2024/2/7 10:01, Zhiguo Niu wrote:
>      > A panic issue happened in a reboot test in small capacity device
>      > as following:
>      > 1.The device size is 64MB, and main area has 24 segments, and
>      > CONFIG_F2FS_CHECK_FS is not enabled.
>      > 2.There is no any free segments left shown in free_segmap_info,
>      > then another write request cause get_new_segment get a out-of-bound
>      > segment with segno 24.
>      > 3.panic happen in update_sit_entry because access invalid bitmap
>      > pointer.
> 
>     Zhiguo,
> 
>     Can you please try below patch to see whether it can fix your problem?
> 
>     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> 
>     Thanks,
> 
> 
> Dear Chao,
> I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.

Zhiguo, thank you!

BTW, I've tested this patch for a while, and it looks there is no issue w/
FAULT_NO_SEGMENT fault injection is on.

> btw, Why can’t I see this patch on your branch^^?
> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>

Too lazy to push patches in time, will do it in this weekend. :P

> thanks！
> 
> 
>      >
>      > More detail shown in following patch sets.
>      > The three patches are splited here because the modifications are
>      > relatively independent and more readable.
>      >
>      > ---
>      > Changes of v2: stop checkpoint when get a out-of-bound segment
>      > ---
>      >
>      > Zhiguo Niu (4):
>      >    f2fs: correct counting methods of free_segments in __set_inuse
>      >    f2fs: fix panic issue in update_sit_entry
>      >    f2fs: enhance judgment conditions of GET_SEGNO
>      >    f2fs: stop checkpoint when get a out-of-bounds segment
>      >
>      >   fs/f2fs/file.c          |  7 ++++++-
>      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
>      >   fs/f2fs/segment.h       |  7 ++++---
>      >   include/linux/f2fs_fs.h |  1 +
>      >   4 files changed, 27 insertions(+), 9 deletions(-)
>      >
> 

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Zhiguo Niu]

Dear Chao,

On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/2/23 10:01, Zhiguo Niu wrote:
> >
> >
> > On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> >
> >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> >      > A panic issue happened in a reboot test in small capacity device
> >      > as following:
> >      > 1.The device size is 64MB, and main area has 24 segments, and
> >      > CONFIG_F2FS_CHECK_FS is not enabled.
> >      > 2.There is no any free segments left shown in free_segmap_info,
> >      > then another write request cause get_new_segment get a out-of-bound
> >      > segment with segno 24.
> >      > 3.panic happen in update_sit_entry because access invalid bitmap
> >      > pointer.
> >
> >     Zhiguo,
> >
> >     Can you please try below patch to see whether it can fix your problem?
> >
> >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> >
> >     Thanks,
> >
> >
> > Dear Chao,
> > I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
>
> Zhiguo, thank you!

We tested this patch  this weekend on the previous version with
problem, and it can not reproduce panic issues,
so this patch should fix the original issue.
thanks！

>
> BTW, I've tested this patch for a while, and it looks there is no issue w/
> FAULT_NO_SEGMENT fault injection is on.
>
> > btw, Why can’t I see this patch on your branch^^?
> > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
>
> Too lazy to push patches in time, will do it in this weekend. :P
>
> > thanks！
> >
> >
> >      >
> >      > More detail shown in following patch sets.
> >      > The three patches are splited here because the modifications are
> >      > relatively independent and more readable.
> >      >
> >      > ---
> >      > Changes of v2: stop checkpoint when get a out-of-bound segment
> >      > ---
> >      >
> >      > Zhiguo Niu (4):
> >      >    f2fs: correct counting methods of free_segments in __set_inuse
> >      >    f2fs: fix panic issue in update_sit_entry
> >      >    f2fs: enhance judgment conditions of GET_SEGNO
> >      >    f2fs: stop checkpoint when get a out-of-bounds segment
> >      >
> >      >   fs/f2fs/file.c          |  7 ++++++-
> >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> >      >   fs/f2fs/segment.h       |  7 ++++---
> >      >   include/linux/f2fs_fs.h |  1 +
> >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> >      >
> >

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Chao Yu]

On 2024/2/26 11:25, Zhiguo Niu wrote:
> Dear Chao,
> 
> On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/2/23 10:01, Zhiguo Niu wrote:
>>>
>>>
>>> On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
>>>
>>>      On 2024/2/7 10:01, Zhiguo Niu wrote:
>>>       > A panic issue happened in a reboot test in small capacity device
>>>       > as following:
>>>       > 1.The device size is 64MB, and main area has 24 segments, and
>>>       > CONFIG_F2FS_CHECK_FS is not enabled.
>>>       > 2.There is no any free segments left shown in free_segmap_info,
>>>       > then another write request cause get_new_segment get a out-of-bound
>>>       > segment with segno 24.
>>>       > 3.panic happen in update_sit_entry because access invalid bitmap
>>>       > pointer.
>>>
>>>      Zhiguo,
>>>
>>>      Can you please try below patch to see whether it can fix your problem?
>>>
>>>      https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
>>>
>>>      Thanks,
>>>
>>>
>>> Dear Chao,
>>> I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
>>
>> Zhiguo, thank you!
> 
> We tested this patch  this weekend on the previous version with
> problem, and it can not reproduce panic issues,
> so this patch should fix the original issue.

Zhiguo,

Thanks a lot for the test!

Do you mind replying to original patch below tag?

Tested-by: Zhiguo Niu <zhiguo.niu@unisoc.com>

Thanks,

> thanks！
> 
>>
>> BTW, I've tested this patch for a while, and it looks there is no issue w/
>> FAULT_NO_SEGMENT fault injection is on.
>>
>>> btw, Why can’t I see this patch on your branch^^?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
>>
>> Too lazy to push patches in time, will do it in this weekend. :P
>>
>>> thanks！
>>>
>>>
>>>       >
>>>       > More detail shown in following patch sets.
>>>       > The three patches are splited here because the modifications are
>>>       > relatively independent and more readable.
>>>       >
>>>       > ---
>>>       > Changes of v2: stop checkpoint when get a out-of-bound segment
>>>       > ---
>>>       >
>>>       > Zhiguo Niu (4):
>>>       >    f2fs: correct counting methods of free_segments in __set_inuse
>>>       >    f2fs: fix panic issue in update_sit_entry
>>>       >    f2fs: enhance judgment conditions of GET_SEGNO
>>>       >    f2fs: stop checkpoint when get a out-of-bounds segment
>>>       >
>>>       >   fs/f2fs/file.c          |  7 ++++++-
>>>       >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
>>>       >   fs/f2fs/segment.h       |  7 ++++---
>>>       >   include/linux/f2fs_fs.h |  1 +
>>>       >   4 files changed, 27 insertions(+), 9 deletions(-)
>>>       >
>>>

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Jaegeuk Kim]

On 02/26, Zhiguo Niu wrote:
> Dear Chao,
> 
> On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
> >
> > On 2024/2/23 10:01, Zhiguo Niu wrote:
> > >
> > >
> > > On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> > >
> > >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> > >      > A panic issue happened in a reboot test in small capacity device
> > >      > as following:
> > >      > 1.The device size is 64MB, and main area has 24 segments, and
> > >      > CONFIG_F2FS_CHECK_FS is not enabled.
> > >      > 2.There is no any free segments left shown in free_segmap_info,
> > >      > then another write request cause get_new_segment get a out-of-bound
> > >      > segment with segno 24.
> > >      > 3.panic happen in update_sit_entry because access invalid bitmap
> > >      > pointer.
> > >
> > >     Zhiguo,
> > >
> > >     Can you please try below patch to see whether it can fix your problem?
> > >
> > >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> > >
> > >     Thanks,
> > >
> > >
> > > Dear Chao,
> > > I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
> >
> > Zhiguo, thank you!
> 
> We tested this patch  this weekend on the previous version with
> problem, and it can not reproduce panic issues,
> so this patch should fix the original issue.
> thanks！

Hey, do you guys please point out which patches were tested without what?
IOWs, which patches should I remove and keep Chao's patch?

> 
> >
> > BTW, I've tested this patch for a while, and it looks there is no issue w/
> > FAULT_NO_SEGMENT fault injection is on.
> >
> > > btw, Why can’t I see this patch on your branch^^?
> > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
> >
> > Too lazy to push patches in time, will do it in this weekend. :P
> >
> > > thanks！
> > >
> > >
> > >      >
> > >      > More detail shown in following patch sets.
> > >      > The three patches are splited here because the modifications are
> > >      > relatively independent and more readable.
> > >      >
> > >      > ---
> > >      > Changes of v2: stop checkpoint when get a out-of-bound segment
> > >      > ---
> > >      >
> > >      > Zhiguo Niu (4):
> > >      >    f2fs: correct counting methods of free_segments in __set_inuse
> > >      >    f2fs: fix panic issue in update_sit_entry
> > >      >    f2fs: enhance judgment conditions of GET_SEGNO
> > >      >    f2fs: stop checkpoint when get a out-of-bounds segment
> > >      >
> > >      >   fs/f2fs/file.c          |  7 ++++++-
> > >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> > >      >   fs/f2fs/segment.h       |  7 ++++---
> > >      >   include/linux/f2fs_fs.h |  1 +
> > >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> > >      >
> > >

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Zhiguo Niu]

On Tue, Feb 27, 2024 at 9:13 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>
> On 02/26, Zhiguo Niu wrote:
> > Dear Chao,
> >
> > On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
> > >
> > > On 2024/2/23 10:01, Zhiguo Niu wrote:
> > > >
> > > >
> > > > On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> > > >
> > > >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> > > >      > A panic issue happened in a reboot test in small capacity device
> > > >      > as following:
> > > >      > 1.The device size is 64MB, and main area has 24 segments, and
> > > >      > CONFIG_F2FS_CHECK_FS is not enabled.
> > > >      > 2.There is no any free segments left shown in free_segmap_info,
> > > >      > then another write request cause get_new_segment get a out-of-bound
> > > >      > segment with segno 24.
> > > >      > 3.panic happen in update_sit_entry because access invalid bitmap
> > > >      > pointer.
> > > >
> > > >     Zhiguo,
> > > >
> > > >     Can you please try below patch to see whether it can fix your problem?
> > > >
> > > >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> > > >
> > > >     Thanks,
> > > >
> > > >
> > > > Dear Chao,
> > > > I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
> > >
> > > Zhiguo, thank you!
> >
> > We tested this patch  this weekend on the previous version with
> > problem, and it can not reproduce panic issues,
> > so this patch should fix the original issue.
> > thanks！
>
Dear Jaegeuk,
> Hey, do you guys please point out which patches were tested without what?
This problem occurred during our platform stability testing.
it can be fixed by my  this patch set, mainly be fixed by:
f2fs: fix panic issue in update_sit_entry & f2fs: enhance judgment
conditions of GET_SEGNO
and Chao's patch can also fix this problems testing without my patch
> IOWs, which patches should I remove and keep Chao's patch?
I think chao's patch is more reasonable, it does error handling more complete.
but my patch just do some sanity check for return value of GET_SEGNO
Same as other codes(update_segment_mtime)
and i think it also needed except this part:

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 3bf2ce46fa0907..bb22feeae1cfcb 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct
f2fs_sb_info *sbi,
(GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & (BLKS_PER_SEG(sbi) - 1))
#define GET_SEGNO(sbi, blk_addr) \
- ((!__is_valid_data_blkaddr(blk_addr)) ? \
+ ((!__is_valid_data_blkaddr(blk_addr) || \
+ !f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ? \
NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
#define CAP_BLKS_PER_SEC(sbi)
because Chao's patch let new_addr=null_addr when  get_new_segment
returns NOSPACE,
so I think this can be reverted and it also saves code running time.
How about Chao's opinions?
thanks!
>
> >
> > >
> > > BTW, I've tested this patch for a while, and it looks there is no issue w/
> > > FAULT_NO_SEGMENT fault injection is on.
> > >
> > > > btw, Why can’t I see this patch on your branch^^?
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
> > >
> > > Too lazy to push patches in time, will do it in this weekend. :P
> > >
> > > > thanks！
> > > >
> > > >
> > > >      >
> > > >      > More detail shown in following patch sets.
> > > >      > The three patches are splited here because the modifications are
> > > >      > relatively independent and more readable.
> > > >      >
> > > >      > ---
> > > >      > Changes of v2: stop checkpoint when get a out-of-bound segment
> > > >      > ---
> > > >      >
> > > >      > Zhiguo Niu (4):
> > > >      >    f2fs: correct counting methods of free_segments in __set_inuse
> > > >      >    f2fs: fix panic issue in update_sit_entry
> > > >      >    f2fs: enhance judgment conditions of GET_SEGNO
> > > >      >    f2fs: stop checkpoint when get a out-of-bounds segment
> > > >      >
> > > >      >   fs/f2fs/file.c          |  7 ++++++-
> > > >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> > > >      >   fs/f2fs/segment.h       |  7 ++++---
> > > >      >   include/linux/f2fs_fs.h |  1 +
> > > >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> > > >      >
> > > >

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Jaegeuk Kim]

On 02/27, Zhiguo Niu wrote:
> On Tue, Feb 27, 2024 at 9:13 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
> >
> > On 02/26, Zhiguo Niu wrote:
> > > Dear Chao,
> > >
> > > On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
> > > >
> > > > On 2024/2/23 10:01, Zhiguo Niu wrote:
> > > > >
> > > > >
> > > > > On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> > > > >
> > > > >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> > > > >      > A panic issue happened in a reboot test in small capacity device
> > > > >      > as following:
> > > > >      > 1.The device size is 64MB, and main area has 24 segments, and
> > > > >      > CONFIG_F2FS_CHECK_FS is not enabled.
> > > > >      > 2.There is no any free segments left shown in free_segmap_info,
> > > > >      > then another write request cause get_new_segment get a out-of-bound
> > > > >      > segment with segno 24.
> > > > >      > 3.panic happen in update_sit_entry because access invalid bitmap
> > > > >      > pointer.
> > > > >
> > > > >     Zhiguo,
> > > > >
> > > > >     Can you please try below patch to see whether it can fix your problem?
> > > > >
> > > > >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> > > > >
> > > > >     Thanks,
> > > > >
> > > > >
> > > > > Dear Chao,
> > > > > I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
> > > >
> > > > Zhiguo, thank you!
> > >
> > > We tested this patch  this weekend on the previous version with
> > > problem, and it can not reproduce panic issues,
> > > so this patch should fix the original issue.
> > > thanks！
> >
> Dear Jaegeuk,
> > Hey, do you guys please point out which patches were tested without what?
> This problem occurred during our platform stability testing.
> it can be fixed by my  this patch set, mainly be fixed by:
> f2fs: fix panic issue in update_sit_entry & f2fs: enhance judgment
> conditions of GET_SEGNO
> and Chao's patch can also fix this problems testing without my patch
> > IOWs, which patches should I remove and keep Chao's patch?
> I think chao's patch is more reasonable, it does error handling more complete.
> but my patch just do some sanity check for return value of GET_SEGNO
> Same as other codes(update_segment_mtime)
> and i think it also needed except this part:

Thanks for confirmation. It seems it'd be better to revert yours and apply
Chao's patch first. If you think there's something to improve on top of it,
could you please send another patch afterwards?

> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 3bf2ce46fa0907..bb22feeae1cfcb 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct
> f2fs_sb_info *sbi,
> (GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & (BLKS_PER_SEG(sbi) - 1))
> #define GET_SEGNO(sbi, blk_addr) \
> - ((!__is_valid_data_blkaddr(blk_addr)) ? \
> + ((!__is_valid_data_blkaddr(blk_addr) || \
> + !f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ? \
> NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
> GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
> #define CAP_BLKS_PER_SEC(sbi)
> because Chao's patch let new_addr=null_addr when  get_new_segment
> returns NOSPACE,
> so I think this can be reverted and it also saves code running time.
> How about Chao's opinions?
> thanks!
> >
> > >
> > > >
> > > > BTW, I've tested this patch for a while, and it looks there is no issue w/
> > > > FAULT_NO_SEGMENT fault injection is on.
> > > >
> > > > > btw, Why can’t I see this patch on your branch^^?
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
> > > >
> > > > Too lazy to push patches in time, will do it in this weekend. :P
> > > >
> > > > > thanks！
> > > > >
> > > > >
> > > > >      >
> > > > >      > More detail shown in following patch sets.
> > > > >      > The three patches are splited here because the modifications are
> > > > >      > relatively independent and more readable.
> > > > >      >
> > > > >      > ---
> > > > >      > Changes of v2: stop checkpoint when get a out-of-bound segment
> > > > >      > ---
> > > > >      >
> > > > >      > Zhiguo Niu (4):
> > > > >      >    f2fs: correct counting methods of free_segments in __set_inuse
> > > > >      >    f2fs: fix panic issue in update_sit_entry
> > > > >      >    f2fs: enhance judgment conditions of GET_SEGNO
> > > > >      >    f2fs: stop checkpoint when get a out-of-bounds segment
> > > > >      >
> > > > >      >   fs/f2fs/file.c          |  7 ++++++-
> > > > >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> > > > >      >   fs/f2fs/segment.h       |  7 ++++---
> > > > >      >   include/linux/f2fs_fs.h |  1 +
> > > > >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> > > > >      >
> > > > >

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Zhiguo Niu]

On Wed, Feb 28, 2024 at 1:18 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>
> On 02/27, Zhiguo Niu wrote:
> > On Tue, Feb 27, 2024 at 9:13 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
> > >
> > > On 02/26, Zhiguo Niu wrote:
> > > > Dear Chao,
> > > >
> > > > On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
> > > > >
> > > > > On 2024/2/23 10:01, Zhiguo Niu wrote:
> > > > > >
> > > > > >
> > > > > > On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> > > > > >
> > > > > >     On 2024/2/7 10:01, Zhiguo Niu wrote:
> > > > > >      > A panic issue happened in a reboot test in small capacity device
> > > > > >      > as following:
> > > > > >      > 1.The device size is 64MB, and main area has 24 segments, and
> > > > > >      > CONFIG_F2FS_CHECK_FS is not enabled.
> > > > > >      > 2.There is no any free segments left shown in free_segmap_info,
> > > > > >      > then another write request cause get_new_segment get a out-of-bound
> > > > > >      > segment with segno 24.
> > > > > >      > 3.panic happen in update_sit_entry because access invalid bitmap
> > > > > >      > pointer.
> > > > > >
> > > > > >     Zhiguo,
> > > > > >
> > > > > >     Can you please try below patch to see whether it can fix your problem?
> > > > > >
> > > > > >     https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
> > > > > >
> > > > > >     Thanks,
> > > > > >
> > > > > >
> > > > > > Dear Chao,
> > > > > > I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
> > > > >
> > > > > Zhiguo, thank you!
> > > >
> > > > We tested this patch  this weekend on the previous version with
> > > > problem, and it can not reproduce panic issues,
> > > > so this patch should fix the original issue.
> > > > thanks！
> > >
> > Dear Jaegeuk,
> > > Hey, do you guys please point out which patches were tested without what?
> > This problem occurred during our platform stability testing.
> > it can be fixed by my  this patch set, mainly be fixed by:
> > f2fs: fix panic issue in update_sit_entry & f2fs: enhance judgment
> > conditions of GET_SEGNO
> > and Chao's patch can also fix this problems testing without my patch
> > > IOWs, which patches should I remove and keep Chao's patch?
> > I think chao's patch is more reasonable, it does error handling more complete.
> > but my patch just do some sanity check for return value of GET_SEGNO
> > Same as other codes(update_segment_mtime)
> > and i think it also needed except this part:
>
> Thanks for confirmation. It seems it'd be better to revert yours and apply
> Chao's patch first. If you think there's something to improve on top of it,
> could you please send another patch afterwards?

OK, I think this two patches still needed
  f2fs: correct counting methods of free_segments in __set_inuse
  f2fs: fix panic issue in update_sit_entry
and I'll reorganize it
thanks
>
> >
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 3bf2ce46fa0907..bb22feeae1cfcb 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct
> > f2fs_sb_info *sbi,
> > (GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & (BLKS_PER_SEG(sbi) - 1))
> > #define GET_SEGNO(sbi, blk_addr) \
> > - ((!__is_valid_data_blkaddr(blk_addr)) ? \
> > + ((!__is_valid_data_blkaddr(blk_addr) || \
> > + !f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ? \
> > NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
> > GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
> > #define CAP_BLKS_PER_SEC(sbi)
> > because Chao's patch let new_addr=null_addr when  get_new_segment
> > returns NOSPACE,
> > so I think this can be reverted and it also saves code running time.
> > How about Chao's opinions?
> > thanks!
> > >
> > > >
> > > > >
> > > > > BTW, I've tested this patch for a while, and it looks there is no issue w/
> > > > > FAULT_NO_SEGMENT fault injection is on.
> > > > >
> > > > > > btw, Why can’t I see this patch on your branch^^?
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
> > > > >
> > > > > Too lazy to push patches in time, will do it in this weekend. :P
> > > > >
> > > > > > thanks！
> > > > > >
> > > > > >
> > > > > >      >
> > > > > >      > More detail shown in following patch sets.
> > > > > >      > The three patches are splited here because the modifications are
> > > > > >      > relatively independent and more readable.
> > > > > >      >
> > > > > >      > ---
> > > > > >      > Changes of v2: stop checkpoint when get a out-of-bound segment
> > > > > >      > ---
> > > > > >      >
> > > > > >      > Zhiguo Niu (4):
> > > > > >      >    f2fs: correct counting methods of free_segments in __set_inuse
> > > > > >      >    f2fs: fix panic issue in update_sit_entry
> > > > > >      >    f2fs: enhance judgment conditions of GET_SEGNO
> > > > > >      >    f2fs: stop checkpoint when get a out-of-bounds segment
> > > > > >      >
> > > > > >      >   fs/f2fs/file.c          |  7 ++++++-
> > > > > >      >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
> > > > > >      >   fs/f2fs/segment.h       |  7 ++++---
> > > > > >      >   include/linux/f2fs_fs.h |  1 +
> > > > > >      >   4 files changed, 27 insertions(+), 9 deletions(-)
> > > > > >      >
> > > > > >

Re: [PATCH v2 0/4] f2fs: fix panic issue in small capacity device

[Chao Yu]

On 2024/2/28 1:18, Jaegeuk Kim wrote:
> On 02/27, Zhiguo Niu wrote:
>> On Tue, Feb 27, 2024 at 9:13 AM Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>>>
>>> On 02/26, Zhiguo Niu wrote:
>>>> Dear Chao,
>>>>
>>>> On Fri, Feb 23, 2024 at 10:38 AM Chao Yu <chao@kernel.org> wrote:
>>>>>
>>>>> On 2024/2/23 10:01, Zhiguo Niu wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Feb 22, 2024 at 8:30 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
>>>>>>
>>>>>>      On 2024/2/7 10:01, Zhiguo Niu wrote:
>>>>>>       > A panic issue happened in a reboot test in small capacity device
>>>>>>       > as following:
>>>>>>       > 1.The device size is 64MB, and main area has 24 segments, and
>>>>>>       > CONFIG_F2FS_CHECK_FS is not enabled.
>>>>>>       > 2.There is no any free segments left shown in free_segmap_info,
>>>>>>       > then another write request cause get_new_segment get a out-of-bound
>>>>>>       > segment with segno 24.
>>>>>>       > 3.panic happen in update_sit_entry because access invalid bitmap
>>>>>>       > pointer.
>>>>>>
>>>>>>      Zhiguo,
>>>>>>
>>>>>>      Can you please try below patch to see whether it can fix your problem?
>>>>>>
>>>>>>      https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org <https://lore.kernel.org/linux-f2fs-devel/20240222121851.883141-3-chao@kernel.org>
>>>>>>
>>>>>>      Thanks,
>>>>>>
>>>>>>
>>>>>> Dear Chao,
>>>>>> I need to coordinate the testing resources. The previous testing has been stopped because it was fixed with the current patch. In addition, this requires stability testing to reproduce, so it will take a certain amount of time. If there is any situation, I will tell you in time.
>>>>>
>>>>> Zhiguo, thank you!
>>>>
>>>> We tested this patch  this weekend on the previous version with
>>>> problem, and it can not reproduce panic issues,
>>>> so this patch should fix the original issue.
>>>> thanks！
>>>
>> Dear Jaegeuk,
>>> Hey, do you guys please point out which patches were tested without what?
>> This problem occurred during our platform stability testing.
>> it can be fixed by my  this patch set, mainly be fixed by:
>> f2fs: fix panic issue in update_sit_entry & f2fs: enhance judgment
>> conditions of GET_SEGNO
>> and Chao's patch can also fix this problems testing without my patch
>>> IOWs, which patches should I remove and keep Chao's patch?
>> I think chao's patch is more reasonable, it does error handling more complete.
>> but my patch just do some sanity check for return value of GET_SEGNO
>> Same as other codes(update_segment_mtime)
>> and i think it also needed except this part:
> 
> Thanks for confirmation. It seems it'd be better to revert yours and apply
> Chao's patch first. If you think there's something to improve on top of it,
> could you please send another patch afterwards?

Agreed.

Thanks,

> 
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 3bf2ce46fa0907..bb22feeae1cfcb 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -96,7 +96,8 @@ static inline void sanity_check_seg_type(struct
>> f2fs_sb_info *sbi,
>> (GET_SEGOFF_FROM_SEG0(sbi, blk_addr) & (BLKS_PER_SEG(sbi) - 1))
>> #define GET_SEGNO(sbi, blk_addr) \
>> - ((!__is_valid_data_blkaddr(blk_addr)) ? \
>> + ((!__is_valid_data_blkaddr(blk_addr) || \
>> + !f2fs_is_valid_blkaddr(sbi, blk_addr, DATA_GENERIC)) ? \
>> NULL_SEGNO : GET_L2R_SEGNO(FREE_I(sbi), \
>> GET_SEGNO_FROM_SEG0(sbi, blk_addr)))
>> #define CAP_BLKS_PER_SEC(sbi)
>> because Chao's patch let new_addr=null_addr when  get_new_segment
>> returns NOSPACE,
>> so I think this can be reverted and it also saves code running time.
>> How about Chao's opinions?
>> thanks!
>>>
>>>>
>>>>>
>>>>> BTW, I've tested this patch for a while, and it looks there is no issue w/
>>>>> FAULT_NO_SEGMENT fault injection is on.
>>>>>
>>>>>> btw, Why can’t I see this patch on your branch^^?
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test <https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=dev-test>
>>>>>
>>>>> Too lazy to push patches in time, will do it in this weekend. :P
>>>>>
>>>>>> thanks！
>>>>>>
>>>>>>
>>>>>>       >
>>>>>>       > More detail shown in following patch sets.
>>>>>>       > The three patches are splited here because the modifications are
>>>>>>       > relatively independent and more readable.
>>>>>>       >
>>>>>>       > ---
>>>>>>       > Changes of v2: stop checkpoint when get a out-of-bound segment
>>>>>>       > ---
>>>>>>       >
>>>>>>       > Zhiguo Niu (4):
>>>>>>       >    f2fs: correct counting methods of free_segments in __set_inuse
>>>>>>       >    f2fs: fix panic issue in update_sit_entry
>>>>>>       >    f2fs: enhance judgment conditions of GET_SEGNO
>>>>>>       >    f2fs: stop checkpoint when get a out-of-bounds segment
>>>>>>       >
>>>>>>       >   fs/f2fs/file.c          |  7 ++++++-
>>>>>>       >   fs/f2fs/segment.c       | 21 ++++++++++++++++-----
>>>>>>       >   fs/f2fs/segment.h       |  7 ++++---
>>>>>>       >   include/linux/f2fs_fs.h |  1 +
>>>>>>       >   4 files changed, 27 insertions(+), 9 deletions(-)
>>>>>>       >
>>>>>>