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

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

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?


Signed-off-by: Yunlong Song <>
  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 
        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)


Yunlong Song

Reply via email to