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

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

[Yunlong Song]

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);
 		ret = -ENODATA;
 		goto stop;
 	}
-- 
1.8.5.2

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

[Jaegeuk Kim]

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

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

[Yunlong Song]

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
> .
>

-- 
Thanks,
Yunlong Song

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

[Jaegeuk Kim]

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
> 

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

[Chao Yu]

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;
>  	}
> 

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

[Yunlong Song]

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;
>>   	}
>>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

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;
>>>       }
>>>
>> .
>>
> 

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

[Yunlong Song]

This can help us to debug on some corner case.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
+		f2fs_bug_on(sbi, !total_freed && need_fggc);
 		ret = -ENODATA;
 		goto stop;
 	}
-- 
1.8.5.2

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

[Chao Yu]

On 2017/10/13 21:31, Yunlong Song wrote:
> This can help us to debug on some corner case.

I can hit this bugon with generic/015 of fstest easily, could have a look at
this?

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
> +		f2fs_bug_on(sbi, !total_freed && need_fggc);
>  		ret = -ENODATA;
>  		goto stop;
>  	}
> 

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

[Yunlong Song]

Do you mean check out-of-space test? I have tried that but no bugon.

On 2017/10/14 8:17, Chao Yu wrote:
> On 2017/10/13 21:31, Yunlong Song wrote:
>> This can help us to debug on some corner case.
> I can hit this bugon with generic/015 of fstest easily, could have a look at
> this?
>
> Thanks,
>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
>> +		f2fs_bug_on(sbi, !total_freed && need_fggc);
>>   		ret = -ENODATA;
>>   		goto stop;
>>   	}
>>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

On 2017/10/14 20:34, Yunlong Song wrote:
> Do you mean check out-of-space test? I have tried that but no bugon.

Yes, test recent f2fs codes with kernel 4.13.0-rc1+ in VM, FYI:

kernel BUG at gc.c:1034!
invalid opcode: 0000 [#1] SMP
Hardware name: Xen HVM domU, BIOS 4.1.2_115-900.260_ 11/06/2015
RIP: 0010:f2fs_gc+0x6e5/0x6f0 [f2fs]
RSP: 0018:ffffc90004af7b40 EFLAGS: 00010202
RAX: ffff8801b0a15940 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff8801b0a15940 RSI: ffff8801978d5f00 RDI: ffff880128148048
RBP: ffffc90004af7c38 R08: ffff8801978d5f00 R09: 0000000000000003
R10: 0000000000000003 R11: ffff8800060703a0 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000001 R15: ffff8801b4279800
FS:  00007f23493cb740(0000) GS:ffff880216f00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffd05402ff8 CR3: 00000001bffb3000 CR4: 00000000001406e0
Call Trace:
 f2fs_balance_fs+0x123/0x140 [f2fs]
 f2fs_create+0x130/0x240 [f2fs]
 path_openat+0xee7/0x1360
 do_filp_open+0x7e/0xd0
 do_sys_open+0x115/0x1f0
 SyS_open+0x1e/0x20
 do_syscall_64+0x6e/0x160
 entry_SYSCALL64_slow_path+0x25/0x25

Thanks,

> 
> On 2017/10/14 8:17, Chao Yu wrote:
>> On 2017/10/13 21:31, Yunlong Song wrote:
>>> This can help us to debug on some corner case.
>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>> this?
>>
>> Thanks,
>>
>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
>>> +		f2fs_bug_on(sbi, !total_freed && need_fggc);
>>>   		ret = -ENODATA;
>>>   		goto stop;
>>>   	}
>>>
>> .
>>
> 

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

[Yunlong Song]

I think there may be bugs somewhere, since no victim is selected but it 
really needs gc.
What is the size of the data image?

On 2017/10/16 11:25, Chao Yu wrote:
> On 2017/10/14 20:34, Yunlong Song wrote:
>> Do you mean check out-of-space test? I have tried that but no bugon.
> Yes, test recent f2fs codes with kernel 4.13.0-rc1+ in VM, FYI:
>
> kernel BUG at gc.c:1034!
> invalid opcode: 0000 [#1] SMP
> Hardware name: Xen HVM domU, BIOS 4.1.2_115-900.260_ 11/06/2015
> RIP: 0010:f2fs_gc+0x6e5/0x6f0 [f2fs]
> RSP: 0018:ffffc90004af7b40 EFLAGS: 00010202
> RAX: ffff8801b0a15940 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: ffff8801b0a15940 RSI: ffff8801978d5f00 RDI: ffff880128148048
> RBP: ffffc90004af7c38 R08: ffff8801978d5f00 R09: 0000000000000003
> R10: 0000000000000003 R11: ffff8800060703a0 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000001 R15: ffff8801b4279800
> FS:  00007f23493cb740(0000) GS:ffff880216f00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ffd05402ff8 CR3: 00000001bffb3000 CR4: 00000000001406e0
> Call Trace:
>   f2fs_balance_fs+0x123/0x140 [f2fs]
>   f2fs_create+0x130/0x240 [f2fs]
>   path_openat+0xee7/0x1360
>   do_filp_open+0x7e/0xd0
>   do_sys_open+0x115/0x1f0
>   SyS_open+0x1e/0x20
>   do_syscall_64+0x6e/0x160
>   entry_SYSCALL64_slow_path+0x25/0x25
>
> Thanks,
>
>> On 2017/10/14 8:17, Chao Yu wrote:
>>> On 2017/10/13 21:31, Yunlong Song wrote:
>>>> This can help us to debug on some corner case.
>>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>>> this?
>>>
>>> Thanks,
>>>
>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
>>>> +		f2fs_bug_on(sbi, !total_freed && need_fggc);
>>>>    		ret = -ENODATA;
>>>>    		goto stop;
>>>>    	}
>>>>
>>> .
>>>
>
> .
>

-- 
Thanks,
Yunlong Song

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

[Chao Yu]

On 2017/10/31 9:32, Yunlong Song wrote:
> I think there may be bugs somewhere, since no victim is selected but it 
> really needs gc.
> What is the size of the data image?

I have providered the testcase, could you check that?

>>>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>>>> this?

Anyway, I have looked into this issue at last weekend, forgot to post related
info. It shows that if the image size is very small, e.g. 50MB, current
segment will encroach overprov and reserved segments, so free segment count will
always be small than reserved segment threshold, result in triggering bugon in
FGGC.

[SB: 1] [CP: 2] [SIT: 2] [NAT: 2] [SSA: 1] [MAIN: 17(OverProv:14 Resv:12)]

  - Valid: 6
  - Dirty: 0
  - Prefree: 0
  - Free: 11 (11)

I modify mkfs.f2fs forcedly as below, which could let generic/015 passed:

diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
index 8a821d3a1747..774bbef73d9a 100644
--- a/mkfs/f2fs_format.c
+++ b/mkfs/f2fs_format.c
@@ -371,6 +371,12 @@ static int f2fs_prepare_super_block(void)
 			(2 * (100 / c.overprovision + 1) + 6)
 			* c.segs_per_sec;

+	if (get_sb(segment_count_main) * (100 - c.overprovision) / 100 -
+		c.reserved_segments < 6) {
+		c.overprovision = 50;
+		c.reserved_segments = get_sb(segment_count_main) - 6 * 2;
+	}
+
 	uuid_generate(sb->uuid);

 	/* precompute checksum seed for metadata */

Thanks,

> 
> On 2017/10/16 11:25, Chao Yu wrote:
>> On 2017/10/14 20:34, Yunlong Song wrote:
>>> Do you mean check out-of-space test? I have tried that but no bugon.
>> Yes, test recent f2fs codes with kernel 4.13.0-rc1+ in VM, FYI:
>>
>> kernel BUG at gc.c:1034!
>> invalid opcode: 0000 [#1] SMP
>> Hardware name: Xen HVM domU, BIOS 4.1.2_115-900.260_ 11/06/2015
>> RIP: 0010:f2fs_gc+0x6e5/0x6f0 [f2fs]
>> RSP: 0018:ffffc90004af7b40 EFLAGS: 00010202
>> RAX: ffff8801b0a15940 RBX: 0000000000000000 RCX: 0000000000000000
>> RDX: ffff8801b0a15940 RSI: ffff8801978d5f00 RDI: ffff880128148048
>> RBP: ffffc90004af7c38 R08: ffff8801978d5f00 R09: 0000000000000003
>> R10: 0000000000000003 R11: ffff8800060703a0 R12: 0000000000000000
>> R13: 0000000000000000 R14: 0000000000000001 R15: ffff8801b4279800
>> FS:  00007f23493cb740(0000) GS:ffff880216f00000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007ffd05402ff8 CR3: 00000001bffb3000 CR4: 00000000001406e0
>> Call Trace:
>>   f2fs_balance_fs+0x123/0x140 [f2fs]
>>   f2fs_create+0x130/0x240 [f2fs]
>>   path_openat+0xee7/0x1360
>>   do_filp_open+0x7e/0xd0
>>   do_sys_open+0x115/0x1f0
>>   SyS_open+0x1e/0x20
>>   do_syscall_64+0x6e/0x160
>>   entry_SYSCALL64_slow_path+0x25/0x25
>>
>> Thanks,
>>
>>> On 2017/10/14 8:17, Chao Yu wrote:
>>>> On 2017/10/13 21:31, Yunlong Song wrote:
>>>>> This can help us to debug on some corner case.
>>>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>>>> this?
>>>>
>>>> Thanks,
>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, gc_type)) {
>>>>> +		f2fs_bug_on(sbi, !total_freed && need_fggc);
>>>>>    		ret = -ENODATA;
>>>>>    		goto stop;
>>>>>    	}
>>>>>
>>>> .
>>>>
>>
>> .
>>
> 

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

[Yunlong Song]

So it seems it is really useful to add this bug_on in gc.

On 2017/10/31 11:17, Chao Yu wrote:
> On 2017/10/31 9:32, Yunlong Song wrote:
>> I think there may be bugs somewhere, since no victim is selected but it
>> really needs gc.
>> What is the size of the data image?
> I have providered the testcase, could you check that?
>
>>>>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>>>>> this?
> Anyway, I have looked into this issue at last weekend, forgot to post related
> info. It shows that if the image size is very small, e.g. 50MB, current
> segment will encroach overprov and reserved segments, so free segment count will
> always be small than reserved segment threshold, result in triggering bugon in
> FGGC.
>
> [SB: 1] [CP: 2] [SIT: 2] [NAT: 2] [SSA: 1] [MAIN: 17(OverProv:14 Resv:12)]
>
>    - Valid: 6
>    - Dirty: 0
>    - Prefree: 0
>    - Free: 11 (11)
>
> I modify mkfs.f2fs forcedly as below, which could let generic/015 passed:
>
> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
> index 8a821d3a1747..774bbef73d9a 100644
> --- a/mkfs/f2fs_format.c
> +++ b/mkfs/f2fs_format.c
> @@ -371,6 +371,12 @@ static int f2fs_prepare_super_block(void)
>   			(2 * (100 / c.overprovision + 1) + 6)
>   			* c.segs_per_sec;
>
> +	if (get_sb(segment_count_main) * (100 - c.overprovision) / 100 -
> +		c.reserved_segments < 6) {
> +		c.overprovision = 50;
> +		c.reserved_segments = get_sb(segment_count_main) - 6 * 2;
> +	}
> +
>   	uuid_generate(sb->uuid);
>
>   	/* precompute checksum seed for metadata */
>
> Thanks,
>
>> On 2017/10/16 11:25, Chao Yu wrote:
>>> On 2017/10/14 20:34, Yunlong Song wrote:
>>>> Do you mean check out-of-space test? I have tried that but no bugon.
>>> Yes, test recent f2fs codes with kernel 4.13.0-rc1+ in VM, FYI:
>>>
>>> kernel BUG at gc.c:1034!
>>> invalid opcode: 0000 [#1] SMP
>>> Hardware name: Xen HVM domU, BIOS 4.1.2_115-900.260_ 11/06/2015
>>> RIP: 0010:f2fs_gc+0x6e5/0x6f0 [f2fs]
>>> RSP: 0018:ffffc90004af7b40 EFLAGS: 00010202
>>> RAX: ffff8801b0a15940 RBX: 0000000000000000 RCX: 0000000000000000
>>> RDX: ffff8801b0a15940 RSI: ffff8801978d5f00 RDI: ffff880128148048
>>> RBP: ffffc90004af7c38 R08: ffff8801978d5f00 R09: 0000000000000003
>>> R10: 0000000000000003 R11: ffff8800060703a0 R12: 0000000000000000
>>> R13: 0000000000000000 R14: 0000000000000001 R15: ffff8801b4279800
>>> FS:  00007f23493cb740(0000) GS:ffff880216f00000(0000) knlGS:0000000000000000
>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 00007ffd05402ff8 CR3: 00000001bffb3000 CR4: 00000000001406e0
>>> Call Trace:
>>>    f2fs_balance_fs+0x123/0x140 [f2fs]
>>>    f2fs_create+0x130/0x240 [f2fs]
>>>    path_openat+0xee7/0x1360
>>>    do_filp_open+0x7e/0xd0
>>>    do_sys_open+0x115/0x1f0
>>>    SyS_open+0x1e/0x20
>>>    do_syscall_64+0x6e/0x160
>>>    entry_SYSCALL64_slow_path+0x25/0x25
>>>
>>> Thanks,
>>>
>>>> On 2017/10/14 8:17, Chao Yu wrote:
>>>>> On 2017/10/13 21:31, Yunlong Song wrote:
>>>>>> This can help us to debug on some corner case.
>>>>> I can hit this bugon with generic/015 of fstest easily, could have a look at
>>>>> this?
>>>>>
>>>>> Thanks,
>>>>>
>>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[Jaegeuk Kim]

On 10/13, Yunlong Song wrote:
> This can help us to debug on some corner case.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[宋云龙]

OK to me.

> 
> 
>> On 10/13, Yunlong Song wrote:
>> This can help us to debug on some corner case.
>> 
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
> > > Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
> > > > Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
> > > > > > Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>>>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>>>>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
> > > > > > > > > > Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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

[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 <yunlong.song@huawei.com>
>>>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@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..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, &segno, 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