Re: [PATCH] f2fs: add fi->commit_lock to protect commit GCed pages

[Chao Yu]

On 2018/2/8 11:11, Yunlong Song wrote:
> Then the GCed data pages are totally mixed with the inmem atomic pages,

If we add dio_rwsem, GC flow is exclude with atomic write flow. There
will be not race case to mix GCed page into atomic pages.

Or you mean:

					- gc_data_segment
					 - move_data_page
					  - f2fs_is_atomic_file
- f2fs_ioc_start_atomic_write
 - set_inode_flag(inode, FI_ATOMIC_FILE);
					  - f2fs_set_data_page_dirty
					   - register_inmem_page

In this case, GCed page can be mixed into database transaction, but could
it cause any problem except break rule of isolation for transaction.

> this will cause the atomic commit ops write the GCed data pages twice
> (the first write happens in GC).
> 
> How about using the early two patches to separate the inmem data pages
> and GCed data pages, and use dio_rwsem instead of this patch to fix the
> dnode page problem (dnode page commited but data page are not committed
> for the GCed page)?

Could we fix the race case first, based on that fixing, and then find the
place that we can improve?

> 
> 
> On 2018/2/7 20:16, Chao Yu wrote:
>> On 2018/2/6 11:49, Yunlong Song wrote:
>>> This patch adds fi->commit_lock to avoid the case that GCed node pages
>>> are committed but GCed data pages are not committed. This can avoid the
>>> db file run into inconsistent state when sudden-power-off happens if
>>> data pages of atomic file is allowed to be GCed before.
>>
>> do_fsync:ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GC:
>> - mutex_lock(&fi->commit_lock);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - lock_page()
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ - mutex_lock(&fi->commit_lock);
>> Â - lock_page()
>>
>>
>> Well, please consider lock dependency & code complexity, IMO, reuse
>> fi->dio_rwsem[WRITE] will be enough as below:
>>
>> ---
>> Â fs/f2fs/file.c | 3 +++
>> Â fs/f2fs/gc.cÂÂ | 5 -----
>> Â 2 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 672a542e5464..1bdc11feb8d0 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1711,6 +1711,8 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>>
>> ÂÂÂÂÂ inode_lock(inode);
>>
>> +ÂÂÂ down_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
>> +
>> ÂÂÂÂÂ if (f2fs_is_volatile_file(inode))
>> ÂÂÂÂÂÂÂÂÂ goto err_out;
>>
>> @@ -1729,6 +1731,7 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>> ÂÂÂÂÂÂÂÂÂ ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
>> ÂÂÂÂÂ }
>> Â err_out:
>> +ÂÂÂ up_write(&F2FS_I(inode)->dio_rwsem[WRITE]);
>> ÂÂÂÂÂ inode_unlock(inode);
>> ÂÂÂÂÂ mnt_drop_write_file(filp);
>> ÂÂÂÂÂ return ret;
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index b9d93fd532a9..e49416283563 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -622,9 +622,6 @@ 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))
>> -ÂÂÂÂÂÂÂ goto out;

Seems that we need this check.

>> -
>> ÂÂÂÂÂ if (f2fs_is_pinned_file(inode)) {
>> ÂÂÂÂÂÂÂÂÂ f2fs_pin_file_control(inode, true);
>> ÂÂÂÂÂÂÂÂÂ goto out;
>> @@ -729,8 +726,6 @@ 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))
>> -ÂÂÂÂÂÂÂ goto out;

Ditto.

Thanks,

>> ÂÂÂÂÂ if (f2fs_is_pinned_file(inode)) {
>> ÂÂÂÂÂÂÂÂÂ if (gc_type == FG_GC)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ f2fs_pin_file_control(inode, true);
>>
>