[PATCH v2] f2fs: give another chance to issue discard with current granularity

[PATCH v2] f2fs: give another chance to issue discard with current granularity

[Chao Yu]

If discard IOs are blocked by user IO, do not skip to select and issue
discard with lower granularity, retry with current granularity.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1f9ae8270f86..6e2b2e717a40 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1191,7 +1191,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 	struct discard_cmd *dc, *tmp;
 	struct blk_plug plug;
 	int i, iter = 0, issued = 0;
-	bool io_interrupted = false;
+	bool io_interrupted = false, end_up = false;
 
 	for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
 		if (i + 1 < dpolicy->granularity)
@@ -1199,6 +1199,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 		pend_list = &dcc->pend_list[i];
 
 		mutex_lock(&dcc->cmd_lock);
+retry:
 		if (list_empty(pend_list))
 			goto next;
 		if (unlikely(dcc->rbtree_check))
@@ -1217,14 +1218,23 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
 			__submit_discard_cmd(sbi, dpolicy, dc);
 			issued++;
 skip:
-			if (++iter >= dpolicy->max_requests)
+			if (++iter >= dpolicy->max_requests) {
+				end_up = true;
 				break;
+			}
 		}
 		blk_finish_plug(&plug);
+
+		/*
+		 * if discard IO was interrupted by user IOs, give another
+		 * chance to issue discard with current granularity.
+		 */
+		if (io_interrupted && !end_up)
+			goto retry;
 next:
 		mutex_unlock(&dcc->cmd_lock);
 
-		if (iter >= dpolicy->max_requests)
+		if (end_up)
 			break;
 	}
 
-- 
2.18.0.rc1

Re: [PATCH v2] f2fs: give another chance to issue discard with current granularity

[Jaegeuk Kim]

On 07/05, Chao Yu wrote:
> If discard IOs are blocked by user IO, do not skip to select and issue
> discard with lower granularity, retry with current granularity.

We need to stop as soon as possible since user activity comes. Later, discard
thread will try it again in another idle time. What's your concern?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> v2:
> - fix deadloop.
>  fs/f2fs/segment.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 1f9ae8270f86..6e2b2e717a40 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1191,7 +1191,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>  	struct discard_cmd *dc, *tmp;
>  	struct blk_plug plug;
>  	int i, iter = 0, issued = 0;
> -	bool io_interrupted = false;
> +	bool io_interrupted = false, end_up = false;
>  
>  	for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
>  		if (i + 1 < dpolicy->granularity)
> @@ -1199,6 +1199,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>  		pend_list = &dcc->pend_list[i];
>  
>  		mutex_lock(&dcc->cmd_lock);
> +retry:
>  		if (list_empty(pend_list))
>  			goto next;
>  		if (unlikely(dcc->rbtree_check))
> @@ -1217,14 +1218,23 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>  			__submit_discard_cmd(sbi, dpolicy, dc);
>  			issued++;
>  skip:
> -			if (++iter >= dpolicy->max_requests)
> +			if (++iter >= dpolicy->max_requests) {
> +				end_up = true;
>  				break;
> +			}
>  		}
>  		blk_finish_plug(&plug);
> +
> +		/*
> +		 * if discard IO was interrupted by user IOs, give another
> +		 * chance to issue discard with current granularity.
> +		 */
> +		if (io_interrupted && !end_up)
> +			goto retry;
>  next:
>  		mutex_unlock(&dcc->cmd_lock);
>  
> -		if (iter >= dpolicy->max_requests)
> +		if (end_up)
>  			break;
>  	}
>  
> -- 
> 2.18.0.rc1

Re: [PATCH v2] f2fs: give another chance to issue discard with current granularity

[Chao Yu]

Hi Jaegeuk,

On 2018/7/7 6:49, Jaegeuk Kim wrote:
> On 07/05, Chao Yu wrote:
>> If discard IOs are blocked by user IO, do not skip to select and issue
>> discard with lower granularity, retry with current granularity.
> 
> We need to stop as soon as possible since user activity comes. Later, discard
> thread will try it again in another idle time. What's your concern?

Currently, our implementation is that we will try to issue
DEF_MAX_DISCARD_REQUEST discards as many as possible in batch, for example:

If there is 4 discard entry in rb-tree, we will try to issue them by below order
in batch:

No.		size		is_idle		op
#1 discard	2MB		false		skip
#2 discard	2MB		false		skip
#3 discard	256KB		true		issue
#4 discard	16KB		true		issue

So if is_idle is false temporarily, we can still have chance to issue #3 & #4
discard in this round once is_idle is flipped?

But the problem is we skip issue discard with big granularity, so I add this
patch to do retry with current granularity.

Do you mean that we need to stop issuing discard immediately once we detect IO
is busy?

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> v2:
>> - fix deadloop.
>>  fs/f2fs/segment.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 1f9ae8270f86..6e2b2e717a40 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1191,7 +1191,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>  	struct discard_cmd *dc, *tmp;
>>  	struct blk_plug plug;
>>  	int i, iter = 0, issued = 0;
>> -	bool io_interrupted = false;
>> +	bool io_interrupted = false, end_up = false;
>>  
>>  	for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
>>  		if (i + 1 < dpolicy->granularity)
>> @@ -1199,6 +1199,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>  		pend_list = &dcc->pend_list[i];
>>  
>>  		mutex_lock(&dcc->cmd_lock);
>> +retry:
>>  		if (list_empty(pend_list))
>>  			goto next;
>>  		if (unlikely(dcc->rbtree_check))
>> @@ -1217,14 +1218,23 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>  			__submit_discard_cmd(sbi, dpolicy, dc);
>>  			issued++;
>>  skip:
>> -			if (++iter >= dpolicy->max_requests)
>> +			if (++iter >= dpolicy->max_requests) {
>> +				end_up = true;
>>  				break;
>> +			}
>>  		}
>>  		blk_finish_plug(&plug);
>> +
>> +		/*
>> +		 * if discard IO was interrupted by user IOs, give another
>> +		 * chance to issue discard with current granularity.
>> +		 */
>> +		if (io_interrupted && !end_up)
>> +			goto retry;
>>  next:
>>  		mutex_unlock(&dcc->cmd_lock);
>>  
>> -		if (iter >= dpolicy->max_requests)
>> +		if (end_up)
>>  			break;
>>  	}
>>  
>> -- 
>> 2.18.0.rc1

Re: [PATCH v2] f2fs: give another chance to issue discard with current granularity

[Jaegeuk Kim]

On 07/07, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2018/7/7 6:49, Jaegeuk Kim wrote:
> > On 07/05, Chao Yu wrote:
> >> If discard IOs are blocked by user IO, do not skip to select and issue
> >> discard with lower granularity, retry with current granularity.
> > 
> > We need to stop as soon as possible since user activity comes. Later, discard
> > thread will try it again in another idle time. What's your concern?
> 
> Currently, our implementation is that we will try to issue
> DEF_MAX_DISCARD_REQUEST discards as many as possible in batch, for example:
> 
> If there is 4 discard entry in rb-tree, we will try to issue them by below order
> in batch:
> 
> No.		size		is_idle		op
> #1 discard	2MB		false		skip
> #2 discard	2MB		false		skip
> #3 discard	256KB		true		issue
> #4 discard	16KB		true		issue
> 
> So if is_idle is false temporarily, we can still have chance to issue #3 & #4
> discard in this round once is_idle is flipped?
> 
> But the problem is we skip issue discard with big granularity, so I add this
> patch to do retry with current granularity.
> 
> Do you mean that we need to stop issuing discard immediately once we detect IO
> is busy?

Yes. If we retry to issue discard continuously, it hurts normal IO latencies.

> 
> Thanks,
> 
> > 
> >>
> >> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >> v2:
> >> - fix deadloop.
> >>  fs/f2fs/segment.c | 16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >> index 1f9ae8270f86..6e2b2e717a40 100644
> >> --- a/fs/f2fs/segment.c
> >> +++ b/fs/f2fs/segment.c
> >> @@ -1191,7 +1191,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >>  	struct discard_cmd *dc, *tmp;
> >>  	struct blk_plug plug;
> >>  	int i, iter = 0, issued = 0;
> >> -	bool io_interrupted = false;
> >> +	bool io_interrupted = false, end_up = false;
> >>  
> >>  	for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
> >>  		if (i + 1 < dpolicy->granularity)
> >> @@ -1199,6 +1199,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >>  		pend_list = &dcc->pend_list[i];
> >>  
> >>  		mutex_lock(&dcc->cmd_lock);
> >> +retry:
> >>  		if (list_empty(pend_list))
> >>  			goto next;
> >>  		if (unlikely(dcc->rbtree_check))
> >> @@ -1217,14 +1218,23 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
> >>  			__submit_discard_cmd(sbi, dpolicy, dc);
> >>  			issued++;
> >>  skip:
> >> -			if (++iter >= dpolicy->max_requests)
> >> +			if (++iter >= dpolicy->max_requests) {
> >> +				end_up = true;
> >>  				break;
> >> +			}
> >>  		}
> >>  		blk_finish_plug(&plug);
> >> +
> >> +		/*
> >> +		 * if discard IO was interrupted by user IOs, give another
> >> +		 * chance to issue discard with current granularity.
> >> +		 */
> >> +		if (io_interrupted && !end_up)
> >> +			goto retry;
> >>  next:
> >>  		mutex_unlock(&dcc->cmd_lock);
> >>  
> >> -		if (iter >= dpolicy->max_requests)
> >> +		if (end_up)
> >>  			break;
> >>  	}
> >>  
> >> -- 
> >> 2.18.0.rc1

Re: [PATCH v2] f2fs: give another chance to issue discard with current granularity

[Chao Yu]

On 2018/7/7 9:10, Jaegeuk Kim wrote:
> On 07/07, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2018/7/7 6:49, Jaegeuk Kim wrote:
>>> On 07/05, Chao Yu wrote:
>>>> If discard IOs are blocked by user IO, do not skip to select and issue
>>>> discard with lower granularity, retry with current granularity.
>>>
>>> We need to stop as soon as possible since user activity comes. Later, discard
>>> thread will try it again in another idle time. What's your concern?
>>
>> Currently, our implementation is that we will try to issue
>> DEF_MAX_DISCARD_REQUEST discards as many as possible in batch, for example:
>>
>> If there is 4 discard entry in rb-tree, we will try to issue them by below order
>> in batch:
>>
>> No.		size		is_idle		op
>> #1 discard	2MB		false		skip
>> #2 discard	2MB		false		skip
>> #3 discard	256KB		true		issue
>> #4 discard	16KB		true		issue
>>
>> So if is_idle is false temporarily, we can still have chance to issue #3 & #4
>> discard in this round once is_idle is flipped?
>>
>> But the problem is we skip issue discard with big granularity, so I add this
>> patch to do retry with current granularity.
>>
>> Do you mean that we need to stop issuing discard immediately once we detect IO
>> is busy?
> 
> Yes. If we retry to issue discard continuously, it hurts normal IO latencies.

OK, I will send another patch to change the logic to terminate issuing discard
immediately if there is queued IO.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>> v2:
>>>> - fix deadloop.
>>>>  fs/f2fs/segment.c | 16 +++++++++++++---
>>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 1f9ae8270f86..6e2b2e717a40 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -1191,7 +1191,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>>>  	struct discard_cmd *dc, *tmp;
>>>>  	struct blk_plug plug;
>>>>  	int i, iter = 0, issued = 0;
>>>> -	bool io_interrupted = false;
>>>> +	bool io_interrupted = false, end_up = false;
>>>>  
>>>>  	for (i = MAX_PLIST_NUM - 1; i >= 0; i--) {
>>>>  		if (i + 1 < dpolicy->granularity)
>>>> @@ -1199,6 +1199,7 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>>>  		pend_list = &dcc->pend_list[i];
>>>>  
>>>>  		mutex_lock(&dcc->cmd_lock);
>>>> +retry:
>>>>  		if (list_empty(pend_list))
>>>>  			goto next;
>>>>  		if (unlikely(dcc->rbtree_check))
>>>> @@ -1217,14 +1218,23 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>>>>  			__submit_discard_cmd(sbi, dpolicy, dc);
>>>>  			issued++;
>>>>  skip:
>>>> -			if (++iter >= dpolicy->max_requests)
>>>> +			if (++iter >= dpolicy->max_requests) {
>>>> +				end_up = true;
>>>>  				break;
>>>> +			}
>>>>  		}
>>>>  		blk_finish_plug(&plug);
>>>> +
>>>> +		/*
>>>> +		 * if discard IO was interrupted by user IOs, give another
>>>> +		 * chance to issue discard with current granularity.
>>>> +		 */
>>>> +		if (io_interrupted && !end_up)
>>>> +			goto retry;
>>>>  next:
>>>>  		mutex_unlock(&dcc->cmd_lock);
>>>>  
>>>> -		if (iter >= dpolicy->max_requests)
>>>> +		if (end_up)
>>>>  			break;
>>>>  	}
>>>>  
>>>> -- 
>>>> 2.18.0.rc1