Re: [PATCH 1/2] f2fs: enable to gc page whose inode already atomic commit

[Chao Yu <yuchao0@huawei.com>]

On 2018/2/5 14:40, Yunlong Song wrote:
> Is it necessary to make atomic commit fail? What's the problem of this
> patch (no lock at all and does not make atomic fail)? These two patches
> aims to provide ability to gc old blocks of opened atomic file, with no
> affection to original atomic commit and no mix with inmem pages.
> 
> On 2018/2/5 14:29, Chao Yu wrote:
>> On 2018/2/5 10:53, Yunlong Song wrote:
>>> 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).
>>
>> Hmm.. w/ original commit log and implementation, I supposed that the patch
>> intended to fix to make atomic write be isolated from other IOs like GC
>> triggered writes...
>>
>> Alright, we have discuss the problem before in below link:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1571951.html
>>
>> I meant, for example:
>>
>> f2fs_ioc_start_atomic_write()
>> inode->atomic_open_time = get_mtime();
>>
>> f2fs_ioc_commit_atomic_write()
>> inode->atomic_open_time = 0;
>>
>> f2fs_balance_fs_bg()
>> for_each_atomic_open_file()
>> 	if (inode->atomic_open_time &&
>> 			inode->atomic_open_time > threshold) {
>> 		drop_inmem_pages();
>> 		f2fs_msg();
>> 	}
>>
>> threshold = 30s
>>
>> Any thoughts?
>>
>> Thanks,
>>
>>>
>>> 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
>>>> codes.
>>>>
>>>> As you know, the problem here is mixed dnode block flushing w/o writebacking
>>>> gced data block, result in making transaction unintegrated.

OK, details as I explained before:

atomic_commit				GC
- file_write_and_wait_range
					- move_data_block
					 - f2fs_submit_page_write
					  - f2fs_update_data_blkaddr
					   - set_page_dirty
 - fsync_node_pages

1. atomic writes data page #1 & update node #1
2. GC data page #2 & update node #2
3. page #1 & node #1 & #2 can be committed into nand flash before page #2 be
committed.

After a sudden pow-cut, database transaction will be inconsistent. So I think
there will be better to exclude gc/atomic_write to each other, with a lock
instead of flag checking.

Thanks,

>>>>
>>>> So how about just using dio_rwsem[WRITE] during atomic committing to exclude
>>>> GCing data block of atomic opened file?
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>> ---
>>>>>    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 bidx,
>>>>>    	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)
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>> .
>>
>