Re: [f2fs-dev] [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute atomic_file

["Sunmin Jeong" <s_min.jeong@samsung.com>]

Hello Chao Yu,

>SQLite App			GC Thread	Shrinker
>- f2fs_ioc_start_atomic_write
>
>- f2fs_ioc_commit_atomic_write
> - f2fs_commit_atomic_write
>  - filemap_write_and_wait_range
>  : write atomic_file's data to cow_inode
>						echo 3 > drop_caches
>				- f2fs_gc
>				 - gc_data_segment
>				  - move_data_page

We modified the code to make GC of the atomic file go to move_data_block
in commit b40a2b003709 (f2fs: use meta inode for GC of atomic file).
Could you please check if this patch is still necessary?

Thanks,

>				   - set_page_dirty
>				   : it may load data of previous
>				     transaction into pagecache.
>  - f2fs_down_write(&fi->i_gc_rwsem[WRITE])
>  - __f2fs_commit_atomic_write
>  - f2fs_up_write(&fi->i_gc_rwsem[WRITE])
>
>During committing atomic_file, GC may be triggered to migrate atomic_file's
>block, so it may contain data of previous transaction in page cache, we
>should drop atomic_file's cache once it was migrated by GC.
>
>And also, we should writeback atomic_file and cow_file's data w/ i_gc_rwsem
>lock held, in order to avoid block address change during
>__f2fs_commit_atomic_write().
>
>Meahwhile, this patch adds f2fs_wait_on_block_writeback_range()
>to wait completion of block migration.
>
>Fixes: 3db1de0e582c ("f2fs: change the current atomic write way")
>Signed-off-by: Chao Yu <chao@kernel.org>
>---
>v2:
>- fix error path handling.
> fs/f2fs/segment.c | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
>diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
>3aee71c9f3c6..a43054ab0cf1 100644
>--- a/fs/f2fs/segment.c
>+++ b/fs/f2fs/segment.c
>@@ -236,6 +236,9 @@ static int __replace_atomic_write_block(struct inode
>*inode, pgoff_t index,
> 		return err;
> 	}
>
>+	if (__is_valid_data_blkaddr(dn.data_blkaddr))
>+		f2fs_wait_on_block_writeback_range(inode, dn.data_blkaddr,
1);
>+
> 	if (recover) {
> 		/* dn.data_blkaddr is always valid */
> 		if (!__is_valid_data_blkaddr(new_addr)) { @@ -339,6 +342,9
@@
>static int __f2fs_commit_atomic_write(struct inode *inode)
> 				goto out;
> 			}
>
>+			f2fs_wait_on_block_writeback_range(cow_inode,
>+								blkaddr, 1);
>+
> 			new = f2fs_kmem_cache_alloc(revoke_entry_slab,
GFP_NOFS,
> 							true, NULL);
>
>@@ -379,16 +385,29 @@ int f2fs_commit_atomic_write(struct inode *inode)
> 	struct f2fs_inode_info *fi = F2FS_I(inode);
> 	int err;
>
>+	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>+
> 	err = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
> 	if (err)
>-		return err;
>+		goto out;
>
>-	f2fs_down_write(&fi->i_gc_rwsem[WRITE]);
>-	f2fs_lock_op(sbi);
>+	/* writeback GCing page of cow_inode */
>+	err = filemap_write_and_wait_range(fi->cow_inode->i_mapping,
>+							0, LLONG_MAX);
>+	if (err)
>+		goto out;
>
>-	err = __f2fs_commit_atomic_write(inode);
>+	filemap_invalidate_lock(inode->i_mapping);
>+
>+	/* don't allow clean page loaded by GC to pollute atomic_file */
>+	truncate_pagecache(inode, 0);
>
>+	f2fs_lock_op(sbi);
>+	err = __f2fs_commit_atomic_write(inode);
> 	f2fs_unlock_op(sbi);
>+
>+	filemap_invalidate_unlock(inode->i_mapping);
>+out:
> 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>
> 	return err;
>--
>2.40.1
>
>
>
>_______________________________________________
>Linux-f2fs-devel mailing list
>Linux-f2fs-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel