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

From: Chao Yu
Date: Fri Jul 06 2018 - 21:42:23 EST


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