Re: [f2fs-dev] 答复: 答复: 答复: [PATCH] f2fs: Fix data consistency in f2fs_move_file_range()

[Chao Yu <chao@kernel.org>]

On 2022/10/25 15:01, zhangqilong wrote:
>> On 2022/10/25 14:27, zhangqilong wrote:
>>>> On 2022/10/20 15:27, zhangqilong via Linux-f2fs-devel wrote:
>>>>>> On 2022/10/18 10:45, Zhang Qilong wrote:
>>>>>>> In the following case:
>>>>>>> process 1			process 2
>>>>>>>      ->open A
>>>>>>>       ->mmap
>>>>>>>        ->read # the first time
>>>>>>>        				 ->ioctl w/h F2FS_IOC_MOVE_RANGE
>>>>>>> 				        	# (range A->B)
>>>>>>>        ->read # the second time
>>>>>>
>>>>>> How about checking B as well? Previous mapped data can still be
>>>>>> accessed after F2FS_IOC_MOVE_RANGE?
>>>>>>
>>>>>
>>>>> Hi
>>>>>
>>>>> I have checked B as well. Previous mapped data can't be accessed
>>>>> after F2FS_IOC_MOVE_RANGE.
>>>>
>>>> I doubt that we didn't call flush_dcache_page() in below branch, so
>>>> user may see stall data after F2FS_IOC_MOVE_RANGE? Am I missing
>> something?
>>>>
>>>
>>> Hi,
>>>
>>> You are right, it needs flush_dcache_page, but it is unnecessary here,
>>> the __clone_blkaddrs() is called by FALLOC_FL_COLLAPSE_RANGE/
>> FALLOC_FL_INSERT_RANGE /F2FS_IOC_MOVE_RANGE.
>>> ->__exchange_data_block()
>>>    ->__clone_blkaddrs()
>>>
>>> f2fs_do_collapse()  and f2fs_insert_range() have truncate_pagecache
>>> after __exchange_data_block() It seem we have analyzed before. So we
>> only need to add a truncate operation for F2FS_IOC_MOVE_RANGE.
>>
>> I mean it needs to call truncate_pagecache_range(dst, ...) in
>> f2fs_move_file_range() as well, right?
> 
> Yes, I think it should call truncate_pagecache_range(dst, ...) or flush_dcache_page() here.
> I submitted a patch before, it seems to be forgetten.
> 
> https://lore.kernel.org/linux-f2fs-devel/20220825024102.120651-1-zhangqilong3@huawei.com/
> 
> But, I test it w/o truncate_pagecache_range(dst, ...) or flush_dcache_page(), user can not
> see stall dst data, maybe It is a bit difficult to construct the scene for me.

Please check the condition how can we run into below else branch. I guess you
need to persist data blocks of src into a checkpoint w/ SYNC(2), then
__clone_blkaddrs() will copy data from page cache directly instead of exchanging
metadatas.

Thanks,


> 
> Thanks,
>>
>> Thanks,
>>
>>>
>>>> __clone_blkaddrs()
>>>> {
>>>> ...
>>>> 		} else {
>>>> 			struct page *psrc, *pdst;
>>>>
>>>> 			psrc = f2fs_get_lock_data_page(src_inode,
>>>> 							src + i, true);
>>>> 			if (IS_ERR(psrc))
>>>> 				return PTR_ERR(psrc);
>>>> 			pdst = f2fs_get_new_data_page(dst_inode, NULL,
>> dst + i,
>>>> 								true);
>>>> 			if (IS_ERR(pdst)) {
>>>> 				f2fs_put_page(psrc, 1);
>>>> 				return PTR_ERR(pdst);
>>>> 			}
>>>> 			memcpy_page(pdst, 0, psrc, 0, PAGE_SIZE);
>>>> 			set_page_dirty(pdst);
>>>> 			f2fs_put_page(pdst, 1);
>>>> 			f2fs_put_page(psrc, 1);
>>>>
>>>> 			ret = f2fs_truncate_hole(src_inode,
>>>> 						src + i, src + i + 1);
>>>> 			if (ret)
>>>> 				return ret;
>>>> 			i++;
>>>> 		}
>>>> ...
>>>> }
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> In addition, this patch could be applied to mainline if possible?
>>>>>
>>>>> Thanks
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>> We will read old data at the second time. The root cause is that
>>>>>>> user still can see the previous source data after being moved. We
>>>>>>> fix it by adding truncating after __exchange_data_block.
>>>>>>>
>>>>>>> Fixes: 4dd6f977fc77 ("f2fs: support an ioctl to move a range of
>>>>>>> data
>>>>>>> blocks")
>>>>>>> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - moving truncating to the range of f2fs_lock_op()
>>>>>>>
>>>>>>> v3:
>>>>>>> - modify the title and commit message
>>>>>>> ---
>>>>>>>      fs/f2fs/file.c | 3 +++
>>>>>>>      1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index
>>>>>>> 82cda1258227..e9dfa41baf9e 100644
>>>>>>> --- a/fs/f2fs/file.c
>>>>>>> +++ b/fs/f2fs/file.c
>>>>>>> @@ -2824,6 +2824,7 @@ static int f2fs_move_file_range(struct file
>>>>>>> *file_in,
>>>>>> loff_t pos_in,
>>>>>>>      			goto out_src;
>>>>>>>      	}
>>>>>>>
>>>>>>> +	filemap_invalidate_lock(src->i_mapping);
>>>>>>>      	f2fs_lock_op(sbi);
>>>>>>>      	ret = __exchange_data_block(src, dst, pos_in >>
>> F2FS_BLKSIZE_BITS,
>>>>>>>      				pos_out >> F2FS_BLKSIZE_BITS, @@
>> -2835,7
>>>> +2836,9 @@ static
>>>>>>> int f2fs_move_file_range(struct file *file_in,
>>>>>> loff_t pos_in,
>>>>>>>      		else if (dst_osize != dst->i_size)
>>>>>>>      			f2fs_i_size_write(dst, dst_osize);
>>>>>>>      	}
>>>>>>> +	truncate_pagecache_range(src, pos_in, pos_in + len - 1);
>>>>>>>      	f2fs_unlock_op(sbi);
>>>>>>> +	filemap_invalidate_unlock(src->i_mapping);
>>>>>>>
>>>>>>>      	if (src != dst)
>>>>>>>      		f2fs_up_write(&F2FS_I(dst)->i_gc_rwsem[WRITE]);
>>>>>
>>>>> _______________________________________________
>>>>> Linux-f2fs-devel mailing list
>>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel