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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AFEABgD8Dpew4xXUTGbeMKpn.3.1619168535646.Hmail.frank.li@vivo.com>
Date:   Fri, 23 Apr 2021 17:02:15 +0800 (GMT+08:00)
From:   李扬韬 <frank.li@...o.com>
To:     Chao Yu <yuchao0@...wei.com>
Cc:     jaegeuk@...nel.org, chao@...nel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: 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