Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-09 Thread Yunlong Song 

Agree.

On 2017/11/10 1:59, Jaegeuk Kim wrote:

On 11/08, Yunlong Song wrote:

So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type ==
FG_GC);

f2fs_bug_on(sbi, !total_freed && has_not_enough_free_secs(sbi, 0, 0)); ?


On 2017/11/7 14:56, Chao Yu wrote:

On 2017/11/7 12:01, Yunlong Song wrote:

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc
at all.
For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.


.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-09 Thread Yunlong Song 

Agree.

On 2017/11/10 1:59, Jaegeuk Kim wrote:

On 11/08, Yunlong Song wrote:

So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type ==
FG_GC);

f2fs_bug_on(sbi, !total_freed && has_not_enough_free_secs(sbi, 0, 0)); ?


On 2017/11/7 14:56, Chao Yu wrote:

On 2017/11/7 12:01, Yunlong Song wrote:

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc
at all.
For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.


.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-09 Thread Jaegeuk Kim 
On 11/08, Yunlong Song wrote:
> So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type ==
> FG_GC);

f2fs_bug_on(sbi, !total_freed && has_not_enough_free_secs(sbi, 0, 0)); ?

> 
> On 2017/11/7 14:56, Chao Yu wrote:
> > On 2017/11/7 12:01, Yunlong Song wrote:
> > > Sorry, misunderstanding, because I think when sync == true, FG_GC does not
> > > check has_not_enough_free_secs, so maybe it does not have to do any gc
> > > at all.
> > > For example, if there are 100 segments for f2fs, and 20 segments are full 
> > > or
> > > valid blocks over fggc_threshold, then it is correct to fail in get 
> > > victim.
> > > 
> > > 
> > > On 2017/11/7 11:26, Jaegeuk Kim wrote:
> > > > On 11/07, Yunlong Song wrote:
> > > > > Because I find that some out-of-free problem is caused by the failure
> > > > > of get victim target. For example, chao has pointed out that he has
> > > > > found out a bug when adding this bug_on last week.
> > > > That's NOT what I asked. Why not checking FG_GC all the time like this?
> > > > 
> > > > f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);
> > ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
> > have to check the conditon only when we run out-of-free-space?
> > 
> > Thanks,
> > 
> > > > > On 2017/11/7 10:40, Jaegeuk Kim wrote:
> > > > > > On 11/06, Jaegeuk Kim wrote:
> > > > > > > On 11/06, Yunlong Song wrote:
> > > > > > > > Agree.
> > > > > > > > 
> > > > > > > > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > > > > > > > On 10/13, Yunlong Song wrote:
> > > > > > > > > > This can help us to debug on some corner case.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Yunlong Song 
> > > > > > > > > > Signed-off-by: Chao Yu 
> > > > > > > > > > ---
> > > > > > > > > >  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > > > > > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > > > > > > > Just like this?
> > > > > > > > That's OK.
> > > > > > > I'm not quite sure whether this is really a bug_on case.
> > > > > > > Let me make it WARN_ON() for debugging purpose first.
> > > > > > BTW, why is this the special case where BG_GC detects FG_GC?
> > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > > >   f2fs_bug_on(sbi, !total_freed && !sync && 
> > > > > > > > > gc_type == FG_GC);
> > > > > > > > > 
> > > > > > > > > > ret = -ENODATA;
> > > > > > > > > > goto stop;
> > > > > > > > > > }
> > > > > > > > > > -- 
> > > > > > > > > > 1.8.5.2
> > > > > > > > > .
> > > > > > > > > 
> > > > > > > > -- 
> > > > > > > > Thanks,
> > > > > > > > Yunlong Song
> > > > > > > > 
> > > > > > .
> > > > > > 
> > > > > -- 
> > > > > Thanks,
> > > > > Yunlong Song
> > > > > 
> > > > .
> > > > 
> > 
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-09 Thread Jaegeuk Kim 
On 11/08, Yunlong Song wrote:
> So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type ==
> FG_GC);

f2fs_bug_on(sbi, !total_freed && has_not_enough_free_secs(sbi, 0, 0)); ?

> 
> On 2017/11/7 14:56, Chao Yu wrote:
> > On 2017/11/7 12:01, Yunlong Song wrote:
> > > Sorry, misunderstanding, because I think when sync == true, FG_GC does not
> > > check has_not_enough_free_secs, so maybe it does not have to do any gc
> > > at all.
> > > For example, if there are 100 segments for f2fs, and 20 segments are full 
> > > or
> > > valid blocks over fggc_threshold, then it is correct to fail in get 
> > > victim.
> > > 
> > > 
> > > On 2017/11/7 11:26, Jaegeuk Kim wrote:
> > > > On 11/07, Yunlong Song wrote:
> > > > > Because I find that some out-of-free problem is caused by the failure
> > > > > of get victim target. For example, chao has pointed out that he has
> > > > > found out a bug when adding this bug_on last week.
> > > > That's NOT what I asked. Why not checking FG_GC all the time like this?
> > > > 
> > > > f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);
> > ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
> > have to check the conditon only when we run out-of-free-space?
> > 
> > Thanks,
> > 
> > > > > On 2017/11/7 10:40, Jaegeuk Kim wrote:
> > > > > > On 11/06, Jaegeuk Kim wrote:
> > > > > > > On 11/06, Yunlong Song wrote:
> > > > > > > > Agree.
> > > > > > > > 
> > > > > > > > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > > > > > > > On 10/13, Yunlong Song wrote:
> > > > > > > > > > This can help us to debug on some corner case.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Yunlong Song 
> > > > > > > > > > Signed-off-by: Chao Yu 
> > > > > > > > > > ---
> > > > > > > > > >  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > > > > > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > > > > > > > Just like this?
> > > > > > > > That's OK.
> > > > > > > I'm not quite sure whether this is really a bug_on case.
> > > > > > > Let me make it WARN_ON() for debugging purpose first.
> > > > > > BTW, why is this the special case where BG_GC detects FG_GC?
> > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > > >   f2fs_bug_on(sbi, !total_freed && !sync && 
> > > > > > > > > gc_type == FG_GC);
> > > > > > > > > 
> > > > > > > > > > ret = -ENODATA;
> > > > > > > > > > goto stop;
> > > > > > > > > > }
> > > > > > > > > > -- 
> > > > > > > > > > 1.8.5.2
> > > > > > > > > .
> > > > > > > > > 
> > > > > > > > -- 
> > > > > > > > Thanks,
> > > > > > > > Yunlong Song
> > > > > > > > 
> > > > > > .
> > > > > > 
> > > > > -- 
> > > > > Thanks,
> > > > > Yunlong Song
> > > > > 
> > > > .
> > > > 
> > 
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-07 Thread Yunlong Song 
So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
FG_GC);


On 2017/11/7 14:56, Chao Yu wrote:

On 2017/11/7 12:01, Yunlong Song wrote:

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc
at all.
For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.



.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-07 Thread Yunlong Song 
So we should use f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
FG_GC);


On 2017/11/7 14:56, Chao Yu wrote:

On 2017/11/7 12:01, Yunlong Song wrote:

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc
at all.
For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.



.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Chao Yu 
On 2017/11/7 12:01, Yunlong Song wrote:
> Sorry, misunderstanding, because I think when sync == true, FG_GC does not
> check has_not_enough_free_secs, so maybe it does not have to do any gc 
> at all.
> For example, if there are 100 segments for f2fs, and 20 segments are full or
> valid blocks over fggc_threshold, then it is correct to fail in get victim.
> 
> 
> On 2017/11/7 11:26, Jaegeuk Kim wrote:
>> On 11/07, Yunlong Song wrote:
>>> Because I find that some out-of-free problem is caused by the failure
>>> of get victim target. For example, chao has pointed out that he has
>>> found out a bug when adding this bug_on last week.
>> That's NOT what I asked. Why not checking FG_GC all the time like this?
>>
>> f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,

>>
>>> On 2017/11/7 10:40, Jaegeuk Kim wrote:
 On 11/06, Jaegeuk Kim wrote:
> On 11/06, Yunlong Song wrote:
>> Agree.
>>
>> On 2017/11/3 11:44, Jaegeuk Kim wrote:
>>> On 10/13, Yunlong Song wrote:
 This can help us to debug on some corner case.

 Signed-off-by: Yunlong Song 
 Signed-off-by: Chao Yu 
 ---
 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
 +  f2fs_bug_on(sbi, !total_freed && need_fggc);
>>> Just like this?
>> That's OK.
> I'm not quite sure whether this is really a bug_on case.
> Let me make it WARN_ON() for debugging purpose first.
 BTW, why is this the special case where BG_GC detects FG_GC?

> Thanks,
>
>>> f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
>>> FG_GC);
>>>
ret = -ENODATA;
goto stop;
}
 -- 
 1.8.5.2
>>> .
>>>
>> -- 
>> Thanks,
>> Yunlong Song
>>
 .

>>> -- 
>>> Thanks,
>>> Yunlong Song
>>>
>> .
>>
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Chao Yu 
On 2017/11/7 12:01, Yunlong Song wrote:
> Sorry, misunderstanding, because I think when sync == true, FG_GC does not
> check has_not_enough_free_secs, so maybe it does not have to do any gc 
> at all.
> For example, if there are 100 segments for f2fs, and 20 segments are full or
> valid blocks over fggc_threshold, then it is correct to fail in get victim.
> 
> 
> On 2017/11/7 11:26, Jaegeuk Kim wrote:
>> On 11/07, Yunlong Song wrote:
>>> Because I find that some out-of-free problem is caused by the failure
>>> of get victim target. For example, chao has pointed out that he has
>>> found out a bug when adding this bug_on last week.
>> That's NOT what I asked. Why not checking FG_GC all the time like this?
>>
>> f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

ioctl(F2FS_IOC_GARBAGE_COLLECT, &1) will simply trigger this bug_on, so we
have to check the conditon only when we run out-of-free-space?

Thanks,

>>
>>> On 2017/11/7 10:40, Jaegeuk Kim wrote:
 On 11/06, Jaegeuk Kim wrote:
> On 11/06, Yunlong Song wrote:
>> Agree.
>>
>> On 2017/11/3 11:44, Jaegeuk Kim wrote:
>>> On 10/13, Yunlong Song wrote:
 This can help us to debug on some corner case.

 Signed-off-by: Yunlong Song 
 Signed-off-by: Chao Yu 
 ---
 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
 +  f2fs_bug_on(sbi, !total_freed && need_fggc);
>>> Just like this?
>> That's OK.
> I'm not quite sure whether this is really a bug_on case.
> Let me make it WARN_ON() for debugging purpose first.
 BTW, why is this the special case where BG_GC detects FG_GC?

> Thanks,
>
>>> f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
>>> FG_GC);
>>>
ret = -ENODATA;
goto stop;
}
 -- 
 1.8.5.2
>>> .
>>>
>> -- 
>> Thanks,
>> Yunlong Song
>>
 .

>>> -- 
>>> Thanks,
>>> Yunlong Song
>>>
>> .
>>
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Yunlong Song 

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc 
at all.

For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Yunlong Song 

Sorry, misunderstanding, because I think when sync == true, FG_GC does not
check has_not_enough_free_secs, so maybe it does not have to do any gc 
at all.

For example, if there are 100 segments for f2fs, and 20 segments are full or
valid blocks over fggc_threshold, then it is correct to fail in get victim.


On 2017/11/7 11:26, Jaegeuk Kim wrote:

On 11/07, Yunlong Song wrote:

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);


On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/07, Yunlong Song wrote:
> Because I find that some out-of-free problem is caused by the failure
> of get victim target. For example, chao has pointed out that he has
> found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

> 
> On 2017/11/7 10:40, Jaegeuk Kim wrote:
> > On 11/06, Jaegeuk Kim wrote:
> > > On 11/06, Yunlong Song wrote:
> > > > Agree.
> > > > 
> > > > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > > > On 10/13, Yunlong Song wrote:
> > > > > > This can help us to debug on some corner case.
> > > > > > 
> > > > > > Signed-off-by: Yunlong Song 
> > > > > > Signed-off-by: Chao Yu 
> > > > > > ---
> > > > > >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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > > > Just like this?
> > > > That's OK.
> > > I'm not quite sure whether this is really a bug_on case.
> > > Let me make it WARN_ON() for debugging purpose first.
> > BTW, why is this the special case where BG_GC detects FG_GC?
> > 
> > > Thanks,
> > > 
> > > > >   f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
> > > > > FG_GC);
> > > > > 
> > > > > > ret = -ENODATA;
> > > > > > goto stop;
> > > > > > }
> > > > > > -- 
> > > > > > 1.8.5.2
> > > > > .
> > > > > 
> > > > -- 
> > > > Thanks,
> > > > Yunlong Song
> > > > 
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/07, Yunlong Song wrote:
> Because I find that some out-of-free problem is caused by the failure
> of get victim target. For example, chao has pointed out that he has
> found out a bug when adding this bug_on last week.

That's NOT what I asked. Why not checking FG_GC all the time like this?

f2fs_bug_on(sbi, !total_freed && gc_type == FG_GC);

> 
> On 2017/11/7 10:40, Jaegeuk Kim wrote:
> > On 11/06, Jaegeuk Kim wrote:
> > > On 11/06, Yunlong Song wrote:
> > > > Agree.
> > > > 
> > > > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > > > On 10/13, Yunlong Song wrote:
> > > > > > This can help us to debug on some corner case.
> > > > > > 
> > > > > > Signed-off-by: Yunlong Song 
> > > > > > Signed-off-by: Chao Yu 
> > > > > > ---
> > > > > >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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > > > Just like this?
> > > > That's OK.
> > > I'm not quite sure whether this is really a bug_on case.
> > > Let me make it WARN_ON() for debugging purpose first.
> > BTW, why is this the special case where BG_GC detects FG_GC?
> > 
> > > Thanks,
> > > 
> > > > >   f2fs_bug_on(sbi, !total_freed && !sync && gc_type == 
> > > > > FG_GC);
> > > > > 
> > > > > > ret = -ENODATA;
> > > > > > goto stop;
> > > > > > }
> > > > > > -- 
> > > > > > 1.8.5.2
> > > > > .
> > > > > 
> > > > -- 
> > > > Thanks,
> > > > Yunlong Song
> > > > 
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Yunlong Song 

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Yunlong Song 

Because I find that some out-of-free problem is caused by the failure
of get victim target. For example, chao has pointed out that he has
found out a bug when adding this bug_on last week.

On 2017/11/7 10:40, Jaegeuk Kim wrote:

On 11/06, Jaegeuk Kim wrote:

On 11/06, Yunlong Song wrote:

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?


Thanks,


f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.


--
Thanks,
Yunlong Song


.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/06, Jaegeuk Kim wrote:
> On 11/06, Yunlong Song wrote:
> > Agree.
> > 
> > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > On 10/13, Yunlong Song wrote:
> > > > This can help us to debug on some corner case.
> > > > 
> > > > Signed-off-by: Yunlong Song 
> > > > Signed-off-by: Chao Yu 
> > > > ---
> > > >   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > Just like this?
> > That's OK.
> 
> I'm not quite sure whether this is really a bug_on case.
> Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?

> 
> Thanks,
> 
> > 
> > > 
> > >   f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);
> > > 
> > > > ret = -ENODATA;
> > > > goto stop;
> > > > }
> > > > -- 
> > > > 1.8.5.2
> > > .
> > > 
> > 
> > -- 
> > Thanks,
> > Yunlong Song
> >

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/06, Jaegeuk Kim wrote:
> On 11/06, Yunlong Song wrote:
> > Agree.
> > 
> > On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > > On 10/13, Yunlong Song wrote:
> > > > This can help us to debug on some corner case.
> > > > 
> > > > Signed-off-by: Yunlong Song 
> > > > Signed-off-by: Chao Yu 
> > > > ---
> > > >   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > > +   f2fs_bug_on(sbi, !total_freed && need_fggc);
> > > Just like this?
> > That's OK.
> 
> I'm not quite sure whether this is really a bug_on case.
> Let me make it WARN_ON() for debugging purpose first.

BTW, why is this the special case where BG_GC detects FG_GC?

> 
> Thanks,
> 
> > 
> > > 
> > >   f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);
> > > 
> > > > ret = -ENODATA;
> > > > goto stop;
> > > > }
> > > > -- 
> > > > 1.8.5.2
> > > .
> > > 
> > 
> > -- 
> > Thanks,
> > Yunlong Song
> >

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/06, Yunlong Song wrote:
> Agree.
> 
> On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > On 10/13, Yunlong Song wrote:
> > > This can help us to debug on some corner case.
> > > 
> > > Signed-off-by: Yunlong Song 
> > > Signed-off-by: Chao Yu 
> > > ---
> > >   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > + f2fs_bug_on(sbi, !total_freed && need_fggc);
> > Just like this?
> That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

Thanks,

> 
> > 
> > f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);
> > 
> > >   ret = -ENODATA;
> > >   goto stop;
> > >   }
> > > -- 
> > > 1.8.5.2
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-06 Thread Jaegeuk Kim 
On 11/06, Yunlong Song wrote:
> Agree.
> 
> On 2017/11/3 11:44, Jaegeuk Kim wrote:
> > On 10/13, Yunlong Song wrote:
> > > This can help us to debug on some corner case.
> > > 
> > > Signed-off-by: Yunlong Song 
> > > Signed-off-by: Chao Yu 
> > > ---
> > >   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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> > > + f2fs_bug_on(sbi, !total_freed && need_fggc);
> > Just like this?
> That's OK.

I'm not quite sure whether this is really a bug_on case.
Let me make it WARN_ON() for debugging purpose first.

Thanks,

> 
> > 
> > f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);
> > 
> > >   ret = -ENODATA;
> > >   goto stop;
> > >   }
> > > -- 
> > > 1.8.5.2
> > .
> > 
> 
> -- 
> Thanks,
> Yunlong Song
>

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-05 Thread Yunlong Song 

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.



f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-05 Thread Yunlong Song 

Agree.

On 2017/11/3 11:44, Jaegeuk Kim wrote:

On 10/13, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

That's OK.



f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);


ret = -ENODATA;
goto stop;
}
--
1.8.5.2

.



--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-03 Thread 宋云龙 
OK to me.

> 
> 
>> On 10/13, Yunlong Song wrote:
>> This can help us to debug on some corner case.
>> 
>> Signed-off-by: Yunlong Song 
>> Signed-off-by: Chao Yu 
>> ---
>> 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
>> +f2fs_bug_on(sbi, !total_freed && need_fggc);
> 
> Just like this?
> 
>f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC)；

Agree

> 
>>ret = -ENODATA;
>>goto stop;
>>}
>> -- 
>> 1.8.5.2

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-03 Thread 宋云龙 
OK to me.

> 
> 
>> On 10/13, Yunlong Song wrote:
>> This can help us to debug on some corner case.
>> 
>> Signed-off-by: Yunlong Song 
>> Signed-off-by: Chao Yu 
>> ---
>> 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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
>> +f2fs_bug_on(sbi, !total_freed && need_fggc);
> 
> Just like this?
> 
>f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC)；

Agree

> 
>>ret = -ENODATA;
>>goto stop;
>>}
>> -- 
>> 1.8.5.2

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-02 Thread Yunlong Song 

ping...

On 2017/10/13 21:31, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);
ret = -ENODATA;
goto stop;
}


--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-02 Thread Yunlong Song 

ping...

On 2017/10/13 21:31, Yunlong Song wrote:

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song 
Signed-off-by: Chao Yu 
---
  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
+   f2fs_bug_on(sbi, !total_freed && need_fggc);
ret = -ENODATA;
goto stop;
}


--
Thanks,
Yunlong Song

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-02 Thread Jaegeuk Kim 
On 10/13, Yunlong Song wrote:
> This can help us to debug on some corner case.
> 
> Signed-off-by: Yunlong Song 
> Signed-off-by: Chao Yu 
> ---
>  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> + f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);

>   ret = -ENODATA;
>   goto stop;
>   }
> -- 
> 1.8.5.2

Re: [PATCH v2] f2fs: add bug_on when f2fs_gc even fails to get one victim

2017-11-02 Thread Jaegeuk Kim 
On 10/13, Yunlong Song wrote:
> This can help us to debug on some corner case.
> 
> Signed-off-by: Yunlong Song 
> Signed-off-by: Chao Yu 
> ---
>  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..2b03202 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_fggc = 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_fggc = 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, , gc_type)) {
> + f2fs_bug_on(sbi, !total_freed && need_fggc);

Just like this?

f2fs_bug_on(sbi, !total_freed && !sync && gc_type == FG_GC);

>   ret = -ENODATA;
>   goto stop;
>   }
> -- 
> 1.8.5.2