On 2018/2/5 14:40, Yunlong Song wrote:
> Is it necessary to make atomic commit fail? What's the problem of this
> patch (no lock at all and does not make atomic fail)? These two patches
> aims to provide ability to gc old blocks of opened atomic file, with no
> affection to original atomic commit and no mix with inmem pages.
>
> On 2018/2/5 14:29, Chao Yu wrote:
>> On 2018/2/5 10:53, Yunlong Song wrote:
>>> Is it necessary to add a lock here? What's the problem of this patch (no
>>> lock at all)? Anyway, the problem is expected to be fixed asap, since
>>> attackers can easily write an app with only atomic start and no atomic
>>> commit, which will cause f2fs run into loop gc if the disk layout is
>>> much fragmented, since f2fs_gc will select the same target victim all
>>> the time (e.g. one block of target victim belongs to the opened atomic
>>> file, and it will not be moved and do_garbage_collect will finally
>>> return 0, and that victim is selected again next time) and goto gc_more
>>> time and time again, which will block all the fs ops (all the fs ops
>>> will hang up in f2fs_balance_fs).
>>
>> Hmm.. w/ original commit log and implementation, I supposed that the patch
>> intended to fix to make atomic write be isolated from other IOs like GC
>> triggered writes...
>>
>> Alright, we have discuss the problem before in below link:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1571951.html
>>
>> I meant, for example:
>>
>> f2fs_ioc_start_atomic_write()
>> inode->atomic_open_time = get_mtime();
>>
>> f2fs_ioc_commit_atomic_write()
>> inode->atomic_open_time = 0;
>>
>> f2fs_balance_fs_bg()
>> for_each_atomic_open_file()
>> if (inode->atomic_open_time &&
>> inode->atomic_open_time > threshold) {
>> drop_inmem_pages();
>> f2fs_msg();
>> }
>>
>> threshold = 30s
>>
>> Any thoughts?
>>
>> Thanks,
>>
>>>
>>> On 2018/2/4 22:56, Chao Yu wrote:
>>>> On 2018/2/3 10:47, Yunlong Song wrote:
>>>>> If inode has already started to atomic commit, then set_page_dirty will
>>>>> not mix the gc pages with the inmem atomic pages, so the page can be
>>>>> gced safely.
>>>>
>>>> Let's avoid Ccing fs mailing list if the patch didn't change vfs common
>>>> codes.
>>>>
>>>> As you know, the problem here is mixed dnode block flushing w/o
>>>> writebacking
>>>> gced data block, result in making transaction unintegrated.
OK, details as I explained before:
atomic_commit GC
- file_write_and_wait_range
- move_data_block
- f2fs_submit_page_write
- f2fs_update_data_blkaddr
- set_page_dirty
- fsync_node_pages
1. atomic writes data page #1 & update node #1
2. GC data page #2 & update node #2
3. page #1 & node #1 & #2 can be committed into nand flash before page #2 be
committed.
After a sudden pow-cut, database transaction will be inconsistent. So I think
there will be better to exclude gc/atomic_write to each other, with a lock
instead of flag checking.
Thanks,
>>>>
>>>> So how about just using dio_rwsem[WRITE] during atomic committing to
>>>> exclude
>>>> GCing data block of atomic opened file?
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.s...@huawei.com>
>>>>> ---
>>>>> fs/f2fs/data.c | 5 ++---
>>>>> fs/f2fs/gc.c | 6 ++++--
>>>>> 2 files changed, 6 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 7435830..edafcb6 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -1580,14 +1580,13 @@ bool should_update_outplace(struct inode *inode,
>>>>> struct f2fs_io_info *fio)
>>>>> return true;
>>>>> if (S_ISDIR(inode->i_mode))
>>>>> return true;
>>>>> - if (f2fs_is_atomic_file(inode))
>>>>> - return true;
>>>>> if (fio) {
>>>>> if (is_cold_data(fio->page))
>>>>> return true;
>>>>> if (IS_ATOMIC_WRITTEN_PAGE(fio->page))
>>>>> return true;
>>>>> - }
>>>>> + } else if (f2fs_is_atomic_file(inode))
>>>>> + return true;
>>>>> return false;
>>>>> }
>>>>>
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index b9d93fd..84ab3ff 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -622,7 +622,8 @@ 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))
>>>>> + if (f2fs_is_atomic_file(inode) &&
>>>>> + !f2fs_is_commit_atomic_write(inode))
>>>>> goto out;
>>>>>
>>>>> if (f2fs_is_pinned_file(inode)) {
>>>>> @@ -729,7 +730,8 @@ 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))
>>>>> + if (f2fs_is_atomic_file(inode) &&
>>>>> + !f2fs_is_commit_atomic_write(inode))
>>>>> goto out;
>>>>> if (f2fs_is_pinned_file(inode)) {
>>>>> if (gc_type == FG_GC)
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>> .
>>
>
