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.
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)
.
.
--
Thanks,
Yunlong Song
------------------------------------------------------------------------------
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
