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@xxxxxxxxxx>
>>>> ---
>>>> 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