[PATCH] f2fs: set prefree as free segments after clear prefree segments

[PATCH] f2fs: set prefree as free segments after clear prefree segments

[Yangtao Li]

For now, when do_checkpoint fails, the prefree bitmap is not cleared,
but these segments are already in the free state. If these segments
are used, the segments in use will be reset to the free state when
f2fs_clear_prefree_segments is called next time.

So move set_prefree_as_free_segments after clear_prefree_segments.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/segment.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c2866561263e..ea1334e17a0d 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1945,20 +1945,6 @@ void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi)
 		release_discard_addr(entry);
 }
 
-/*
- * Should call f2fs_clear_prefree_segments after checkpoint is done.
- */
-static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
-{
-	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
-	unsigned int segno;
-
-	mutex_lock(&dirty_i->seglist_lock);
-	for_each_set_bit(segno, dirty_i->dirty_segmap[PRE], MAIN_SEGS(sbi))
-		__set_test_and_free(sbi, segno, false);
-	mutex_unlock(&dirty_i->seglist_lock);
-}
-
 void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
 						struct cp_control *cpc)
 {
@@ -1993,6 +1979,7 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
 		for (i = start; i < end; i++) {
 			if (test_and_clear_bit(i, prefree_map))
 				dirty_i->nr_dirty[PRE]--;
+			__set_test_and_free(sbi, i, false);
 		}
 
 		if (!f2fs_realtime_discard_enable(sbi))
@@ -4188,8 +4175,6 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 		cpc->trim_start = trim_start;
 	}
 	up_write(&sit_i->sentry_lock);
-
-	set_prefree_as_free_segments(sbi);
 }
 
 static int build_sit_info(struct f2fs_sb_info *sbi)
-- 
2.25.1

Re: [f2fs-dev] [PATCH] f2fs: set prefree as free segments after clear prefree segments

[Chao Yu]

On 2021/4/22 20:59, Yangtao Li wrote:
> For now, when do_checkpoint fails, the prefree bitmap is not cleared,
> but these segments are already in the free state. If these segments
> are used, the segments in use will be reset to the free state when
> f2fs_clear_prefree_segments is called next time.
> 
> So move set_prefree_as_free_segments after clear_prefree_segments.

That's not correct.

/*
  * Should call f2fs_clear_prefree_segments after checkpoint is done.
  */
static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)

Comments above set_prefree_as_free_segments() should have told you
the rule, otherwise if checkpoint failed, valid data in last valid
checkpoint could be corrupted after segment reuse.

Thanks,

> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/segment.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index c2866561263e..ea1334e17a0d 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1945,20 +1945,6 @@ void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi)
>   		release_discard_addr(entry);
>   }
>   
> -/*
> - * Should call f2fs_clear_prefree_segments after checkpoint is done.
> - */
> -static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
> -{
> -	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> -	unsigned int segno;
> -
> -	mutex_lock(&dirty_i->seglist_lock);
> -	for_each_set_bit(segno, dirty_i->dirty_segmap[PRE], MAIN_SEGS(sbi))
> -		__set_test_and_free(sbi, segno, false);
> -	mutex_unlock(&dirty_i->seglist_lock);
> -}
> -
>   void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
>   						struct cp_control *cpc)
>   {
> @@ -1993,6 +1979,7 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
>   		for (i = start; i < end; i++) {
>   			if (test_and_clear_bit(i, prefree_map))
>   				dirty_i->nr_dirty[PRE]--;
> +			__set_test_and_free(sbi, i, false);
>   		}
>   
>   		if (!f2fs_realtime_discard_enable(sbi))
> @@ -4188,8 +4175,6 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>   		cpc->trim_start = trim_start;
>   	}
>   	up_write(&sit_i->sentry_lock);
> -
> -	set_prefree_as_free_segments(sbi);
>   }
>   
>   static int build_sit_info(struct f2fs_sb_info *sbi)
> 

Re:Re: [f2fs-dev] [PATCH] f2fs: set prefree as free segments after clear prefree segments

[李扬韬]

HI Chao,
 
>> For now, when do_checkpoint fails, the prefree bitmap is not cleared,
>> but these segments are already in the free state. If these segments
>> are used, the segments in use will be reset to the free state when
>> f2fs_clear_prefree_segments is called next time.
>> 
>> So move set_prefree_as_free_segments after clear_prefree_segments.
>
>That's not correct.
>
>/*
>  * Should call f2fs_clear_prefree_segments after checkpoint is done.
>  */
>static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
>
>Comments above set_prefree_as_free_segments() should have told you
>the rule, otherwise if checkpoint failed, valid data in last valid
>checkpoint could be corrupted after segment reuse.

For do_checkpoint sucess:

f2fs_write_checkpoint
->f2fs_flush_sit_entries
    ->set_prefree_as_free_segments
->do_checkpoint
->f2fs_clear_prefree_segments


Calling set_prefree_as_free_segments when do_checkpoint fails,
seems to be incorrect. I think clear free bitmap should be after
clear prefree bitmap.

For do_checkpoint fail:

f2fs_write_checkpoint
->f2fs_flush_sit_entries
    ->set_prefree_as_free_segments
->do_checkpoint
->f2fs_release_discard_addrs

The prefree bitmap is not cleared, but free bitmap is cleared，which means
we can use these segments that are marked as free. When the free segments
is used, the next f2fs_clear_prefree_segments will mark prefree as free again,
causing some problem.

With this patch, for do_checkpoint fail:

f2fs_write_checkpoint
->f2fs_flush_sit_entries
->do_checkpoint
->f2fs_release_discard_addrs

At this time, we did not mark prefree as free segments, so these segments will not be used.

Thx

Re: [f2fs-dev] [PATCH] f2fs: set prefree as free segments after clear prefree segments

[Chao Yu]

Hi Yangtao,

On 2021/4/23 10:40, 李扬韬 wrote:
> HI Chao,
>   
>>> For now, when do_checkpoint fails, the prefree bitmap is not cleared,
>>> but these segments are already in the free state. If these segments
>>> are used, the segments in use will be reset to the free state when
>>> f2fs_clear_prefree_segments is called next time.
>>>
>>> So move set_prefree_as_free_segments after clear_prefree_segments.
>>
>> That's not correct.
>>
>> /*
>>   * Should call f2fs_clear_prefree_segments after checkpoint is done.
>>   */
>> static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
>>
>> Comments above set_prefree_as_free_segments() should have told you
>> the rule, otherwise if checkpoint failed, valid data in last valid
>> checkpoint could be corrupted after segment reuse.

Oh, it seems I misunderstood what the patch did, please ignore above
comments.

> 
> For do_checkpoint sucess:
> 
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
>      ->set_prefree_as_free_segments
> ->do_checkpoint
> ->f2fs_clear_prefree_segments
> 
> 
> Calling set_prefree_as_free_segments when do_checkpoint fails,
> seems to be incorrect. I think clear free bitmap should be after
> clear prefree bitmap.
> 
> For do_checkpoint fail:
> 
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
>      ->set_prefree_as_free_segments
> ->do_checkpoint
> ->f2fs_release_discard_addrs
> 
> The prefree bitmap is not cleared, but free bitmap is cleared，which means
> we can use these segments that are marked as free. When the free segments
> is used, the next f2fs_clear_prefree_segments will mark prefree as free again,
> causing some problem.

Okay, I can understand that.

But the problem here is, after applying this patch, successful checkpoint
may record wrong free_segment value:

- f2fs_write_checkpoint
  - f2fs_flush_sit_entries
  - do_checkpoint
   - ckpt->free_segment_count = cpu_to_le32(free_segments(sbi));
  - f2fs_clear_prefree_segments
   - __set_test_and_free
    - free_i->free_segments++;

I guess for the case of do_checkpoint() fails, maybe we can reset
free segment to prefree status.

Thoughts?

Thanks,

> 
> With this patch, for do_checkpoint fail:
> 
> f2fs_write_checkpoint
> ->f2fs_flush_sit_entries
> ->do_checkpoint
> ->f2fs_release_discard_addrs
> 
> At this time, we did not mark prefree as free segments, so these segments will not be used.
> 
> Thx
> 

Re:Re: [f2fs-dev] [PATCH] f2fs: set prefree as free segments after clear prefree segments

[李扬韬]

HI Chao,
>
>> For do_checkpoint sucess:
>> 
>> f2fs_write_checkpoint
>> ->f2fs_flush_sit_entries
>>      ->set_prefree_as_free_segments
>> ->do_checkpoint
>> ->f2fs_clear_prefree_segments
>> 
>> 
>> Calling set_prefree_as_free_segments when do_checkpoint fails,
>> seems to be incorrect. I think clear free bitmap should be after
>> clear prefree bitmap.
>> 
>> For do_checkpoint fail:
>> 
>> f2fs_write_checkpoint
>> ->f2fs_flush_sit_entries
>>      ->set_prefree_as_free_segments
>> ->do_checkpoint
>> ->f2fs_release_discard_addrs
>> 
>> The prefree bitmap is not cleared, but free bitmap is cleared，which means
>> we can use these segments that are marked as free. When the free segments
>> is used, the next f2fs_clear_prefree_segments will mark prefree as free again,
>> causing some problem.
>
>Okay, I can understand that.
>
>But the problem here is, after applying this patch, successful checkpoint
>may record wrong free_segment value:
>
>- f2fs_write_checkpoint
>  - f2fs_flush_sit_entries
>  - do_checkpoint
>   - ckpt->free_segment_count = cpu_to_le32(free_segments(sbi));
>  - f2fs_clear_prefree_segments
>   - __set_test_and_free
>    - free_i->free_segments++;

Yeah.

>
>I guess for the case of do_checkpoint() fails, maybe we can reset
>free segment to prefree status.
>
>Thoughts?
>

Reset free segment to prefree status or separate the number of statistics from the clear bitmap.

How about this one below？

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index be5415a0dbbc..0200af4d02ef 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1647,10 +1647,12 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	f2fs_save_inmem_curseg(sbi);
 
 	err = do_checkpoint(sbi, cpc);
-	if (err)
+	if (err) {
 		f2fs_release_discard_addrs(sbi);
-	else
+		f2fs_set_free_as_prefree_segments(sbi);
+	} else {
 		f2fs_clear_prefree_segments(sbi, cpc);
+	}
 
 	f2fs_restore_inmem_curseg(sbi);
 stop:
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index e2d302ae3a46..1618e9a74e89 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3369,6 +3369,7 @@ bool f2fs_is_checkpointed_data(struct f2fs_sb_info *sbi, block_t blkaddr);
 void f2fs_drop_discard_cmd(struct f2fs_sb_info *sbi);
 void f2fs_stop_discard_thread(struct f2fs_sb_info *sbi);
 bool f2fs_issue_discard_timeout(struct f2fs_sb_info *sbi);
+void f2fs_set_free_as_prefree_segments(struct f2fs_sb_info *sbi);
 void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
 					struct cp_control *cpc);
 void f2fs_dirty_to_prefree(struct f2fs_sb_info *sbi);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c2866561263e..334e499a0f43 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1959,6 +1959,19 @@ static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
 	mutex_unlock(&dirty_i->seglist_lock);
 }
 
+void f2fs_set_free_as_prefree_segments(struct f2fs_sb_info *sbi)
+{
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+	unsigned int segno;
+
+	mutex_lock(&dirty_i->seglist_lock);
+	for_each_set_bit(segno, dirty_i->dirty_segmap[PRE], MAIN_SEGS(sbi)) {
+		if (__set_test_and_inuse(sbi, segno))
+			test_and_clear_bit(segno, dirty_i->dirty_segmap[PRE]);
+	}
+	mutex_unlock(&dirty_i->seglist_lock);
+}
+
 void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
 						struct cp_control *cpc)
 {
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index e9a7a637d688..5da8d1100b87 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -487,19 +487,24 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
 	spin_unlock(&free_i->segmap_lock);
 }
 
-static inline void __set_test_and_inuse(struct f2fs_sb_info *sbi,
+static inline bool __set_test_and_inuse(struct f2fs_sb_info *sbi,
 		unsigned int segno)
 {
 	struct free_segmap_info *free_i = FREE_I(sbi);
 	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
+	bool is_inuse = false;
 
 	spin_lock(&free_i->segmap_lock);
 	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--;
+	} else {
+		is_inuse = true;
 	}
 	spin_unlock(&free_i->segmap_lock);
+
+	return is_inuse;
 }
 
 static inline void get_sit_bitmap(struct f2fs_sb_info *sbi,

 Thx