[PATCH 1/3] f2fs: fix wrong value of tracepoint parameter

[PATCH 1/3] f2fs: fix wrong value of tracepoint parameter

[Chao Yu]

In f2fs_lookup(), we should set @err correctly before printing it
in tracepoint.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/namei.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 63b34a161cf4..2c2f8c36c828 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -504,6 +504,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 			err = PTR_ERR(page);
 			goto out;
 		}
+		err = -ENOENT;
 		goto out_splice;
 	}
 
@@ -549,7 +550,7 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 #endif
 	new = d_splice_alias(inode, dentry);
 	err = PTR_ERR_OR_ZERO(new);
-	trace_f2fs_lookup_end(dir, dentry, ino, err);
+	trace_f2fs_lookup_end(dir, dentry, ino, !new ? -ENOENT : err);
 	return new;
 out_iput:
 	iput(inode);
-- 
2.18.0.rc1

[PATCH 2/3] f2fs: remove unneeded return value of __insert_discard_tree()

[Chao Yu]

We never use return value of __insert_discard_tree(), so remove it.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1c48ec866b8c..ebbadde6cbce 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1221,7 +1221,7 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
 	return err;
 }
 
-static struct discard_cmd *__insert_discard_tree(struct f2fs_sb_info *sbi,
+static void __insert_discard_tree(struct f2fs_sb_info *sbi,
 				struct block_device *bdev, block_t lstart,
 				block_t start, block_t len,
 				struct rb_node **insert_p,
@@ -1230,7 +1230,6 @@ static struct discard_cmd *__insert_discard_tree(struct f2fs_sb_info *sbi,
 	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
 	struct rb_node **p;
 	struct rb_node *parent = NULL;
-	struct discard_cmd *dc = NULL;
 	bool leftmost = true;
 
 	if (insert_p && insert_parent) {
@@ -1242,12 +1241,8 @@ static struct discard_cmd *__insert_discard_tree(struct f2fs_sb_info *sbi,
 	p = f2fs_lookup_rb_tree_for_insert(sbi, &dcc->root, &parent,
 							lstart, &leftmost);
 do_insert:
-	dc = __attach_discard_cmd(sbi, bdev, lstart, start, len, parent,
+	__attach_discard_cmd(sbi, bdev, lstart, start, len, parent,
 								p, leftmost);
-	if (!dc)
-		return NULL;
-
-	return dc;
 }
 
 static void __relocate_discard_cmd(struct discard_cmd_control *dcc,
-- 
2.18.0.rc1

[PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Chao Yu]

meta inode page should be flushed under cp_lock, fix it.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f7de2a1da528..0fcae4d90074 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 		break;
 	case F2FS_GOING_DOWN_METAFLUSH:
+		mutex_lock(&sbi->cp_mutex);
 		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
+		mutex_unlock(&sbi->cp_mutex);
 		f2fs_stop_checkpoint(sbi, false);
 		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 		break;
-- 
2.18.0.rc1

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Jaegeuk Kim]

On 05/27, Chao Yu wrote:
> meta inode page should be flushed under cp_lock, fix it.

It doesn't matter for this case, yes?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/file.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f7de2a1da528..0fcae4d90074 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>  		break;
>  	case F2FS_GOING_DOWN_METAFLUSH:
> +		mutex_lock(&sbi->cp_mutex);
>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
> +		mutex_unlock(&sbi->cp_mutex);
>  		f2fs_stop_checkpoint(sbi, false);
>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>  		break;
> -- 
> 2.18.0.rc1

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Chao Yu]

On 2020/5/28 5:02, Jaegeuk Kim wrote:
> On 05/27, Chao Yu wrote:
>> meta inode page should be flushed under cp_lock, fix it.
> 
> It doesn't matter for this case, yes?

It's not related to discard issue.

Now, I got some progress, I can reproduce that bug occasionally.

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/file.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index f7de2a1da528..0fcae4d90074 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>  		break;
>>  	case F2FS_GOING_DOWN_METAFLUSH:
>> +		mutex_lock(&sbi->cp_mutex);
>>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
>> +		mutex_unlock(&sbi->cp_mutex);
>>  		f2fs_stop_checkpoint(sbi, false);
>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>  		break;
>> -- 
>> 2.18.0.rc1
> .
> 

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Jaegeuk Kim]

On 05/28, Chao Yu wrote:
> On 2020/5/28 5:02, Jaegeuk Kim wrote:
> > On 05/27, Chao Yu wrote:
> >> meta inode page should be flushed under cp_lock, fix it.
> > 
> > It doesn't matter for this case, yes?
> 
> It's not related to discard issue.

I meant we really need this or not. :P

> 
> Now, I got some progress, I can reproduce that bug occasionally.
> 
> Thanks,
> 
> > 
> >>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >>  fs/f2fs/file.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index f7de2a1da528..0fcae4d90074 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
> >>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
> >>  		break;
> >>  	case F2FS_GOING_DOWN_METAFLUSH:
> >> +		mutex_lock(&sbi->cp_mutex);
> >>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
> >> +		mutex_unlock(&sbi->cp_mutex);
> >>  		f2fs_stop_checkpoint(sbi, false);
> >>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
> >>  		break;
> >> -- 
> >> 2.18.0.rc1
> > .
> > 

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Chao Yu]

On 2020/5/28 9:26, Jaegeuk Kim wrote:
> On 05/28, Chao Yu wrote:
>> On 2020/5/28 5:02, Jaegeuk Kim wrote:
>>> On 05/27, Chao Yu wrote:
>>>> meta inode page should be flushed under cp_lock, fix it.
>>>
>>> It doesn't matter for this case, yes?
>>
>> It's not related to discard issue.
> 
> I meant we really need this or not. :P

Yes, let's keep that rule: flush meta pages under cp_lock, otherwise
checkpoint flush order may be broken due to race, right? as checkpoint
should write 2rd cp park page after flushing all meta pages.

> 
>>
>> Now, I got some progress, I can reproduce that bug occasionally.
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>  fs/f2fs/file.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index f7de2a1da528..0fcae4d90074 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
>>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>>>  		break;
>>>>  	case F2FS_GOING_DOWN_METAFLUSH:
>>>> +		mutex_lock(&sbi->cp_mutex);
>>>>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
>>>> +		mutex_unlock(&sbi->cp_mutex);
>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>>>  		break;
>>>> -- 
>>>> 2.18.0.rc1
>>> .
>>>
> .
> 

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Jaegeuk Kim]

On 05/28, Chao Yu wrote:
> On 2020/5/28 9:26, Jaegeuk Kim wrote:
> > On 05/28, Chao Yu wrote:
> >> On 2020/5/28 5:02, Jaegeuk Kim wrote:
> >>> On 05/27, Chao Yu wrote:
> >>>> meta inode page should be flushed under cp_lock, fix it.
> >>>
> >>> It doesn't matter for this case, yes?
> >>
> >> It's not related to discard issue.
> > 
> > I meant we really need this or not. :P
> 
> Yes, let's keep that rule: flush meta pages under cp_lock, otherwise
> checkpoint flush order may be broken due to race, right? as checkpoint
> should write 2rd cp park page after flushing all meta pages.

Well, this is for shutdown test, and thus we don't need to sync up here.

> 
> > 
> >>
> >> Now, I got some progress, I can reproduce that bug occasionally.
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>>> ---
> >>>>  fs/f2fs/file.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>>> index f7de2a1da528..0fcae4d90074 100644
> >>>> --- a/fs/f2fs/file.c
> >>>> +++ b/fs/f2fs/file.c
> >>>> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
> >>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
> >>>>  		break;
> >>>>  	case F2FS_GOING_DOWN_METAFLUSH:
> >>>> +		mutex_lock(&sbi->cp_mutex);
> >>>>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
> >>>> +		mutex_unlock(&sbi->cp_mutex);
> >>>>  		f2fs_stop_checkpoint(sbi, false);
> >>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
> >>>>  		break;
> >>>> -- 
> >>>> 2.18.0.rc1
> >>> .
> >>>
> > .
> > 

Re: [PATCH 3/3] f2fs: fix to cover meta flush with cp_lock

[Chao Yu]

On 2020/5/29 3:00, Jaegeuk Kim wrote:
> On 05/28, Chao Yu wrote:
>> On 2020/5/28 9:26, Jaegeuk Kim wrote:
>>> On 05/28, Chao Yu wrote:
>>>> On 2020/5/28 5:02, Jaegeuk Kim wrote:
>>>>> On 05/27, Chao Yu wrote:
>>>>>> meta inode page should be flushed under cp_lock, fix it.
>>>>>
>>>>> It doesn't matter for this case, yes?
>>>>
>>>> It's not related to discard issue.
>>>
>>> I meant we really need this or not. :P
>>
>> Yes, let's keep that rule: flush meta pages under cp_lock, otherwise
>> checkpoint flush order may be broken due to race, right? as checkpoint
>> should write 2rd cp park page after flushing all meta pages.
> 
> Well, this is for shutdown test, and thus we don't need to sync up here.

I'm a little worried about race condition:

f2fs_write_checkpoint
 do_checkpoint
  ...
					shutdown
					f2fs_sync_meta_pages
					stop_checkpoint
  ...

Though, I haven't figure out potential damage of their racing.

> 
>>
>>>
>>>>
>>>> Now, I got some progress, I can reproduce that bug occasionally.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>>  fs/f2fs/file.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index f7de2a1da528..0fcae4d90074 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -2260,7 +2260,9 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
>>>>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>>>>>  		break;
>>>>>>  	case F2FS_GOING_DOWN_METAFLUSH:
>>>>>> +		mutex_lock(&sbi->cp_mutex);
>>>>>>  		f2fs_sync_meta_pages(sbi, META, LONG_MAX, FS_META_IO);
>>>>>> +		mutex_unlock(&sbi->cp_mutex);
>>>>>>  		f2fs_stop_checkpoint(sbi, false);
>>>>>>  		set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>>>>>>  		break;
>>>>>> -- 
>>>>>> 2.18.0.rc1
>>>>> .
>>>>>
>>> .
>>>
> .
>