Re: [PATCH RFC] f2fs: add PRE2 to mark segments free to one checkpoint but obsolete to the other

[Chao Yu <chao@kernel.org>]

Hi Weichao,

On 2018/1/20 7:29, Weichao Guo wrote:
> Currently, we set prefree segments as free ones after writing
> a checkpoint, then believe the segments could be used safely.
> However, if a sudden power-off coming and the newer checkpoint
> corrupted due to hardware issues at the same time, we will try
> to use the old checkpoint and may face an inconsistent file
> system status.

IIUC, you mean:

1. write nodes into segment x;
2. write checkpoint A;
3. remove nodes in segment x;
4. write checkpoint B;
5. issue discard or write datas into segment x;
6. sudden power-cut

But after reboot, we found checkpoint B is corrupted due to hardware, and
then start to use checkpoint A, but nodes in segment x recorded as valid
data in checkpoint A has been overcovered in step 5), so we will encounter
inconsistent meta data, right?

Thanks,

> 
> How about add an PRE2 status for prefree segments, and make
> sure the segments could be used safely to both checkpoints?
> Or any better solutions? Or this is not a problem?
> 
> Look forward to your comments!
> 
> Signed-off-by: Weichao Guo <guoweichao@huawei.com>
> ---
>  fs/f2fs/gc.c      | 11 +++++++++--
>  fs/f2fs/segment.c | 21 ++++++++++++++++++---
>  fs/f2fs/segment.h |  6 ++++++
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 33e7969..153e3ea 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1030,7 +1030,12 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  		 * threshold, we can make them free by checkpoint. Then, we
>  		 * secure free segments which doesn't need fggc any more.
>  		 */
> -		if (prefree_segments(sbi)) {
> +		if (prefree_segments(sbi) || prefree2_segments(sbi)) {
> +			ret = write_checkpoint(sbi, &cpc);
> +			if (ret)
> +				goto stop;
> +		}
> +		if (has_not_enough_free_secs(sbi, 0, 0) && prefree2_segments(sbi)) {
>  			ret = write_checkpoint(sbi, &cpc);
>  			if (ret)
>  				goto stop;
> @@ -1063,8 +1068,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
>  			goto gc_more;
>  		}
>  
> -		if (gc_type == FG_GC)
> +		if (gc_type == FG_GC) {
> +			ret = write_checkpoint(sbi, &cpc);
>  			ret = write_checkpoint(sbi, &cpc);
> +		}
>  	}
>  stop:
>  	SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 2e8e054d..9dec445 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1606,7 +1606,7 @@ static void set_prefree_as_free_segments(struct f2fs_sb_info *sbi)
>  	unsigned int segno;
>  
>  	mutex_lock(&dirty_i->seglist_lock);
> -	for_each_set_bit(segno, dirty_i->dirty_segmap[PRE], MAIN_SEGS(sbi))
> +	for_each_set_bit(segno, dirty_i->dirty_segmap[PRE2], MAIN_SEGS(sbi))
>  		__set_test_and_free(sbi, segno);
>  	mutex_unlock(&dirty_i->seglist_lock);
>  }
> @@ -1617,13 +1617,17 @@ void clear_prefree_segments(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	struct list_head *head = &dcc->entry_list;
>  	struct discard_entry *entry, *this;
>  	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> -	unsigned long *prefree_map = dirty_i->dirty_segmap[PRE];
> +	unsigned long *prefree_map;
>  	unsigned int start = 0, end = -1;
>  	unsigned int secno, start_segno;
>  	bool force = (cpc->reason & CP_DISCARD);
> +	int phase = 0;
> +	enum dirty_type dirty_type = PRE2;
>  
>  	mutex_lock(&dirty_i->seglist_lock);
>  
> +next_step:
> +	prefree_map = dirty_i->dirty_segmap[dirty_type];
>  	while (1) {
>  		int i;
>  		start = find_next_bit(prefree_map, MAIN_SEGS(sbi), end + 1);
> @@ -1635,7 +1639,7 @@ void clear_prefree_segments(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		for (i = start; i < end; i++)
>  			clear_bit(i, prefree_map);
>  
> -		dirty_i->nr_dirty[PRE] -= end - start;
> +		dirty_i->nr_dirty[dirty_type] -= end - start;
>  
>  		if (!test_opt(sbi, DISCARD))
>  			continue;
> @@ -1663,6 +1667,16 @@ void clear_prefree_segments(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  		else
>  			end = start - 1;
>  	}
> +	if (phase == 0) {
> +		/* status change: PRE -> PRE2 */
> +		for_each_set_bit(segno, dirty_i->dirty_segmap[PRE], MAIN_SEGS(sbi))
> +			if (!test_and_set_bit(segno, prefree_map))
> +				dirty_i->nr_dirty[PRE2]++;
> +
> +		phase = 1;
> +		dirty_type = PRE;
> +		goto next_step;
> +	}
>  	mutex_unlock(&dirty_i->seglist_lock);
>  
>  	/* send small discards */
> @@ -2245,6 +2259,7 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
>  
>  	mutex_lock(&dirty_i->seglist_lock);
>  	__remove_dirty_segment(sbi, new_segno, PRE);
> +	__remove_dirty_segment(sbi, new_segno, PRE2);
>  	__remove_dirty_segment(sbi, new_segno, DIRTY);
>  	mutex_unlock(&dirty_i->seglist_lock);
>  
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 71a2aaa..f702726 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -263,6 +263,7 @@ enum dirty_type {
>  	DIRTY_COLD_NODE,	/* dirty segments assigned as cold node logs */
>  	DIRTY,			/* to count # of dirty segments */
>  	PRE,			/* to count # of entirely obsolete segments */
> +	PRE2,			/* to count # of the segments free to one checkpoint but obsolete to the other checkpoint */
>  	NR_DIRTY_TYPE
>  };
>  
> @@ -477,6 +478,11 @@ static inline unsigned int prefree_segments(struct f2fs_sb_info *sbi)
>  	return DIRTY_I(sbi)->nr_dirty[PRE];
>  }
>  
> +static inline unsigned int prefree2_segments(struct f2fs_sb_info *sbi)
> +{
> +	return DIRTY_I(sbi)->nr_dirty[PRE2];
> +}
> +
>  static inline unsigned int dirty_segments(struct f2fs_sb_info *sbi)
>  {
>  	return DIRTY_I(sbi)->nr_dirty[DIRTY_HOT_DATA] +
>