Re: [PATCH] f2fs: fix to redirty page if fail to gc data page

[Chao Yu]

On 2016/6/3 13:17, Jaegeuk Kim wrote:
> On Fri, Jun 03, 2016 at 01:13:21PM +0800, Chao Yu wrote:
>> On 2016/6/3 13:08, Jaegeuk Kim wrote:
>>> On Tue, May 31, 2016 at 02:10:50PM +0800, Chao Yu wrote:
>>>> Hi Jaegeuk,
>>>>
>>>> On 2016/5/30 10:37, Jaegeuk Kim wrote:
>>>>> Hi Chao,
>>>>>
>>>>> On Sat, May 21, 2016 at 01:19:11PM +0800, Chao Yu wrote:
>>>>>> From: Chao Yu <yuchao0@xxxxxxxxxx>
>>>>>>
>>>>>> If we fail to move data page during foreground GC, we should give another
>>>>>> chance to writeback that page which was set dirty previously by writer.
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>
>>>>>> ---
>>>>>>  fs/f2fs/gc.c | 5 ++++-
>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>> index 38d56f6..ee213a8 100644
>>>>>> --- a/fs/f2fs/gc.c
>>>>>> +++ b/fs/f2fs/gc.c
>>>>>> @@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
>>>>>>  			.page = page,
>>>>>>  			.encrypted_page = NULL,
>>>>>>  		};
>>>>>> +		bool is_dirty = PageDirty(page);
>>>>>> +
>>>>>>  		set_page_dirty(page);
>>>>>>  		f2fs_wait_on_page_writeback(page, DATA, true);
>>>>>>  		if (clear_page_dirty_for_io(page))
>>>>>>  			inode_dec_dirty_pages(inode);
>>>>>>  		set_cold_data(page);
>>>>>> -		do_write_data_page(&fio);
>>>>>> +		if (do_write_data_page(&fio) && is_dirty)
>>>>>> +			set_page_dirty(page);
>>>>>
>>>>> If this page is truncated with -ENOENT, we don't need to set it dirty again.
>>>>
>>>> Agree
>>>>
>>>>> I expect that, if we get an error here, do_garbage_collect() would retry FG_GC
>>>>
>>>> IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying
>>>> do the movement just one time. Here, I think if there are just few of blocks are
>>>> failed to be moved, we can give one more time for retrying. How do you think?
>>>
>>> Mostly I expected here -ENOENT caused by race condition.
>>
>> If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to
>> worry about this case, right?
>>
>>> Do we have another expectation?
>>
>> ENOMEM or EIO?
> 
> EIO will stop everything.
> ENOMEM would be better to wait for a while from page reclaim?

Agree, but for ioctl path, IMO, we don't need to let user waiting for ENOMEM
case looping.

> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>> again.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>  		clear_cold_data(page);
>>>>>>  	}
>>>>>>  out:
>>>>>> -- 
>>>>>> 2.7.2
>>>>> .
>>>>>
>>> .
>>>
> .
>