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)
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>> .
>>
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to