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