Messages in this thread | | | Subject | Re: [PATCH RFC] f2fs: fix to avoid selecting full segment w/ {AT,}SSR allocator | From | Chao Yu <> | Date | Wed, 24 Mar 2021 11:28:09 +0800 |
| |
On 2021/3/24 6:59, Jaegeuk Kim wrote: > On 03/19, Chao Yu wrote: >> On 2021/3/19 1:17, Jaegeuk Kim wrote: >>> On 02/20, Chao Yu wrote: >>>> In cp disabling mode, there could be a condition >>>> - target segment has 128 ckpt valid blocks >>>> - GC migrates 128 valid blocks to other segment (segment is still in >>>> dirty list) >>>> - GC migrates 384 blocks to target segment (segment has 128 cp_vblocks >>>> and 384 vblocks) >>>> - 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> >>>> --- >>>> fs/f2fs/f2fs.h | 1 + >>>> fs/f2fs/gc.c | 17 +++++++++++++---- >>>> fs/f2fs/segment.c | 20 ++++++++++++++++++++ >>>> 3 files changed, 34 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>> index ed7807103c8e..9c753eff0814 100644 >>>> --- a/fs/f2fs/f2fs.h >>>> +++ b/fs/f2fs/f2fs.h >>>> @@ -3376,6 +3376,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 86ba8ed0b8a7..a1d8062cdace 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++) >>>> @@ -736,6 +732,19 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi, >>>> if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap)) >>>> goto next; >>>> + if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { >>>> + /* >>>> + * to avoid selecting candidate which has below valid >>>> + * block distribution: >>>> + * partial blocks are valid and all left ones are valid >>>> + * in previous checkpoint. >>>> + */ >>>> + if (p.alloc_mode == SSR || p.alloc_mode == AT_SSR) { >>>> + if (!segment_has_free_slot(sbi, segno)) >>>> + goto next; >>> >>> Do we need to change this to check free_slot instead of get_ckpt_valid_blocks()? >> >> Jaegeuk, >> >> LFS was assigned only for GC case, in this case we are trying to select source >> section, rather than target segment for SSR/AT_SSR case, so we don't need to >> check free_slot. >> >> - f2fs_gc >> - __get_victim >> - get_victim(sbi, victim, gc_type, NO_CHECK_TYPE, LFS, 0); >> >>> >>> 732 if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED) && >>> 733 get_ckpt_valid_blocks(sbi, segno) && >>> 734 p.alloc_mode == LFS)) >> >> BTW, in LFS mode, GC wants to find source section rather than segment, so we >> should change to check valid ckpt blocks in every segment of targe section here? > > Alright. I refactored a bit on this patch with new one. Could you please take a look? > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=00152bd7cabd69b4615ebead823ff23887b0e0f7
I see, newly added comment looks good to me.
One more concern is commit title and commit message is out-of-update, I've revised it in v2:
https://lore.kernel.org/linux-f2fs-devel/20210324031828.67133-1-yuchao0@huawei.com/T/#u
Thanks,
> > Thanks, > >> >> Thanks, >> >>> >>> >>>> + } >>>> + } >>>> + >>>> if (is_atgc) { >>>> add_victim_entry(sbi, &p, segno); >>>> goto next; >>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>> index 2d5a82c4ca15..deaf57e13125 100644 >>>> --- a/fs/f2fs/segment.c >>>> +++ b/fs/f2fs/segment.c >>>> @@ -2650,6 +2650,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); >>>> + 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 >>>> -- >>>> 2.29.2 >>> . >>> > . >
| |