Re: [f2fs-dev] 答复: 答复: 答复: 答复: [PATCH] f2fs: Fix data consistency in f2fs_move_file_range()
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
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()
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
---
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);
+
[f2fs-dev] 答复: 答复: 答复: 答复: [PATCH] f2fs: Fix data consistency in f2fs_move_file_range()
> 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
> >>> ---
> >>> 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;
> >>> }
>
