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