> 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
> > qilo...@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()
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 <zhangqilo...@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
