lkml.org 
[lkml]   [2021]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] f2fs: fix to avoid touching checkpointed data in get_victim()
    From
    Date
    On 2021/3/25 7:49, Jaegeuk Kim wrote:
    > On 03/24, Chao Yu wrote:
    >> In CP disabling mode, there are two issues when using LFS or SSR | AT_SSR
    >> mode to select victim:
    >>
    >> 1. LFS is set to find source section during GC, the victim should have
    >> no checkpointed data, since after GC, section could not be set free for
    >> reuse.
    >>
    >> Previously, we only check valid chpt blocks in current segment rather
    >> than section, fix it.
    >>
    >> 2. SSR | AT_SSR are set to find target segment for writes which can be
    >> fully filled by checkpointed and newly written blocks, we should never
    >> select such segment, otherwise it can cause panic or data corruption
    >> during allocation, potential case is described as below:
    >>
    >> a) target segment has 128 ckpt valid blocks
    >> b) GC migrates 'n' (n < 512) valid blocks to other segment (segment is
    >> still in dirty list)
    >> c) GC migrates '512 - n' blocks to target segment (segment has 'n'
    >> cp_vblocks and '512 - n' vblocks)
    >> d) If GC selects target segment via {AT,}SSR allocator, however there
    >> is no free space in targe segment.
    >>
    >> Fixes: 4354994f097d ("f2fs: checkpoint disabling")
    >> Fixes: 093749e296e2 ("f2fs: support age threshold based garbage collection")
    >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
    >> ---
    >> v2:
    >> - fix to check checkpointed data in section rather than segment for
    >> LFS mode.
    >> - update commit title and message.
    >> fs/f2fs/f2fs.h | 1 +
    >> fs/f2fs/gc.c | 28 ++++++++++++++++++++--------
    >> fs/f2fs/segment.c | 39 ++++++++++++++++++++++++---------------
    >> fs/f2fs/segment.h | 14 +++++++++++++-
    >> 4 files changed, 58 insertions(+), 24 deletions(-)
    >>
    >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
    >> index eb154d9cb063..29e634d08a27 100644
    >> --- a/fs/f2fs/f2fs.h
    >> +++ b/fs/f2fs/f2fs.h
    >> @@ -3387,6 +3387,7 @@ block_t f2fs_get_unusable_blocks(struct f2fs_sb_info *sbi);
    >> int f2fs_disable_cp_again(struct f2fs_sb_info *sbi, block_t unusable);
    >> void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
    >> int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
    >> +bool segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
    >> void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
    >> void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
    >> void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
    >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
    >> index d96acc6531f2..4d9616373a4a 100644
    >> --- a/fs/f2fs/gc.c
    >> +++ b/fs/f2fs/gc.c
    >> @@ -392,10 +392,6 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
    >> if (p->gc_mode == GC_AT &&
    >> get_valid_blocks(sbi, segno, true) == 0)
    >> return;
    >> -
    >> - if (p->alloc_mode == AT_SSR &&
    >> - get_seg_entry(sbi, segno)->ckpt_valid_blocks == 0)
    >> - return;
    >> }
    >>
    >> for (i = 0; i < sbi->segs_per_sec; i++)
    >> @@ -728,11 +724,27 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
    >>
    >> if (sec_usage_check(sbi, secno))
    >> goto next;
    >> +
    >> /* Don't touch checkpointed data */
    >> - if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED) &&
    >> - get_ckpt_valid_blocks(sbi, segno) &&
    >> - p.alloc_mode == LFS))
    >> - goto next;
    >> + if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
    >> + if (p.alloc_mode == LFS) {
    >> + /*
    >> + * LFS is set to find source section during GC.
    >> + * The victim should have no checkpointed data.
    >> + */
    >> + if (get_ckpt_valid_blocks(sbi, segno, true))
    >> + goto next;
    >> + } else {
    >> + /*
    >> + * SSR | AT_SSR are set to find target segment
    >> + * for writes which can be full by checkpointed
    >> + * and newly written blocks.
    >> + */
    >> + if (!segment_has_free_slot(sbi, segno))
    >> + goto next;
    >> + }
    >> + }
    >> +
    >> if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
    >> goto next;
    >>
    >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
    >> index 6e1a5f5657bf..f6a30856ceda 100644
    >> --- a/fs/f2fs/segment.c
    >> +++ b/fs/f2fs/segment.c
    >> @@ -865,7 +865,7 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
    >> mutex_lock(&dirty_i->seglist_lock);
    >>
    >> valid_blocks = get_valid_blocks(sbi, segno, false);
    >> - ckpt_valid_blocks = get_ckpt_valid_blocks(sbi, segno);
    >> + ckpt_valid_blocks = get_ckpt_valid_blocks(sbi, segno, false);
    >>
    >> if (valid_blocks == 0 && (!is_sbi_flag_set(sbi, SBI_CP_DISABLED) ||
    >> ckpt_valid_blocks == usable_blocks)) {
    >> @@ -950,7 +950,7 @@ static unsigned int get_free_segment(struct f2fs_sb_info *sbi)
    >> for_each_set_bit(segno, dirty_i->dirty_segmap[DIRTY], MAIN_SEGS(sbi)) {
    >> if (get_valid_blocks(sbi, segno, false))
    >> continue;
    >> - if (get_ckpt_valid_blocks(sbi, segno))
    >> + if (get_ckpt_valid_blocks(sbi, segno, false))
    >> continue;
    >> mutex_unlock(&dirty_i->seglist_lock);
    >> return segno;
    >> @@ -2643,6 +2643,26 @@ static void __refresh_next_blkoff(struct f2fs_sb_info *sbi,
    >> seg->next_blkoff++;
    >> }
    >>
    >> +bool segment_has_free_slot(struct f2fs_sb_info *sbi, int segno)
    >> +{
    >> + struct sit_info *sit = SIT_I(sbi);
    >> + struct seg_entry *se = get_seg_entry(sbi, segno);
    >> + int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
    >> + unsigned long *target_map = SIT_I(sbi)->tmp_map;
    >> + unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
    >> + unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
    >> + int i, pos;
    >> +
    >> + down_write(&sit->sentry_lock);
    >
    > Should remove this lock.
    > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev

    Oh, correct.

    BTW, could you please add 'f2fs_' prefix for segment_has_free_slot()
    like we did for other non-static symbols?

    Thanks,

    >
    >> + for (i = 0; i < entries; i++)
    >> + target_map[i] = ckpt_map[i] | cur_map[i];
    >> +
    >> + pos = __find_rev_next_zero_bit(target_map, sbi->blocks_per_seg, 0);
    >> + up_write(&sit->sentry_lock);
    >> +
    >> + return pos < sbi->blocks_per_seg;
    >> +}
    >> +
    >> /*
    >> * This function always allocates a used segment(from dirty seglist) by SSR
    >> * manner, so it should recover the existing segment information of valid blocks
    >> @@ -2913,19 +2933,8 @@ static void __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
    >> get_valid_blocks(sbi, curseg->segno, new_sec))
    >> goto alloc;
    >>
    >> - if (new_sec) {
    >> - unsigned int segno = START_SEGNO(curseg->segno);
    >> - int i;
    >> -
    >> - for (i = 0; i < sbi->segs_per_sec; i++, segno++) {
    >> - if (get_ckpt_valid_blocks(sbi, segno))
    >> - goto alloc;
    >> - }
    >> - } else {
    >> - if (!get_ckpt_valid_blocks(sbi, curseg->segno))
    >> - return;
    >> - }
    >> -
    >> + if (!get_ckpt_valid_blocks(sbi, curseg->segno, new_sec))
    >> + return;
    >> alloc:
    >> old_segno = curseg->segno;
    >> SIT_I(sbi)->s_ops->allocate_segment(sbi, type, true);
    >> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
    >> index 144980b62f9e..dab87ecba2b5 100644
    >> --- a/fs/f2fs/segment.h
    >> +++ b/fs/f2fs/segment.h
    >> @@ -359,8 +359,20 @@ static inline unsigned int get_valid_blocks(struct f2fs_sb_info *sbi,
    >> }
    >>
    >> static inline unsigned int get_ckpt_valid_blocks(struct f2fs_sb_info *sbi,
    >> - unsigned int segno)
    >> + unsigned int segno, bool use_section)
    >> {
    >> + if (use_section && __is_large_section(sbi)) {
    >> + unsigned int start_segno = START_SEGNO(segno);
    >> + unsigned int blocks = 0;
    >> + int i;
    >> +
    >> + for (i = 0; i < sbi->segs_per_sec; i++, start_segno++) {
    >> + struct seg_entry *se = get_seg_entry(sbi, start_segno);
    >> +
    >> + blocks += se->ckpt_valid_blocks;
    >> + }
    >> + return blocks;
    >> + }
    >> return get_seg_entry(sbi, segno)->ckpt_valid_blocks;
    >> }
    >>
    >> --
    >> 2.29.2
    > .
    >

    \
     
     \ /
      Last update: 2021-03-25 02:33    [W:2.540 / U:0.348 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site