Re: [f2fs-dev] [PATCH] f2fs: add fi->commit_lock to protect commit GCed pages

[Chao Yu]

On 2018/2/9 21:29, Yunlong Song wrote:
> Back to the problem, if we skip out, then the f2fs_gc will go
> into dead loop if the apps only atomic start but never atomic

That's another issue, which I have suggest to set a threshold time
to release atomic/volatile pages by balance_fs_bg.

Thanks,

> commit. The main aim of my two patches is to remove the skip
> action to avoid the dead loop.
> 
> On 2018/2/9 21:26, Chao Yu wrote:
>> On 2018/2/9 20:56, Yunlong Song wrote:
>>> As what I point in last mail, if the atomic file is not committed
>>> yet, gc_data_segment will register_inmem_page the GCed data pages.
>>
>> We will skip GCing that page as below check:
>>
>> - move_data_{page,block}
>> Â - f2fs_is_atomic_file()
>> ÂÂÂ skip out;
>>
>> No?
>>
>> Thanks,
>>
>>> This will cause these data pages written twice, the first write
>>> happens in move_data_page->do_write_data_page, and the second
>>> write happens in later __commit_inmem_pages->do_write_data_page.
>>>
>>> On 2018/2/9 20:44, Chao Yu wrote:
>>>> On 2018/2/8 11:11, Yunlong Song wrote:
>>>>> Then the GCed data pages are totally mixed with the inmem atomic pages,
>>>>
>>>> If we add dio_rwsem, GC flow is exclude with atomic write flow. There
>>>> will be not race case to mix GCed page into atomic pages.
>>>>
>>>> Or you mean:
>>>>
>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - gc_data_segment
>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - move_data_page
>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - f2fs_is_atomic_file
>>>> - f2fs_ioc_start_atomic_write
>>>> ÂÂ - set_inode_flag(inode, FI_ATOMIC_FILE);
>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - f2fs_set_data_page_dirty
>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - register_inmem_page
>>>>
>>>> In this case, GCed page can be mixed into database transaction, but could
>>>> it cause any problem except break rule of isolation for transaction.
>>>>
>>>>> this will cause the atomic commit ops write the GCed data pages twice
>>>>> (the first write happens in GC).
>>>>>
>>>>> How about using the early two patches to separate the inmem data pages
>>>>> and GCed data pages, and use dio_rwsem instead of this patch to fix the
>>>>> dnode page problem (dnode page commited but data page are not committed
>>>>> for the GCed page)?
>>>>
>>>> Could we fix the race case first, based on that fixing, and then find the
>>>> place that we can improve?
>>>>
>>>>>
>>>>>
>>>>> On 2018/2/7 20:16, Chao Yu wrote:
>>>>>> On 2018/2/6 11:49, Yunlong Song wrote:
>>>>>>> This patch adds fi->commit_lock to avoid the case that GCed node pages
>>>>>>> are committed but GCed data pages are not committed. This can avoid the
>>>>>>> db file run into inconsistent state when sudden-power-off happens if
>>>>>>> data pages of atomic file is allowed to be GCed before.
>>>>>>
>>>>>> do_fsync:ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GC:
>>>>>> - mutex_lock(&fi->commit_lock);
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - lock_page()
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - mutex_lock(&fi->commit_lock);
>>>>>> ÂÂÂ - lock_page()
>>>>>>
>>>>>>
>>>>>> Well, please consider lock dependency & code complexity, IMO, reuse
>>>>>> fi->dio_rwsem[WRITE] will be enough as below:
>>>>>>
>>>>>> ---
>>>>>> ÂÂÂ fs/f2fs/file.c | 3 +++
>>>>>> ÂÂÂ fs/f2fs/gc.cÂÂ | 5 -----
>>>>>> ÂÂÂ 2 files changed, 3 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>>> index 672a542e5464..1bdc11feb8d0 100644
>>>>>> --- a/fs/f2fs/file.c
>>>>>> +++ b/fs/f2fs/file.c
>>>>>> @@ -1711,6 +1711,8 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>>>>>>
>>>>>> ÂÂÂÂÂÂÂ inode_lock(inode);
>>>>>>
>>>>>> +ÂÂÂ down_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
>>>>>> +
>>>>>> ÂÂÂÂÂÂÂ if (f2fs_is_volatile_file(inode))
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ goto err_out;
>>>>>>
>>>>>> @@ -1729,6 +1731,7 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
>>>>>> ÂÂÂÂÂÂÂ }
>>>>>> ÂÂÂ err_out:
>>>>>> +ÂÂÂ up_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
>>>>>> ÂÂÂÂÂÂÂ inode_unlock(inode);
>>>>>> ÂÂÂÂÂÂÂ mnt_drop_write_file(filp);
>>>>>> ÂÂÂÂÂÂÂ return ret;
>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>> index b9d93fd532a9..e49416283563 100644
>>>>>> --- a/fs/f2fs/gc.c
>>>>>> +++ b/fs/f2fs/gc.c
>>>>>> @@ -622,9 +622,6 @@ static void move_data_block(struct inode *inode, block_t bidx,
>>>>>> ÂÂÂÂÂÂÂ if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ goto out;
>>>>>>
>>>>>> -ÂÂÂ if (f2fs_is_atomic_file(inode))
>>>>>> -ÂÂÂÂÂÂÂ goto out;
>>>>
>>>> Seems that we need this check.
>>>>
>>>>>> -
>>>>>> ÂÂÂÂÂÂÂ if (f2fs_is_pinned_file(inode)) {
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ f2fs_pin_file_control(inode, true);
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ goto out;
>>>>>> @@ -729,8 +726,6 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type,
>>>>>> ÂÂÂÂÂÂÂ if (!check_valid_map(F2FS_I_SB(inode), segno, off))
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ goto out;
>>>>>>
>>>>>> -ÂÂÂ if (f2fs_is_atomic_file(inode))
>>>>>> -ÂÂÂÂÂÂÂ goto out;
>>>>
>>>> Ditto.
>>>>
>>>> Thanks,
>>>>
>>>>>> ÂÂÂÂÂÂÂ if (f2fs_is_pinned_file(inode)) {
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂ if (gc_type == FG_GC)
>>>>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ f2fs_pin_file_control(inode, true);
>>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>