[PATCH 1/3] f2fs: clean up destroy_discard_cmd_control

[PATCH 1/3] f2fs: clean up destroy_discard_cmd_control

[Chao Yu]

Remove unneeded parameter and simply change flow in
destroy_discard_cmd_control.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ff53423e35fb..c1a03b0857f9 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1181,20 +1181,22 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
 	return err;
 }
 
-static void destroy_discard_cmd_control(struct f2fs_sb_info *sbi, bool free)
+static void destroy_discard_cmd_control(struct f2fs_sb_info *sbi)
 {
 	struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
 
-	if (dcc && dcc->f2fs_issue_discard) {
+	if (!dcc)
+		return;
+
+	if (dcc->f2fs_issue_discard) {
 		struct task_struct *discard_thread = dcc->f2fs_issue_discard;
 
 		dcc->f2fs_issue_discard = NULL;
 		kthread_stop(discard_thread);
 	}
-	if (free) {
-		kfree(dcc);
-		SM_I(sbi)->dcc_info = NULL;
-	}
+
+	kfree(dcc);
+	SM_I(sbi)->dcc_info = NULL;
 }
 
 static bool __mark_sit_entry_dirty(struct f2fs_sb_info *sbi, unsigned int segno)
@@ -3086,7 +3088,7 @@ void destroy_segment_manager(struct f2fs_sb_info *sbi)
 	if (!sm_info)
 		return;
 	destroy_flush_cmd_control(sbi, true);
-	destroy_discard_cmd_control(sbi, true);
+	destroy_discard_cmd_control(sbi);
 	destroy_dirty_segmap(sbi);
 	destroy_curseg(sbi);
 	destroy_free_segmap(sbi);
-- 
2.8.2.295.g3f1c1d0

[PATCH 2/3] f2fs: shrink blk plug region

[Chao Yu]

Don't use blk plug covering area where there won't be any IOs being issued.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index c1a03b0857f9..57a81f9c8c14 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -848,9 +848,8 @@ static int issue_discard_thread(void *data)
 	if (kthread_should_stop())
 		return 0;
 
-	blk_start_plug(&plug);
-
 	mutex_lock(&dcc->cmd_lock);
+	blk_start_plug(&plug);
 	list_for_each_entry_safe(dc, tmp, pend_list, list) {
 		f2fs_bug_on(sbi, dc->state != D_PREP);
 
@@ -860,6 +859,7 @@ static int issue_discard_thread(void *data)
 		if (iter++ > DISCARD_ISSUE_RATE)
 			break;
 	}
+	blk_finish_plug(&plug);
 
 	list_for_each_entry_safe(dc, tmp, wait_list, list) {
 		if (dc->state == D_DONE)
@@ -867,8 +867,6 @@ static int issue_discard_thread(void *data)
 	}
 	mutex_unlock(&dcc->cmd_lock);
 
-	blk_finish_plug(&plug);
-
 	iter = 0;
 	congestion_wait(BLK_RW_SYNC, HZ/50);
 
-- 
2.8.2.295.g3f1c1d0

[PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Chao Yu]

In f2fs_submit_discard_endio, we will wake up waiter before setting
discard command states, so waiter may use incorrect states. Change
the order between complete() and states setting to fix this issue.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 57a81f9c8c14..9f9542c9fe47 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
 {
 	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
 
-	complete(&dc->wait);
 	dc->error = bio->bi_error;
 	dc->state = D_DONE;
+	complete(&dc->wait);
 	bio_put(bio);
 }
 
-- 
2.8.2.295.g3f1c1d0

Re: [PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Jaegeuk Kim]

On 03/27, Chao Yu wrote:
> In f2fs_submit_discard_endio, we will wake up waiter before setting
> discard command states, so waiter may use incorrect states. Change
> the order between complete() and states setting to fix this issue.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 57a81f9c8c14..9f9542c9fe47 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>  {
>  	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>  
> -	complete(&dc->wait);
>  	dc->error = bio->bi_error;
>  	dc->state = D_DONE;
> +	complete(&dc->wait);

If we set D_DONE first, the object can be released by __remove_discard_cmd()?

Thanks,

>  	bio_put(bio);
>  }
>  
> -- 
> 2.8.2.295.g3f1c1d0

Re: [PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Chao Yu]

On 2017/3/28 7:56, Jaegeuk Kim wrote:
> On 03/27, Chao Yu wrote:
>> In f2fs_submit_discard_endio, we will wake up waiter before setting
>> discard command states, so waiter may use incorrect states. Change
>> the order between complete() and states setting to fix this issue.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/segment.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 57a81f9c8c14..9f9542c9fe47 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>>  {
>>  	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>>  
>> -	complete(&dc->wait);
>>  	dc->error = bio->bi_error;
>>  	dc->state = D_DONE;
>> +	complete(&dc->wait);
> 
> If we set D_DONE first, the object can be released by __remove_discard_cmd()?

Yes, I think so.

Thanks,

> 
> Thanks,
> 
>>  	bio_put(bio);
>>  }
>>  
>> -- 
>> 2.8.2.295.g3f1c1d0
> 
> .
> 

Re: [PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Chao Yu]

Ping,

Any problem here?

Thanks,

On 2017/3/28 9:17, Chao Yu wrote:
> On 2017/3/28 7:56, Jaegeuk Kim wrote:
>> On 03/27, Chao Yu wrote:
>>> In f2fs_submit_discard_endio, we will wake up waiter before setting
>>> discard command states, so waiter may use incorrect states. Change
>>> the order between complete() and states setting to fix this issue.
>>>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>>  fs/f2fs/segment.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 57a81f9c8c14..9f9542c9fe47 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>>>  {
>>>  	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>>>  
>>> -	complete(&dc->wait);
>>>  	dc->error = bio->bi_error;
>>>  	dc->state = D_DONE;
>>> +	complete(&dc->wait);
>>
>> If we set D_DONE first, the object can be released by __remove_discard_cmd()?
> 
> Yes, I think so.
> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>  	bio_put(bio);
>>>  }
>>>  
>>> -- 
>>> 2.8.2.295.g3f1c1d0
>>
>> .
>>

Re: [PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Jaegeuk Kim]

On 04/01, Chao Yu wrote:
> Ping,
> 
> Any problem here?
> 
> Thanks,
> 
> On 2017/3/28 9:17, Chao Yu wrote:
> > On 2017/3/28 7:56, Jaegeuk Kim wrote:
> >> On 03/27, Chao Yu wrote:
> >>> In f2fs_submit_discard_endio, we will wake up waiter before setting
> >>> discard command states, so waiter may use incorrect states. Change
> >>> the order between complete() and states setting to fix this issue.
> >>>
> >>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >>> ---
> >>>  fs/f2fs/segment.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index 57a81f9c8c14..9f9542c9fe47 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
> >>>  {
> >>>  	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> >>>  
> >>> -	complete(&dc->wait);
> >>>  	dc->error = bio->bi_error;
> >>>  	dc->state = D_DONE;
> >>> +	complete(&dc->wait);
> >>
> >> If we set D_DONE first, the object can be released by __remove_discard_cmd()?

What I mean was about use-after-free.

Thanks,

> > 
> > Yes, I think so.
> > 
> > Thanks,
> > 
> >>
> >> Thanks,
> >>
> >>>  	bio_put(bio);
> >>>  }
> >>>  
> >>> -- 
> >>> 2.8.2.295.g3f1c1d0
> >>
> >> .
> >>

Re: [PATCH 3/3] f2fs: prevent waiter encountering incorrect discard states

[Chao Yu]

On 2017/4/4 1:40, Jaegeuk Kim wrote:
> On 04/01, Chao Yu wrote:
>> Ping,
>>
>> Any problem here?
>>
>> Thanks,
>>
>> On 2017/3/28 9:17, Chao Yu wrote:
>>> On 2017/3/28 7:56, Jaegeuk Kim wrote:
>>>> On 03/27, Chao Yu wrote:
>>>>> In f2fs_submit_discard_endio, we will wake up waiter before setting
>>>>> discard command states, so waiter may use incorrect states. Change
>>>>> the order between complete() and states setting to fix this issue.
>>>>>
>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>> ---
>>>>>  fs/f2fs/segment.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>> index 57a81f9c8c14..9f9542c9fe47 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -717,9 +717,9 @@ static void f2fs_submit_discard_endio(struct bio *bio)
>>>>>  {
>>>>>  	struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
>>>>>  
>>>>> -	complete(&dc->wait);
>>>>>  	dc->error = bio->bi_error;
>>>>>  	dc->state = D_DONE;
>>>>> +	complete(&dc->wait);
>>>>
>>>> If we set D_DONE first, the object can be released by __remove_discard_cmd()?
> 
> What I mean was about use-after-free.

I updated the patch, could you help to review it?

Thanks,

> 
> Thanks,
> 
>>>
>>> Yes, I think so.
>>>
>>> Thanks,
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>  	bio_put(bio);
>>>>>  }
>>>>>  
>>>>> -- 
>>>>> 2.8.2.295.g3f1c1d0
>>>>
>>>> .
>>>>
> 
> .
> 

Re: [PATCH 2/3] f2fs: shrink blk plug region

[Chao Yu]

Hi Jaegeuk,

Just reminding, miss merging this patch? :)

Thanks,

On 2017/3/27 18:14, Chao Yu wrote:
> Don't use blk plug covering area where there won't be any IOs being issued.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/segment.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index c1a03b0857f9..57a81f9c8c14 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -848,9 +848,8 @@ static int issue_discard_thread(void *data)
>  	if (kthread_should_stop())
>  		return 0;
>  
> -	blk_start_plug(&plug);
> -
>  	mutex_lock(&dcc->cmd_lock);
> +	blk_start_plug(&plug);
>  	list_for_each_entry_safe(dc, tmp, pend_list, list) {
>  		f2fs_bug_on(sbi, dc->state != D_PREP);
>  
> @@ -860,6 +859,7 @@ static int issue_discard_thread(void *data)
>  		if (iter++ > DISCARD_ISSUE_RATE)
>  			break;
>  	}
> +	blk_finish_plug(&plug);
>  
>  	list_for_each_entry_safe(dc, tmp, wait_list, list) {
>  		if (dc->state == D_DONE)
> @@ -867,8 +867,6 @@ static int issue_discard_thread(void *data)
>  	}
>  	mutex_unlock(&dcc->cmd_lock);
>  
> -	blk_finish_plug(&plug);
> -
>  	iter = 0;
>  	congestion_wait(BLK_RW_SYNC, HZ/50);
>  
> 

Re: [PATCH 2/3] f2fs: shrink blk plug region

[Jaegeuk Kim]

On 04/12, Chao Yu wrote:
> Hi Jaegeuk,
> 
> Just reminding, miss merging this patch? :)

Yup, merged. ;)

> 
> Thanks,
> 
> On 2017/3/27 18:14, Chao Yu wrote:
> > Don't use blk plug covering area where there won't be any IOs being issued.
> > 
> > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > ---
> >  fs/f2fs/segment.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index c1a03b0857f9..57a81f9c8c14 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -848,9 +848,8 @@ static int issue_discard_thread(void *data)
> >  	if (kthread_should_stop())
> >  		return 0;
> >  
> > -	blk_start_plug(&plug);
> > -
> >  	mutex_lock(&dcc->cmd_lock);
> > +	blk_start_plug(&plug);
> >  	list_for_each_entry_safe(dc, tmp, pend_list, list) {
> >  		f2fs_bug_on(sbi, dc->state != D_PREP);
> >  
> > @@ -860,6 +859,7 @@ static int issue_discard_thread(void *data)
> >  		if (iter++ > DISCARD_ISSUE_RATE)
> >  			break;
> >  	}
> > +	blk_finish_plug(&plug);
> >  
> >  	list_for_each_entry_safe(dc, tmp, wait_list, list) {
> >  		if (dc->state == D_DONE)
> > @@ -867,8 +867,6 @@ static int issue_discard_thread(void *data)
> >  	}
> >  	mutex_unlock(&dcc->cmd_lock);
> >  
> > -	blk_finish_plug(&plug);
> > -
> >  	iter = 0;
> >  	congestion_wait(BLK_RW_SYNC, HZ/50);
> >  
> >