Re: [PATCH 2/2] f2fs: fix to avoid data update racing between GC and DIO

[Chao Yu]

Hi Jaegeuk,

On 2016/7/1 8:03, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Thu, Jun 30, 2016 at 04:42:48PM +0800, Chao Yu wrote:
>> Datas in file can be operated by GC and DIO simultaneously, so we will
>> face race case as below:
>>
>> For write case:
>> Thread A				Thread B
>> - generic_file_direct_write
>>  - invalidate_inode_pages2_range
>>  - f2fs_direct_IO
>>   - do_blockdev_direct_IO
>>    - do_direct_IO
>>     - get_more_blocks
>> 					- f2fs_gc
>> 					 - do_garbage_collect
>> 					  - gc_data_segment
>> 					   - move_data_page
>> 					    - do_write_data_page
>> 					    migrate data block to new block address
>>    - dio_bio_submit
>>    update user data to old block address
>>
>> For read case:
>> Thread A                                Thread B
>> - generic_file_direct_write
>>  - invalidate_inode_pages2_range
>>  - f2fs_direct_IO
>>   - do_blockdev_direct_IO
>>    - do_direct_IO
>>     - get_more_blocks
>> 					- f2fs_balance_fs
>> 					 - f2fs_gc
>> 					  - do_garbage_collect
>> 					   - gc_data_segment
>> 					    - move_data_page
>> 					     - do_write_data_page
>> 					     migrate data block to new block address
>> 					  - write_checkpoint
>> 					   - do_checkpoint
>> 					    - clear_prefree_segments
>> 					     - f2fs_issue_discard
>>                                              discard old block adress
>>    - dio_bio_submit
>>    update user buffer from obsolete block address
>>
>> In order to fix this, for one file, we should let DIO and GC getting exclusion
>> against with each other.
>>
>> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>
>> ---
>>  fs/f2fs/data.c  |  2 ++
>>  fs/f2fs/f2fs.h  |  1 +
>>  fs/f2fs/gc.c    | 14 +++++++++++++-
>>  fs/f2fs/super.c |  1 +
>>  4 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index ba4963f..08dc060 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1716,7 +1716,9 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>>  
>>  	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
>>  
>> +	mutex_lock(&F2FS_I(inode)->dio_mutex);
>>  	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
>> +	mutex_unlock(&F2FS_I(inode)->dio_mutex);
> 
> This means we need to sacrifice entire parallism even in the normal cases?
> Can we find another way?

1. For dio write vs dio write, writer will grab i_mutex before dio_mutex, so
anyway, concurrent dio writes will be exclusive.

2. For dio write vs gc, keep using dio_mutex for making them exclusive.

3. For dio read vs dio read, and dio read vs gc, what about adding dio_rwsem to
control the parallelism?

4. For dio write vs dio read, we grab different lock (write grabs dio_mutex,
read grabs dio_rwsem), so there is no race condition.

Thanks,

> 
> Thanks,
> 
>>  	if (iov_iter_rw(iter) == WRITE) {
>>  		if (err > 0)
>>  			set_inode_flag(inode, FI_UPDATE_WRITE);
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index bd82b6d..a241576 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -474,6 +474,7 @@ struct f2fs_inode_info {
>>  	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
>>  	struct mutex inmem_lock;	/* lock for inmemory pages */
>>  	struct extent_tree *extent_tree;	/* cached extent_tree entry */
>> +	struct mutex dio_mutex;		/* avoid racing between dio and gc */
>>  };
>>  
>>  static inline void get_extent_info(struct extent_info *ext,
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index c2c4ac3..98e3763 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -744,12 +744,24 @@ next_step:
>>  		/* phase 3 */
>>  		inode = find_gc_inode(gc_list, dni.ino);
>>  		if (inode) {
>> +			bool locked = false;
>> +
>> +			if (S_ISREG(inode->i_mode)) {
>> +				if (!mutex_trylock(&F2FS_I(inode)->dio_mutex))
>> +					continue;
>> +				locked = true;
>> +			}
>> +
>>  			start_bidx = start_bidx_of_node(nofs, inode)
>>  								+ ofs_in_node;
>> -			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
>> +			if (f2fs_encrypted_inode(inode) &&
>> +							S_ISREG(inode->i_mode))
>>  				move_encrypted_block(inode, start_bidx);
>>  			else
>>  				move_data_page(inode, start_bidx, gc_type);
>> +			if (locked)
>> +				mutex_unlock(&F2FS_I(inode)->dio_mutex);
>> +
>>  			stat_inc_data_blk_count(sbi, 1, gc_type);
>>  		}
>>  	}
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 8c698e1..24aab3f 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -575,6 +575,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>>  	INIT_LIST_HEAD(&fi->gdirty_list);
>>  	INIT_LIST_HEAD(&fi->inmem_pages);
>>  	mutex_init(&fi->inmem_lock);
>> +	mutex_init(&fi->dio_mutex);
>>  
>>  	/* Will be used by directory only */
>>  	fi->i_dir_level = F2FS_SB(sb)->dir_level;
>> -- 
>> 2.8.2.311.gee88674
> 
> .
>