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

[Chao Yu]

On 2024/8/20 10:44, Sunmin Jeong wrote:
> 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?

Sunmin,

Yes, you're right.

Also, I've checked f2fs/003 of xfstests w/ commit b40a2b003709, it can fix the
regression issue as well, so let's drop it.

Thanks for pointing it out. :)

Thanks,

> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel