2022-05-12 13:25:44

by Chao Yu

Subject: Re: [f2fs-dev] [PATCH 4/5] f2fs: do not stop GC when requiring a free section

On 2022/5/12 0:47, Jaegeuk Kim wrote:
>>>>> @@ -147,6 +147,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.nr_free_secs = foreground ? 1 : 0;

[snip]

>
> I mean gc_control->nr_free_secs should be 0.

[snip]

>>>>> @@ -528,7 +528,8 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
>>>>> .init_gc_type = BG_GC,
>>>>> .no_bg_gc = true,
>>>>> .should_migrate_blocks = false,
>>>>> - .err_gc_skipped = false };
>>>>> + .err_gc_skipped = false,
>>>>> + .nr_free_secs = 1 };

Oh, so, in above two paths, when .nr_free_secs is 1, no_bg_gc should be true
to keep skipping BG_GC flow.

How about adding a check condition in f2fs_gc() to avoid invalid argument
usage in future?

From: Chao Yu <[email protected]>

---
fs/f2fs/gc.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 385282017317..a98276fd3cc1 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1799,10 +1799,19 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
gc_type = FG_GC;
}

- /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */
- if (gc_type == BG_GC && gc_control->no_bg_gc) {
- ret = -EINVAL;
- goto stop;
+ if (gc_type == BG_GC) {
+ /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */
+ if (gc_control->no_bg_gc) {
+ ret = -EINVAL;
+ goto stop;
+ }
+ /*
+ * BG_GC never guarantee that blocks are migrated synchronously.
+ */
+ if (gc_control->nr_free_secs) {
+ ret = -EINVAL;
+ goto stop;
+ }
}
retry:
ret = __get_victim(sbi, &segno, gc_type);
--
2.25.1



Thanks,

2022-05-14 02:36:25

by Jaegeuk Kim

Subject: Re: [f2fs-dev] [PATCH 4/5] f2fs: do not stop GC when requiring a free section

On 05/12, Chao Yu wrote:
> On 2022/5/12 0:47, Jaegeuk Kim wrote:
> > > > > > @@ -147,6 +147,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.nr_free_secs = foreground ? 1 : 0;
>
> [snip]
>
> >
> > I mean gc_control->nr_free_secs should be 0.
>
> [snip]
>
> > > > > > @@ -528,7 +528,8 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
> > > > > > .init_gc_type = BG_GC,
> > > > > > .no_bg_gc = true,
> > > > > > .should_migrate_blocks = false,
> > > > > > - .err_gc_skipped = false };
> > > > > > + .err_gc_skipped = false,
> > > > > > + .nr_free_secs = 1 };
>
> Oh, so, in above two paths, when .nr_free_secs is 1, no_bg_gc should be true
> to keep skipping BG_GC flow.
>
> How about adding a check condition in f2fs_gc() to avoid invalid argument
> usage in future?

Sent out v2. Could you please check?

>
> From: Chao Yu <[email protected]>
>
> ---
> fs/f2fs/gc.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 385282017317..a98276fd3cc1 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1799,10 +1799,19 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
> gc_type = FG_GC;
> }
>
> - /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */
> - if (gc_type == BG_GC && gc_control->no_bg_gc) {
> - ret = -EINVAL;
> - goto stop;
> + if (gc_type == BG_GC) {
> + /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */
> + if (gc_control->no_bg_gc) {
> + ret = -EINVAL;
> + goto stop;
> + }
> + /*
> + * BG_GC never guarantee that blocks are migrated synchronously.
> + */
> + if (gc_control->nr_free_secs) {
> + ret = -EINVAL;
> + goto stop;
> + }
> }
> retry:
> ret = __get_victim(sbi, &segno, gc_type);
> --
> 2.25.1
>
>
>
> Thanks,