lkml.org 
[lkml]   [2018]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 1/2] f2fs: enable to gc page whose inode already atomic commit
From
Date
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.
>>
>> 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.song@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)
>>>
>>
>> .
>>
>

\
 
 \ /
  Last update: 2018-02-05 07:30    [W:0.059 / U:1.800 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site