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

[Chao Yu <chao@kernel.org>]

On 2022/10/25 15:36, zhangqilong wrote:
>> 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-zhang
>>> qilong3@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 for your suggestion, I try it later for this point. If I have any progress,
> I will notify you immediately.
> 
> Do you have any suggestion for this patch? :)
>          f2fs: Fix data consistency in f2fs_move_file_range()

No objection from my side... :)

I think it needs to test/verify the case I mentioned before merging
this patch.

Thanks,

> 
> Thanks,
> 
>> 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