Message ID | 1507729864-118702-1-git-send-email-yunlong.song@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Yunlong, On 10/11, Yunlong Song wrote: > This can help us to debug on some corner case. > > Signed-off-by: Yunlong Song <yunlong.song@huawei.com> > --- > fs/f2fs/gc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 197ebf4..960503e 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > .ilist = LIST_HEAD_INIT(gc_list.ilist), > .iroot = RADIX_TREE_INIT(GFP_NOFS), > }; > + bool need_gc = false; > > trace_f2fs_gc_begin(sbi->sb, sync, background, > get_pages(sbi, F2FS_DIRTY_NODES), > @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > if (ret) > goto stop; > } > - if (has_not_enough_free_secs(sbi, 0, 0)) > + if (has_not_enough_free_secs(sbi, 0, 0)) { > gc_type = FG_GC; > + need_gc = true; > + } > } > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > goto stop; > } > if (!__get_victim(sbi, &segno, gc_type)) { > + f2fs_bug_on(sbi, !total_freed && need_gc); How about this, if this is for debugging purpose? if (!total_freed && need_gc) f2fs_msg(...); It's not a good way to define a value for f2fs_bug_on(), since it is only activated by CHECK_FS config. Thanks, > ret = -ENODATA; > goto stop; > } > -- > 1.8.5.2
Hi, Jay, I think it should not happen when need_gc == true but total_freed ==0, so I add it as bug_on to let it panic at once. And even CHECK_FS is closed, this can also printk WARNING message for notice. On 2017/10/13 7:23, Jaegeuk Kim wrote: > Hi Yunlong, > > On 10/11, Yunlong Song wrote: >> This can help us to debug on some corner case. >> >> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >> --- >> fs/f2fs/gc.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >> index 197ebf4..960503e 100644 >> --- a/fs/f2fs/gc.c >> +++ b/fs/f2fs/gc.c >> @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> .ilist = LIST_HEAD_INIT(gc_list.ilist), >> .iroot = RADIX_TREE_INIT(GFP_NOFS), >> }; >> + bool need_gc = false; >> >> trace_f2fs_gc_begin(sbi->sb, sync, background, >> get_pages(sbi, F2FS_DIRTY_NODES), >> @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> if (ret) >> goto stop; >> } >> - if (has_not_enough_free_secs(sbi, 0, 0)) >> + if (has_not_enough_free_secs(sbi, 0, 0)) { >> gc_type = FG_GC; >> + need_gc = true; >> + } >> } >> >> /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ >> @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> goto stop; >> } >> if (!__get_victim(sbi, &segno, gc_type)) { >> + f2fs_bug_on(sbi, !total_freed && need_gc); > How about this, if this is for debugging purpose? > > if (!total_freed && need_gc) > f2fs_msg(...); > > It's not a good way to define a value for f2fs_bug_on(), since it is only > activated by CHECK_FS config. > > Thanks, > >> ret = -ENODATA; >> goto stop; >> } >> -- >> 1.8.5.2 > . >
On 10/13, Yunlong Song wrote: > Hi, Jay, > I think it should not happen when need_gc == true but total_freed ==0, > so I add it as bug_on to let it panic at once. And even CHECK_FS is closed, > this can also printk WARNING message for notice. Ah, got it. Merged. :) Thanks, > > On 2017/10/13 7:23, Jaegeuk Kim wrote: > > Hi Yunlong, > > > > On 10/11, Yunlong Song wrote: > > > This can help us to debug on some corner case. > > > > > > Signed-off-by: Yunlong Song <yunlong.song@huawei.com> > > > --- > > > fs/f2fs/gc.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > > > index 197ebf4..960503e 100644 > > > --- a/fs/f2fs/gc.c > > > +++ b/fs/f2fs/gc.c > > > @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > > > .ilist = LIST_HEAD_INIT(gc_list.ilist), > > > .iroot = RADIX_TREE_INIT(GFP_NOFS), > > > }; > > > + bool need_gc = false; > > > trace_f2fs_gc_begin(sbi->sb, sync, background, > > > get_pages(sbi, F2FS_DIRTY_NODES), > > > @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > > > if (ret) > > > goto stop; > > > } > > > - if (has_not_enough_free_secs(sbi, 0, 0)) > > > + if (has_not_enough_free_secs(sbi, 0, 0)) { > > > gc_type = FG_GC; > > > + need_gc = true; > > > + } > > > } > > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > > > @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > > > goto stop; > > > } > > > if (!__get_victim(sbi, &segno, gc_type)) { > > > + f2fs_bug_on(sbi, !total_freed && need_gc); > > How about this, if this is for debugging purpose? > > > > if (!total_freed && need_gc) > > f2fs_msg(...); > > > > It's not a good way to define a value for f2fs_bug_on(), since it is only > > activated by CHECK_FS config. > > > > Thanks, > > > > > ret = -ENODATA; > > > goto stop; > > > } > > > -- > > > 1.8.5.2 > > . > > > > -- > Thanks, > Yunlong Song >
On 2017/10/11 21:51, Yunlong Song wrote: > This can help us to debug on some corner case. > > Signed-off-by: Yunlong Song <yunlong.song@huawei.com> > --- > fs/f2fs/gc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c > index 197ebf4..960503e 100644 > --- a/fs/f2fs/gc.c > +++ b/fs/f2fs/gc.c > @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > .ilist = LIST_HEAD_INIT(gc_list.ilist), > .iroot = RADIX_TREE_INIT(GFP_NOFS), > }; > + bool need_gc = false; How about changing variable name to need_fggc for better readability? Thanks, > > trace_f2fs_gc_begin(sbi->sb, sync, background, > get_pages(sbi, F2FS_DIRTY_NODES), > @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > if (ret) > goto stop; > } > - if (has_not_enough_free_secs(sbi, 0, 0)) > + if (has_not_enough_free_secs(sbi, 0, 0)) { > gc_type = FG_GC; > + need_gc = true; > + } > } > > /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ > @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, > goto stop; > } > if (!__get_victim(sbi, &segno, gc_type)) { > + f2fs_bug_on(sbi, !total_freed && need_gc); > ret = -ENODATA; > goto stop; > } >
Yep, both are OK, since I do not care it is foreground or background. Here it just needs to do gc. On 2017/10/13 19:09, Chao Yu wrote: > On 2017/10/11 21:51, Yunlong Song wrote: >> This can help us to debug on some corner case. >> >> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >> --- >> fs/f2fs/gc.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >> index 197ebf4..960503e 100644 >> --- a/fs/f2fs/gc.c >> +++ b/fs/f2fs/gc.c >> @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> .ilist = LIST_HEAD_INIT(gc_list.ilist), >> .iroot = RADIX_TREE_INIT(GFP_NOFS), >> }; >> + bool need_gc = false; > How about changing variable name to need_fggc for better readability? > > Thanks, > >> >> trace_f2fs_gc_begin(sbi->sb, sync, background, >> get_pages(sbi, F2FS_DIRTY_NODES), >> @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> if (ret) >> goto stop; >> } >> - if (has_not_enough_free_secs(sbi, 0, 0)) >> + if (has_not_enough_free_secs(sbi, 0, 0)) { >> gc_type = FG_GC; >> + need_gc = true; >> + } >> } >> >> /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ >> @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >> goto stop; >> } >> if (!__get_victim(sbi, &segno, gc_type)) { >> + f2fs_bug_on(sbi, !total_freed && need_gc); >> ret = -ENODATA; >> goto stop; >> } >> > . >
On 2017/10/13 19:20, Yunlong Song wrote: > Yep, both are OK, since I do not care it is foreground or background. Here it just needs to do gc. But you only set need_gc as true for foreground GC case. Thanks, > > On 2017/10/13 19:09, Chao Yu wrote: >> On 2017/10/11 21:51, Yunlong Song wrote: >>> This can help us to debug on some corner case. >>> >>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >>> --- >>> fs/f2fs/gc.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>> index 197ebf4..960503e 100644 >>> --- a/fs/f2fs/gc.c >>> +++ b/fs/f2fs/gc.c >>> @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >>> .ilist = LIST_HEAD_INIT(gc_list.ilist), >>> .iroot = RADIX_TREE_INIT(GFP_NOFS), >>> }; >>> + bool need_gc = false; >> How about changing variable name to need_fggc for better readability? >> >> Thanks, >> >>> trace_f2fs_gc_begin(sbi->sb, sync, background, >>> get_pages(sbi, F2FS_DIRTY_NODES), >>> @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >>> if (ret) >>> goto stop; >>> } >>> - if (has_not_enough_free_secs(sbi, 0, 0)) >>> + if (has_not_enough_free_secs(sbi, 0, 0)) { >>> gc_type = FG_GC; >>> + need_gc = true; >>> + } >>> } >>> /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ >>> @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, >>> goto stop; >>> } >>> if (!__get_victim(sbi, &segno, gc_type)) { >>> + f2fs_bug_on(sbi, !total_freed && need_gc); >>> ret = -ENODATA; >>> goto stop; >>> } >>> >> . >> >
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c index 197ebf4..960503e 100644 --- a/fs/f2fs/gc.c +++ b/fs/f2fs/gc.c @@ -986,6 +986,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, .ilist = LIST_HEAD_INIT(gc_list.ilist), .iroot = RADIX_TREE_INIT(GFP_NOFS), }; + bool need_gc = false; trace_f2fs_gc_begin(sbi->sb, sync, background, get_pages(sbi, F2FS_DIRTY_NODES), @@ -1018,8 +1019,10 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, if (ret) goto stop; } - if (has_not_enough_free_secs(sbi, 0, 0)) + if (has_not_enough_free_secs(sbi, 0, 0)) { gc_type = FG_GC; + need_gc = true; + } } /* f2fs_balance_fs doesn't need to do BG_GC in critical path. */ @@ -1028,6 +1031,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, goto stop; } if (!__get_victim(sbi, &segno, gc_type)) { + f2fs_bug_on(sbi, !total_freed && need_gc); ret = -ENODATA; goto stop; }
This can help us to debug on some corner case. Signed-off-by: Yunlong Song <yunlong.song@huawei.com> --- fs/f2fs/gc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)