| Message ID | 20230324071028.336982-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] f2fs: fix to trigger a checkpoint in the end of foreground garbage collection | expand |
On 03/24, Chao Yu wrote: > In order to reclaim free blocks in prefree sections before latter use. We were supposed to do checkpoint as is? > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/gc.c | 8 ++++++++ > fs/f2fs/segment.c | 1 + > 3 files changed, 10 insertions(+) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 53a005b420cf..b1515375cb4c 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1269,6 +1269,7 @@ struct f2fs_gc_control { > unsigned int victim_segno; /* target victim segment number */ > int init_gc_type; /* FG_GC or BG_GC */ > bool no_bg_gc; /* check the space and stop bg_gc */ > + bool reclaim_space; /* trigger checkpoint to reclaim space */ > bool should_migrate_blocks; /* should migrate blocks */ > bool err_gc_skipped; /* return EAGAIN if GC skipped */ > unsigned int nr_free_secs; /* # of free sections to do GC */ > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 2996d38aa89c..5a451d3d512d 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -132,6 +132,7 @@ static int gc_thread_func(void *data) > > gc_control.init_gc_type = sync_mode ? FG_GC : BG_GC; > gc_control.no_bg_gc = foreground; > + gc_control.reclaim_space = foreground; > gc_control.nr_free_secs = foreground ? 1 : 0; > > /* if return value is not zero, no victim was selected */ > @@ -1880,6 +1881,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > goto go_gc_more; > + > + /* > + * trigger a checkpoint in the end of foreground garbage > + * collection. > + */ > + if (gc_control->reclaim_space) > + ret = f2fs_write_checkpoint(sbi, &cpc); > goto stop; > } > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 6c11789da884..b62af2ae1685 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -421,6 +421,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need) > .victim_segno = NULL_SEGNO, > .init_gc_type = BG_GC, > .no_bg_gc = true, > + .reclaim_space = true, > .should_migrate_blocks = false, > .err_gc_skipped = false, > .nr_free_secs = 1 }; > -- > 2.25.1
On 2023/4/4 2:13, Jaegeuk Kim wrote: > On 03/24, Chao Yu wrote: >> In order to reclaim free blocks in prefree sections before latter use. > > We were supposed to do checkpoint as is? It seems commit 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") changed that logic? It caused that if has_not_enough_free_secs() returns false, it missed to call f2fs_write_checkpoint() before exit f2fs_gc()? @@ -1110,15 +1116,23 @@ gc_more: if (gc_type == FG_GC) sbi->cur_victim_sec = NULL_SEGNO; - if (!sync) { - if (has_not_enough_free_secs(sbi, sec_freed, 0)) { - if (skipped_round > MAX_SKIP_ATOMIC_COUNT && - skipped_round * 2 >= round) - f2fs_drop_inmem_pages_all(sbi, true); + if (sync) + goto stop; + + if (has_not_enough_free_secs(sbi, sec_freed, 0)) { + if (skipped_round <= MAX_SKIP_GC_COUNT || + skipped_round * 2 < round) { segno = NULL_SEGNO; goto gc_more; } + if (first_skipped < last_skipped && + (last_skipped - first_skipped) > + sbi->skipped_gc_rwsem) { + f2fs_drop_inmem_pages_all(sbi, true); + segno = NULL_SEGNO; + goto gc_more; + } if (gc_type == FG_GC) ret = f2fs_write_checkpoint(sbi, &cpc); } > >> >> Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/f2fs.h | 1 + >> fs/f2fs/gc.c | 8 ++++++++ >> fs/f2fs/segment.c | 1 + >> 3 files changed, 10 insertions(+) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 53a005b420cf..b1515375cb4c 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -1269,6 +1269,7 @@ struct f2fs_gc_control { >> unsigned int victim_segno; /* target victim segment number */ >> int init_gc_type; /* FG_GC or BG_GC */ >> bool no_bg_gc; /* check the space and stop bg_gc */ >> + bool reclaim_space; /* trigger checkpoint to reclaim space */ >> bool should_migrate_blocks; /* should migrate blocks */ >> bool err_gc_skipped; /* return EAGAIN if GC skipped */ >> unsigned int nr_free_secs; /* # of free sections to do GC */ >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >> index 2996d38aa89c..5a451d3d512d 100644 >> --- a/fs/f2fs/gc.c >> +++ b/fs/f2fs/gc.c >> @@ -132,6 +132,7 @@ static int gc_thread_func(void *data) >> >> gc_control.init_gc_type = sync_mode ? FG_GC : BG_GC; >> gc_control.no_bg_gc = foreground; >> + gc_control.reclaim_space = foreground; >> gc_control.nr_free_secs = foreground ? 1 : 0; >> >> /* if return value is not zero, no victim was selected */ >> @@ -1880,6 +1881,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >> (gc_type == FG_GC) ? sec_freed : 0, 0)) { >> if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) >> goto go_gc_more; >> + >> + /* >> + * trigger a checkpoint in the end of foreground garbage >> + * collection. >> + */ >> + if (gc_control->reclaim_space) >> + ret = f2fs_write_checkpoint(sbi, &cpc); >> goto stop; >> } >> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >> index 6c11789da884..b62af2ae1685 100644 >> --- a/fs/f2fs/segment.c >> +++ b/fs/f2fs/segment.c >> @@ -421,6 +421,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need) >> .victim_segno = NULL_SEGNO, >> .init_gc_type = BG_GC, >> .no_bg_gc = true, >> + .reclaim_space = true, >> .should_migrate_blocks = false, >> .err_gc_skipped = false, >> .nr_free_secs = 1 }; >> -- >> 2.25.1
Can we do like this? From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim <jaegeuk@kernel.org> Date: Tue, 4 Apr 2023 14:36:00 -0700 Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree sctions in time. Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") Signed-off-by: Chao Yu <chao@kernel.org> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/gc.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 56c53dbe05c9..f1d0dd9c5a6c 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) }; unsigned int skipped_round = 0, round = 0; unsigned int upper_secs; + bool stop_gc = false; trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, gc_control->nr_free_secs, @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) (gc_type == FG_GC) ? sec_freed : 0, 0)) { if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) goto go_gc_more; - goto stop; - } - - /* FG_GC stops GC by skip_count */ - if (gc_type == FG_GC) { + stop_gc = true; + } else if (gc_type == FG_GC) { + /* FG_GC stops GC by skip_count */ if (sbi->skipped_gc_rwsem) skipped_round++; round++; if (skipped_round > MAX_SKIP_GC_COUNT && - skipped_round * 2 >= round) { - ret = f2fs_write_checkpoint(sbi, &cpc); - goto stop; - } + skipped_round * 2 >= round) + stop_gc = true; } __get_secs_required(sbi, NULL, &upper_secs, NULL); @@ -1901,12 +1898,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) prefree_segments(sbi)) { ret = f2fs_write_checkpoint(sbi, &cpc); if (ret) - goto stop; + stop_gc = true; } go_gc_more: - segno = NULL_SEGNO; - goto gc_more; - + if (!stop_gc) { + segno = NULL_SEGNO; + goto gc_more; + } stop: SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0; SIT_I(sbi)->last_victim[FLUSH_DEVICE] = gc_control->victim_segno;
On 2023/4/5 5:39, Jaegeuk Kim wrote: > Can we do like this? > > From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Tue, 4 Apr 2023 14:36:00 -0700 > Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections > > We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree > sctions in time. > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > Signed-off-by: Chao Yu <chao@kernel.org> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/gc.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 56c53dbe05c9..f1d0dd9c5a6c 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > }; > unsigned int skipped_round = 0, round = 0; > unsigned int upper_secs; > + bool stop_gc = false; > > trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, > gc_control->nr_free_secs, > @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > goto go_gc_more; > - goto stop; > - } > - > - /* FG_GC stops GC by skip_count */ > - if (gc_type == FG_GC) { > + stop_gc = true; I guess below condition is for emergency recycle of prefree segments during foreground GC, in order to avoid exhausting free sections due to to many metadata allocation during CP. if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && prefree_segments(sbi)) { But for common case, free_sections() is close to reserved_segments(), and upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), so checkpoint may not be trggered as expected, IIUC. So it's fine to just trigger CP in the end of foreground garbage collection? One other concern is for those path as below: - disable_checkpoint - ioc_gc - ioc_gc_range - ioc_resize ... We've passed gc_type as FG_GC, but the demand here is to migrate block in time, rather than dirtying blocks, and callers don't expect checkpoint in f2fs_gc(), instead the callers will do the checkpoit as it needs. That means it's better to decouple FG_GC and write_checkpoint behavior, so I added another parameter .reclaim_space to just let f2fs_balance_fs() to trigger checkpoit in the end of f2fs_gc(). Thanks, > + } else if (gc_type == FG_GC) { > + /* FG_GC stops GC by skip_count */ > if (sbi->skipped_gc_rwsem) > skipped_round++; > round++; > if (skipped_round > MAX_SKIP_GC_COUNT && > - skipped_round * 2 >= round) { > - ret = f2fs_write_checkpoint(sbi, &cpc); > - goto stop; > - } > + skipped_round * 2 >= round) > + stop_gc = true; > } > > __get_secs_required(sbi, NULL, &upper_secs, NULL); > @@ -1901,12 +1898,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > prefree_segments(sbi)) { > ret = f2fs_write_checkpoint(sbi, &cpc); > if (ret) > - goto stop; > + stop_gc = true; > } > go_gc_more: > - segno = NULL_SEGNO; > - goto gc_more; > - > + if (!stop_gc) { > + segno = NULL_SEGNO; > + goto gc_more; > + } > stop: > SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0; > SIT_I(sbi)->last_victim[FLUSH_DEVICE] = gc_control->victim_segno;
On 04/05, Chao Yu wrote: > On 2023/4/5 5:39, Jaegeuk Kim wrote: > > Can we do like this? > > > > From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > Date: Tue, 4 Apr 2023 14:36:00 -0700 > > Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections > > > > We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree > > sctions in time. > > > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > > Signed-off-by: Chao Yu <chao@kernel.org> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/gc.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > index 56c53dbe05c9..f1d0dd9c5a6c 100644 > > --- a/fs/f2fs/gc.c > > +++ b/fs/f2fs/gc.c > > @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > }; > > unsigned int skipped_round = 0, round = 0; > > unsigned int upper_secs; > > + bool stop_gc = false; > > trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, > > gc_control->nr_free_secs, > > @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > goto go_gc_more; > > - goto stop; > > - } > > - > > - /* FG_GC stops GC by skip_count */ > > - if (gc_type == FG_GC) { > > + stop_gc = true; > > I guess below condition is for emergency recycle of prefree segments during > foreground GC, in order to avoid exhausting free sections due to to many > metadata allocation during CP. > > if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && > prefree_segments(sbi)) { > > But for common case, free_sections() is close to reserved_segments(), and > upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), > so checkpoint may not be trggered as expected, IIUC. > > So it's fine to just trigger CP in the end of foreground garbage collection? My major concern is to avoid unnecessary checkpointing given multiple FG_GC requests were pending in parallel. And, I don't want to add so many combination which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint automatically in the worst case scenario only. By the way, do we just need to call checkpoint here including FG_GC as well? 1832 1833 if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { 1834 /* 1835 * For example, if there are many prefree_segments below given 1836 * threshold, we can make them free by checkpoint. Then, we 1837 * secure free segments which doesn't need fggc any more. 1838 */ 1839 if (prefree_segments(sbi)) { 1840 ret = f2fs_write_checkpoint(sbi, &cpc); 1841 if (ret) 1842 goto stop; 1843 } 1844 if (has_not_enough_free_secs(sbi, 0, 0)) 1845 gc_type = FG_GC; 1846 } > > One other concern is for those path as below: > - disable_checkpoint > - ioc_gc > - ioc_gc_range > - ioc_resize > ... I think the upper caller should decide to call checkpoint, if they want to reclaim the prefree likewise f2fs_disable_checkpoint. > > We've passed gc_type as FG_GC, but the demand here is to migrate block in time, > rather than dirtying blocks, and callers don't expect checkpoint in f2fs_gc(), > instead the callers will do the checkpoit as it needs. > > That means it's better to decouple FG_GC and write_checkpoint behavior, so I > added another parameter .reclaim_space to just let f2fs_balance_fs() to trigger > checkpoit in the end of f2fs_gc(). > > Thanks, > > > + } else if (gc_type == FG_GC) { > > + /* FG_GC stops GC by skip_count */ > > if (sbi->skipped_gc_rwsem) > > skipped_round++; > > round++; > > if (skipped_round > MAX_SKIP_GC_COUNT && > > - skipped_round * 2 >= round) { > > - ret = f2fs_write_checkpoint(sbi, &cpc); > > - goto stop; > > - } > > + skipped_round * 2 >= round) > > + stop_gc = true; > > } > > __get_secs_required(sbi, NULL, &upper_secs, NULL); > > @@ -1901,12 +1898,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > prefree_segments(sbi)) { > > ret = f2fs_write_checkpoint(sbi, &cpc); > > if (ret) > > - goto stop; > > + stop_gc = true; > > } > > go_gc_more: > > - segno = NULL_SEGNO; > > - goto gc_more; > > - > > + if (!stop_gc) { > > + segno = NULL_SEGNO; > > + goto gc_more; > > + } > > stop: > > SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0; > > SIT_I(sbi)->last_victim[FLUSH_DEVICE] = gc_control->victim_segno;
On 2023/4/5 23:55, Jaegeuk Kim wrote: > On 04/05, Chao Yu wrote: >> On 2023/4/5 5:39, Jaegeuk Kim wrote: >>> Can we do like this? >>> >>> From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 >>> From: Jaegeuk Kim <jaegeuk@kernel.org> >>> Date: Tue, 4 Apr 2023 14:36:00 -0700 >>> Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections >>> >>> We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree >>> sctions in time. >>> >>> Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") >>> Signed-off-by: Chao Yu <chao@kernel.org> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/gc.c | 24 +++++++++++------------- >>> 1 file changed, 11 insertions(+), 13 deletions(-) >>> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>> index 56c53dbe05c9..f1d0dd9c5a6c 100644 >>> --- a/fs/f2fs/gc.c >>> +++ b/fs/f2fs/gc.c >>> @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>> }; >>> unsigned int skipped_round = 0, round = 0; >>> unsigned int upper_secs; >>> + bool stop_gc = false; >>> trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, >>> gc_control->nr_free_secs, >>> @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>> (gc_type == FG_GC) ? sec_freed : 0, 0)) { >>> if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) >>> goto go_gc_more; >>> - goto stop; >>> - } >>> - >>> - /* FG_GC stops GC by skip_count */ >>> - if (gc_type == FG_GC) { >>> + stop_gc = true; >> >> I guess below condition is for emergency recycle of prefree segments during >> foreground GC, in order to avoid exhausting free sections due to to many >> metadata allocation during CP. >> >> if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && >> prefree_segments(sbi)) { >> >> But for common case, free_sections() is close to reserved_segments(), and >> upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), >> so checkpoint may not be trggered as expected, IIUC. >> >> So it's fine to just trigger CP in the end of foreground garbage collection? > > My major concern is to avoid unnecessary checkpointing given multiple FG_GC > requests were pending in parallel. And, I don't want to add so many combination > which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint > automatically in the worst case scenario only. Alright. > > By the way, do we just need to call checkpoint here including FG_GC as well? I didn't get it, do you mean? - f2fs_balance_fs() - f2fs_gc() creates prefree segments but not call checkpoint to reclaim - f2fs_balance_fs() - f2fs_gc() - detect prefree segments created by last f2fs_balance_fs, then call f2fs_write_checkpoint to reclaim Or could you please provide a draft patch? :-P Thanks, > > 1832 > 1833 if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { > 1834 /* > 1835 * For example, if there are many prefree_segments below given > 1836 * threshold, we can make them free by checkpoint. Then, we > 1837 * secure free segments which doesn't need fggc any more. > 1838 */ > 1839 if (prefree_segments(sbi)) { > 1840 ret = f2fs_write_checkpoint(sbi, &cpc); > 1841 if (ret) > 1842 goto stop; > 1843 } > 1844 if (has_not_enough_free_secs(sbi, 0, 0)) > 1845 gc_type = FG_GC; > 1846 } > >> >> One other concern is for those path as below: >> - disable_checkpoint >> - ioc_gc >> - ioc_gc_range >> - ioc_resize >> ... > > I think the upper caller should decide to call checkpoint, if they want to > reclaim the prefree likewise f2fs_disable_checkpoint. > >> >> We've passed gc_type as FG_GC, but the demand here is to migrate block in time, >> rather than dirtying blocks, and callers don't expect checkpoint in f2fs_gc(), >> instead the callers will do the checkpoit as it needs. >> >> That means it's better to decouple FG_GC and write_checkpoint behavior, so I >> added another parameter .reclaim_space to just let f2fs_balance_fs() to trigger >> checkpoit in the end of f2fs_gc(). > >> >> Thanks, >> >>> + } else if (gc_type == FG_GC) { >>> + /* FG_GC stops GC by skip_count */ >>> if (sbi->skipped_gc_rwsem) >>> skipped_round++; >>> round++; >>> if (skipped_round > MAX_SKIP_GC_COUNT && >>> - skipped_round * 2 >= round) { >>> - ret = f2fs_write_checkpoint(sbi, &cpc); >>> - goto stop; >>> - } >>> + skipped_round * 2 >= round) >>> + stop_gc = true; >>> } >>> __get_secs_required(sbi, NULL, &upper_secs, NULL); >>> @@ -1901,12 +1898,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>> prefree_segments(sbi)) { >>> ret = f2fs_write_checkpoint(sbi, &cpc); >>> if (ret) >>> - goto stop; >>> + stop_gc = true; >>> } >>> go_gc_more: >>> - segno = NULL_SEGNO; >>> - goto gc_more; >>> - >>> + if (!stop_gc) { >>> + segno = NULL_SEGNO; >>> + goto gc_more; >>> + } >>> stop: >>> SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0; >>> SIT_I(sbi)->last_victim[FLUSH_DEVICE] = gc_control->victim_segno;
On 04/10, Chao Yu wrote: > On 2023/4/5 23:55, Jaegeuk Kim wrote: > > On 04/05, Chao Yu wrote: > > > On 2023/4/5 5:39, Jaegeuk Kim wrote: > > > > Can we do like this? > > > > > > > > From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 > > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > Date: Tue, 4 Apr 2023 14:36:00 -0700 > > > > Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections > > > > > > > > We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree > > > > sctions in time. > > > > > > > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > --- > > > > fs/f2fs/gc.c | 24 +++++++++++------------- > > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > > > index 56c53dbe05c9..f1d0dd9c5a6c 100644 > > > > --- a/fs/f2fs/gc.c > > > > +++ b/fs/f2fs/gc.c > > > > @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > }; > > > > unsigned int skipped_round = 0, round = 0; > > > > unsigned int upper_secs; > > > > + bool stop_gc = false; > > > > trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, > > > > gc_control->nr_free_secs, > > > > @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > > > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > > > goto go_gc_more; > > > > - goto stop; > > > > - } > > > > - > > > > - /* FG_GC stops GC by skip_count */ > > > > - if (gc_type == FG_GC) { > > > > + stop_gc = true; > > > > > > I guess below condition is for emergency recycle of prefree segments during > > > foreground GC, in order to avoid exhausting free sections due to to many > > > metadata allocation during CP. > > > > > > if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && > > > prefree_segments(sbi)) { > > > > > > But for common case, free_sections() is close to reserved_segments(), and > > > upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), > > > so checkpoint may not be trggered as expected, IIUC. > > > > > > So it's fine to just trigger CP in the end of foreground garbage collection? > > > > My major concern is to avoid unnecessary checkpointing given multiple FG_GC > > requests were pending in parallel. And, I don't want to add so many combination > > which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint > > automatically in the worst case scenario only. > > Alright. > > > > > By the way, do we just need to call checkpoint here including FG_GC as well? > > I didn't get it, do you mean? > > - f2fs_balance_fs() > - f2fs_gc() creates prefree segments but not call checkpoint to reclaim > > - f2fs_balance_fs() > - f2fs_gc() > - detect prefree segments created by last f2fs_balance_fs, then call > f2fs_write_checkpoint to reclaim > > Or could you please provide a draft patch? :-P Testing this. From ec5f37bbe33110257c04e0ec97a80b0111465b52 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim <jaegeuk@kernel.org> Date: Mon, 10 Apr 2023 14:48:50 -0700 Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition The major change is to call checkpoint, if there's not enough space while having some prefree segments in FG_GC case. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/gc.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index c748cdfb0501..0a823d2e8b9d 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -1829,7 +1829,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) goto stop; } - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { + /* Let's run FG_GC, if we don't have enough space. */ + if (has_not_enough_free_secs(sbi, 0, 0)) { + gc_type = FG_GC; + /* * For example, if there are many prefree_segments below given * threshold, we can make them free by checkpoint. Then, we @@ -1840,8 +1843,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) if (ret) goto stop; } - if (has_not_enough_free_secs(sbi, 0, 0)) - gc_type = FG_GC; } /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ @@ -1868,19 +1869,14 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) sec_freed++; - if (gc_type == FG_GC) + if (gc_type == FG_GC) { sbi->cur_victim_sec = NULL_SEGNO; - if (gc_control->init_gc_type == FG_GC || - !has_not_enough_free_secs(sbi, - (gc_type == FG_GC) ? sec_freed : 0, 0)) { - if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) - goto go_gc_more; - goto stop; - } - - /* FG_GC stops GC by skip_count */ - if (gc_type == FG_GC) { + if (!has_not_enough_free_secs(sbi, sec_freed, 0)) { + if (sec_freed < gc_control->nr_free_secs) + goto go_gc_more; + goto stop; + } if (sbi->skipped_gc_rwsem) skipped_round++; round++; @@ -1889,6 +1885,8 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) ret = f2fs_write_checkpoint(sbi, &cpc); goto stop; } + } else if (!has_not_enough_free_secs(sbi, 0, 0)) { + goto stop; } __get_secs_required(sbi, NULL, &upper_secs, NULL);
On 2023/4/11 7:21, Jaegeuk Kim wrote: > On 04/10, Chao Yu wrote: >> On 2023/4/5 23:55, Jaegeuk Kim wrote: >>> On 04/05, Chao Yu wrote: >>>> On 2023/4/5 5:39, Jaegeuk Kim wrote: >>>>> Can we do like this? >>>>> >>>>> From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 >>>>> From: Jaegeuk Kim <jaegeuk@kernel.org> >>>>> Date: Tue, 4 Apr 2023 14:36:00 -0700 >>>>> Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections >>>>> >>>>> We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree >>>>> sctions in time. >>>>> >>>>> Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") >>>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>>> --- >>>>> fs/f2fs/gc.c | 24 +++++++++++------------- >>>>> 1 file changed, 11 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>>> index 56c53dbe05c9..f1d0dd9c5a6c 100644 >>>>> --- a/fs/f2fs/gc.c >>>>> +++ b/fs/f2fs/gc.c >>>>> @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>>>> }; >>>>> unsigned int skipped_round = 0, round = 0; >>>>> unsigned int upper_secs; >>>>> + bool stop_gc = false; >>>>> trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, >>>>> gc_control->nr_free_secs, >>>>> @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) >>>>> (gc_type == FG_GC) ? sec_freed : 0, 0)) { >>>>> if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) >>>>> goto go_gc_more; >>>>> - goto stop; >>>>> - } >>>>> - >>>>> - /* FG_GC stops GC by skip_count */ >>>>> - if (gc_type == FG_GC) { >>>>> + stop_gc = true; >>>> >>>> I guess below condition is for emergency recycle of prefree segments during >>>> foreground GC, in order to avoid exhausting free sections due to to many >>>> metadata allocation during CP. >>>> >>>> if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && >>>> prefree_segments(sbi)) { >>>> >>>> But for common case, free_sections() is close to reserved_segments(), and >>>> upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), >>>> so checkpoint may not be trggered as expected, IIUC. >>>> >>>> So it's fine to just trigger CP in the end of foreground garbage collection? >>> >>> My major concern is to avoid unnecessary checkpointing given multiple FG_GC >>> requests were pending in parallel. And, I don't want to add so many combination >>> which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint >>> automatically in the worst case scenario only. >> >> Alright. >> >>> >>> By the way, do we just need to call checkpoint here including FG_GC as well? >> >> I didn't get it, do you mean? >> >> - f2fs_balance_fs() >> - f2fs_gc() creates prefree segments but not call checkpoint to reclaim >> >> - f2fs_balance_fs() >> - f2fs_gc() >> - detect prefree segments created by last f2fs_balance_fs, then call >> f2fs_write_checkpoint to reclaim >> >> Or could you please provide a draft patch? :-P > > Testing this. > > From ec5f37bbe33110257c04e0ec97a80b0111465b52 Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Mon, 10 Apr 2023 14:48:50 -0700 > Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition > > The major change is to call checkpoint, if there's not enough space while having > some prefree segments in FG_GC case. I found generic/269 will hang w/ this patch. Thanks, > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/gc.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index c748cdfb0501..0a823d2e8b9d 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -1829,7 +1829,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > goto stop; > } > > - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { > + /* Let's run FG_GC, if we don't have enough space. */ > + if (has_not_enough_free_secs(sbi, 0, 0)) { > + gc_type = FG_GC; > + > /* > * For example, if there are many prefree_segments below given > * threshold, we can make them free by checkpoint. Then, we > @@ -1840,8 +1843,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > if (ret) > goto stop; > } > - if (has_not_enough_free_secs(sbi, 0, 0)) > - gc_type = FG_GC; > } > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > @@ -1868,19 +1869,14 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) > sec_freed++; > > - if (gc_type == FG_GC) > + if (gc_type == FG_GC) { > sbi->cur_victim_sec = NULL_SEGNO; > > - if (gc_control->init_gc_type == FG_GC || > - !has_not_enough_free_secs(sbi, > - (gc_type == FG_GC) ? sec_freed : 0, 0)) { > - if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > - goto go_gc_more; > - goto stop; > - } > - > - /* FG_GC stops GC by skip_count */ > - if (gc_type == FG_GC) { > + if (!has_not_enough_free_secs(sbi, sec_freed, 0)) { > + if (sec_freed < gc_control->nr_free_secs) > + goto go_gc_more; > + goto stop; > + } > if (sbi->skipped_gc_rwsem) > skipped_round++; > round++; > @@ -1889,6 +1885,8 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > ret = f2fs_write_checkpoint(sbi, &cpc); > goto stop; > } > + } else if (!has_not_enough_free_secs(sbi, 0, 0)) { > + goto stop; > } > > __get_secs_required(sbi, NULL, &upper_secs, NULL);
On 04/13, Chao Yu wrote: > On 2023/4/11 7:21, Jaegeuk Kim wrote: > > On 04/10, Chao Yu wrote: > > > On 2023/4/5 23:55, Jaegeuk Kim wrote: > > > > On 04/05, Chao Yu wrote: > > > > > On 2023/4/5 5:39, Jaegeuk Kim wrote: > > > > > > Can we do like this? > > > > > > > > > > > > From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 > > > > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > Date: Tue, 4 Apr 2023 14:36:00 -0700 > > > > > > Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections > > > > > > > > > > > > We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree > > > > > > sctions in time. > > > > > > > > > > > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > --- > > > > > > fs/f2fs/gc.c | 24 +++++++++++------------- > > > > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > > > > > index 56c53dbe05c9..f1d0dd9c5a6c 100644 > > > > > > --- a/fs/f2fs/gc.c > > > > > > +++ b/fs/f2fs/gc.c > > > > > > @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > > > }; > > > > > > unsigned int skipped_round = 0, round = 0; > > > > > > unsigned int upper_secs; > > > > > > + bool stop_gc = false; > > > > > > trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, > > > > > > gc_control->nr_free_secs, > > > > > > @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > > > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > > > > > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > > > > > goto go_gc_more; > > > > > > - goto stop; > > > > > > - } > > > > > > - > > > > > > - /* FG_GC stops GC by skip_count */ > > > > > > - if (gc_type == FG_GC) { > > > > > > + stop_gc = true; > > > > > > > > > > I guess below condition is for emergency recycle of prefree segments during > > > > > foreground GC, in order to avoid exhausting free sections due to to many > > > > > metadata allocation during CP. > > > > > > > > > > if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && > > > > > prefree_segments(sbi)) { > > > > > > > > > > But for common case, free_sections() is close to reserved_segments(), and > > > > > upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), > > > > > so checkpoint may not be trggered as expected, IIUC. > > > > > > > > > > So it's fine to just trigger CP in the end of foreground garbage collection? > > > > > > > > My major concern is to avoid unnecessary checkpointing given multiple FG_GC > > > > requests were pending in parallel. And, I don't want to add so many combination > > > > which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint > > > > automatically in the worst case scenario only. > > > > > > Alright. > > > > > > > > > > > By the way, do we just need to call checkpoint here including FG_GC as well? > > > > > > I didn't get it, do you mean? > > > > > > - f2fs_balance_fs() > > > - f2fs_gc() creates prefree segments but not call checkpoint to reclaim > > > > > > - f2fs_balance_fs() > > > - f2fs_gc() > > > - detect prefree segments created by last f2fs_balance_fs, then call > > > f2fs_write_checkpoint to reclaim > > > > > > Or could you please provide a draft patch? :-P > > > > Testing this. > > > > From ec5f37bbe33110257c04e0ec97a80b0111465b52 Mon Sep 17 00:00:00 2001 > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > Date: Mon, 10 Apr 2023 14:48:50 -0700 > > Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition > > > > The major change is to call checkpoint, if there's not enough space while having > > some prefree segments in FG_GC case. > > I found generic/269 will hang w/ this patch. Yeah, I got 270 as well. I removed it in the tree first, and will check it out later. > > Thanks, > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/gc.c | 26 ++++++++++++-------------- > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > index c748cdfb0501..0a823d2e8b9d 100644 > > --- a/fs/f2fs/gc.c > > +++ b/fs/f2fs/gc.c > > @@ -1829,7 +1829,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > goto stop; > > } > > - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { > > + /* Let's run FG_GC, if we don't have enough space. */ > > + if (has_not_enough_free_secs(sbi, 0, 0)) { > > + gc_type = FG_GC; > > + > > /* > > * For example, if there are many prefree_segments below given > > * threshold, we can make them free by checkpoint. Then, we > > @@ -1840,8 +1843,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > if (ret) > > goto stop; > > } > > - if (has_not_enough_free_secs(sbi, 0, 0)) > > - gc_type = FG_GC; > > } > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > > @@ -1868,19 +1869,14 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) > > sec_freed++; > > - if (gc_type == FG_GC) > > + if (gc_type == FG_GC) { > > sbi->cur_victim_sec = NULL_SEGNO; > > - if (gc_control->init_gc_type == FG_GC || > > - !has_not_enough_free_secs(sbi, > > - (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > - if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > - goto go_gc_more; > > - goto stop; > > - } > > - > > - /* FG_GC stops GC by skip_count */ > > - if (gc_type == FG_GC) { > > + if (!has_not_enough_free_secs(sbi, sec_freed, 0)) { > > + if (sec_freed < gc_control->nr_free_secs) > > + goto go_gc_more; > > + goto stop; > > + } > > if (sbi->skipped_gc_rwsem) > > skipped_round++; > > round++; > > @@ -1889,6 +1885,8 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > ret = f2fs_write_checkpoint(sbi, &cpc); > > goto stop; > > } > > + } else if (!has_not_enough_free_secs(sbi, 0, 0)) { > > + goto stop; > > } > > __get_secs_required(sbi, NULL, &upper_secs, NULL);
On 04/13, Jaegeuk Kim wrote: > On 04/13, Chao Yu wrote: > > On 2023/4/11 7:21, Jaegeuk Kim wrote: > > > On 04/10, Chao Yu wrote: > > > > On 2023/4/5 23:55, Jaegeuk Kim wrote: > > > > > On 04/05, Chao Yu wrote: > > > > > > On 2023/4/5 5:39, Jaegeuk Kim wrote: > > > > > > > Can we do like this? > > > > > > > > > > > > > > From 9a58f0e59364241aa31b555cfe793d278e39b0dc Mon Sep 17 00:00:00 2001 > > > > > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > > Date: Tue, 4 Apr 2023 14:36:00 -0700 > > > > > > > Subject: [PATCH] f2fs: do checkpoint when there's not enough free sections > > > > > > > > > > > > > > We didn't do checkpoint in FG_GC case, which may cause losing to reclaim prefree > > > > > > > sctions in time. > > > > > > > > > > > > > > Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") > > > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > > --- > > > > > > > fs/f2fs/gc.c | 24 +++++++++++------------- > > > > > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > > > > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > > > > > > index 56c53dbe05c9..f1d0dd9c5a6c 100644 > > > > > > > --- a/fs/f2fs/gc.c > > > > > > > +++ b/fs/f2fs/gc.c > > > > > > > @@ -1806,6 +1806,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > > > > }; > > > > > > > unsigned int skipped_round = 0, round = 0; > > > > > > > unsigned int upper_secs; > > > > > > > + bool stop_gc = false; > > > > > > > trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc, > > > > > > > gc_control->nr_free_secs, > > > > > > > @@ -1876,19 +1877,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > > > > > (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > > > > > > if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > > > > > > goto go_gc_more; > > > > > > > - goto stop; > > > > > > > - } > > > > > > > - > > > > > > > - /* FG_GC stops GC by skip_count */ > > > > > > > - if (gc_type == FG_GC) { > > > > > > > + stop_gc = true; > > > > > > > > > > > > I guess below condition is for emergency recycle of prefree segments during > > > > > > foreground GC, in order to avoid exhausting free sections due to to many > > > > > > metadata allocation during CP. > > > > > > > > > > > > if (free_sections(sbi) <= upper_secs + NR_GC_CHECKPOINT_SECS && > > > > > > prefree_segments(sbi)) { > > > > > > > > > > > > But for common case, free_sections() is close to reserved_segments(), and > > > > > > upper_secs + NR_GC_CHECKPOINT_SECS value may be far smaller than free_sections(), > > > > > > so checkpoint may not be trggered as expected, IIUC. > > > > > > > > > > > > So it's fine to just trigger CP in the end of foreground garbage collection? > > > > > > > > > > My major concern is to avoid unnecessary checkpointing given multiple FG_GC > > > > > requests were pending in parallel. And, I don't want to add so many combination > > > > > which gives so many corner cases, and feel f2fs_gc() needs to call checkpoint > > > > > automatically in the worst case scenario only. > > > > > > > > Alright. > > > > > > > > > > > > > > By the way, do we just need to call checkpoint here including FG_GC as well? > > > > > > > > I didn't get it, do you mean? > > > > > > > > - f2fs_balance_fs() > > > > - f2fs_gc() creates prefree segments but not call checkpoint to reclaim > > > > > > > > - f2fs_balance_fs() > > > > - f2fs_gc() > > > > - detect prefree segments created by last f2fs_balance_fs, then call > > > > f2fs_write_checkpoint to reclaim > > > > > > > > Or could you please provide a draft patch? :-P > > > > > > Testing this. > > > > > > From ec5f37bbe33110257c04e0ec97a80b0111465b52 Mon Sep 17 00:00:00 2001 > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > Date: Mon, 10 Apr 2023 14:48:50 -0700 > > > Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition > > > > > > The major change is to call checkpoint, if there's not enough space while having > > > some prefree segments in FG_GC case. > > > > I found generic/269 will hang w/ this patch. > > Yeah, I got 270 as well. I removed it in the tree first, and will check it out > later. And, I figured out somehow this patch falls in a loop: - get_victim(FG_GC) returns a same segno - do nothing when migrating blocks due to i_gc_rwsem[WRITE] - !has_not_enough_free_secs() is true, so goto gc_more loop > > > > > Thanks, > > > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > fs/f2fs/gc.c | 26 ++++++++++++-------------- > > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > > index c748cdfb0501..0a823d2e8b9d 100644 > > > --- a/fs/f2fs/gc.c > > > +++ b/fs/f2fs/gc.c > > > @@ -1829,7 +1829,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > goto stop; > > > } > > > - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { > > > + /* Let's run FG_GC, if we don't have enough space. */ > > > + if (has_not_enough_free_secs(sbi, 0, 0)) { > > > + gc_type = FG_GC; > > > + > > > /* > > > * For example, if there are many prefree_segments below given > > > * threshold, we can make them free by checkpoint. Then, we > > > @@ -1840,8 +1843,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > if (ret) > > > goto stop; > > > } > > > - if (has_not_enough_free_secs(sbi, 0, 0)) > > > - gc_type = FG_GC; > > > } > > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > > > @@ -1868,19 +1869,14 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) > > > sec_freed++; > > > - if (gc_type == FG_GC) > > > + if (gc_type == FG_GC) { > > > sbi->cur_victim_sec = NULL_SEGNO; > > > - if (gc_control->init_gc_type == FG_GC || > > > - !has_not_enough_free_secs(sbi, > > > - (gc_type == FG_GC) ? sec_freed : 0, 0)) { > > > - if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) > > > - goto go_gc_more; > > > - goto stop; > > > - } > > > - > > > - /* FG_GC stops GC by skip_count */ > > > - if (gc_type == FG_GC) { > > > + if (!has_not_enough_free_secs(sbi, sec_freed, 0)) { > > > + if (sec_freed < gc_control->nr_free_secs) > > > + goto go_gc_more; > > > + goto stop; > > > + } > > > if (sbi->skipped_gc_rwsem) > > > skipped_round++; > > > round++; > > > @@ -1889,6 +1885,8 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) > > > ret = f2fs_write_checkpoint(sbi, &cpc); > > > goto stop; > > > } > > > + } else if (!has_not_enough_free_secs(sbi, 0, 0)) { > > > + goto stop; > > > } > > > __get_secs_required(sbi, NULL, &upper_secs, NULL); > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Fixed a xfstests failure. From 400c722c2117660b83190c88e5442d63fbbffe6e Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim <jaegeuk@kernel.org> Date: Mon, 10 Apr 2023 14:48:50 -0700 Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition The major change is to call checkpoint, if there's not enough space while having some prefree segments in FG_GC case. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/gc.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index c748cdfb0501..ba5775dcade6 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -1829,7 +1829,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) goto stop; } - if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0, 0)) { + /* Let's run FG_GC, if we don't have enough space. */ + if (has_not_enough_free_secs(sbi, 0, 0)) { + gc_type = FG_GC; + /* * For example, if there are many prefree_segments below given * threshold, we can make them free by checkpoint. Then, we @@ -1840,8 +1843,6 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) if (ret) goto stop; } - if (has_not_enough_free_secs(sbi, 0, 0)) - gc_type = FG_GC; } /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ @@ -1868,19 +1869,15 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) if (seg_freed == f2fs_usable_segs_in_sec(sbi, segno)) sec_freed++; - if (gc_type == FG_GC) + if (gc_type == FG_GC) { sbi->cur_victim_sec = NULL_SEGNO; - if (gc_control->init_gc_type == FG_GC || - !has_not_enough_free_secs(sbi, - (gc_type == FG_GC) ? sec_freed : 0, 0)) { - if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) - goto go_gc_more; - goto stop; - } - - /* FG_GC stops GC by skip_count */ - if (gc_type == FG_GC) { + if (!has_not_enough_free_secs(sbi, sec_freed, 0)) { + if (!gc_control->no_bg_gc && + sec_freed < gc_control->nr_free_secs) + goto go_gc_more; + goto stop; + } if (sbi->skipped_gc_rwsem) skipped_round++; round++; @@ -1889,6 +1886,8 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) ret = f2fs_write_checkpoint(sbi, &cpc); goto stop; } + } else if (!has_not_enough_free_secs(sbi, 0, 0)) { + goto stop; } __get_secs_required(sbi, NULL, &upper_secs, NULL);
On 2023/4/14 3:25, Jaegeuk Kim wrote: > Fixed a xfstests failure. > > From 400c722c2117660b83190c88e5442d63fbbffe6e Mon Sep 17 00:00:00 2001 > From: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Mon, 10 Apr 2023 14:48:50 -0700 > Subject: [PATCH] f2fs: refactor f2fs_gc to call checkpoint in urgent condition > > The major change is to call checkpoint, if there's not enough space while having > some prefree segments in FG_GC case. > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> Reviewed-by: Chao Yu <chao@kernel.org> Thanks,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 53a005b420cf..b1515375cb4c 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -1269,6 +1269,7 @@ struct f2fs_gc_control { unsigned int victim_segno; /* target victim segment number */ int init_gc_type; /* FG_GC or BG_GC */ bool no_bg_gc; /* check the space and stop bg_gc */ + bool reclaim_space; /* trigger checkpoint to reclaim space */ bool should_migrate_blocks; /* should migrate blocks */ bool err_gc_skipped; /* return EAGAIN if GC skipped */ unsigned int nr_free_secs; /* # of free sections to do GC */ diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 2996d38aa89c..5a451d3d512d 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -132,6 +132,7 @@ static int gc_thread_func(void *data) gc_control.init_gc_type = sync_mode ? FG_GC : BG_GC; gc_control.no_bg_gc = foreground; + gc_control.reclaim_space = foreground; gc_control.nr_free_secs = foreground ? 1 : 0; /* if return value is not zero, no victim was selected */ @@ -1880,6 +1881,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control) (gc_type == FG_GC) ? sec_freed : 0, 0)) { if (gc_type == FG_GC && sec_freed < gc_control->nr_free_secs) goto go_gc_more; + + /* + * trigger a checkpoint in the end of foreground garbage + * collection. + */ + if (gc_control->reclaim_space) + ret = f2fs_write_checkpoint(sbi, &cpc); goto stop; } diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 6c11789da884..b62af2ae1685 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -421,6 +421,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need) .victim_segno = NULL_SEGNO, .init_gc_type = BG_GC, .no_bg_gc = true, + .reclaim_space = true, .should_migrate_blocks = false, .err_gc_skipped = false, .nr_free_secs = 1 };
In order to reclaim free blocks in prefree sections before latter use. Fixes: 6f8d4455060d ("f2fs: avoid fi->i_gc_rwsem[WRITE] lock in f2fs_gc") Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/f2fs.h | 1 + fs/f2fs/gc.c | 8 ++++++++ fs/f2fs/segment.c | 1 + 3 files changed, 10 insertions(+)