[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <27e01c20-2fb6-4d51-b2c0-1c5e3f0d4f26@kernel.org>
Date: Mon, 2 Sep 2024 14:38:53 +0800
From: Chao Yu <chao@...nel.org>
To: Sunmin Jeong <s_min.jeong@...sung.com>, jaegeuk@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to
pollute atomic_file
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@...nel.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@...ts.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
Powered by blists - more mailing lists