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

[zhangqilong via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

> 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.

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