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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2905d4bf-279c-45f0-a434-b9d47766bbd1@kernel.org>
Date: Mon, 2 Sep 2024 14:40:32 +0800
From: Chao Yu <chao@...nel.org>
To: Jaegeuk Kim <jaegeuk@...nel.org>
Cc: linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] f2fs: atomic: fix to not allow GC to pollute
 atomic_file

On 2024/8/28 23:27, Jaegeuk Kim wrote:
> On 08/14, Chao Yu wrote:
>> 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
>> 				   - 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);
> 
> Performance impact?

Please drop this patch due to commit b40a2b003709 (f2fs: use meta inode for
GC of atomic file) has fixed this issue. Thanks.

Thanks,

> 
>>   
>> +	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