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

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

[Zhang Qilong via Linux-f2fs-devel]

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

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]);
-- 
2.25.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: Fix the race condition of resize flag between resizefs

[Zhang Qilong via Linux-f2fs-devel]

Because the set/clear SBI_IS_RESIZEFS flag not between any locks,
In the following case:
  thread1			thread2
   ->ioctl(resizefs)
    ->set RESIZEFS flag		 ->ioctl(resizefs)
    ...                   	  ->set RESIZEFS flag
    ->clear RESIZEFS flag
    				  ->resizefs stream
				    # No RESIZEFS flag in the stream

Also before freeze_super, the resizefs not started, we should not set
the SBI_IS_RESIZEFS flag.

So move the set/clear SBI_IS_RESIZEFS flag between the cp_mutex and
gc_lock.

Fixes: b4b10061ef98 ("f2fs: refactor resize_fs to avoid meta updates in progress")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
v2:
- modify commit message
---
 fs/f2fs/gc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4546e01b2ee0..2a9d825b84f7 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -2133,8 +2133,6 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
 	if (err)
 		return err;
 
-	set_sbi_flag(sbi, SBI_IS_RESIZEFS);
-
 	freeze_super(sbi->sb);
 	f2fs_down_write(&sbi->gc_lock);
 	f2fs_down_write(&sbi->cp_global_sem);
@@ -2150,6 +2148,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
 	if (err)
 		goto out_err;
 
+	set_sbi_flag(sbi, SBI_IS_RESIZEFS);
 	err = free_segment_range(sbi, secs, false);
 	if (err)
 		goto recover_out;
@@ -2173,6 +2172,7 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
 		f2fs_commit_super(sbi, false);
 	}
 recover_out:
+	clear_sbi_flag(sbi, SBI_IS_RESIZEFS);
 	if (err) {
 		set_sbi_flag(sbi, SBI_NEED_FSCK);
 		f2fs_err(sbi, "resize_fs failed, should run fsck to repair!");
@@ -2185,6 +2185,5 @@ int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count)
 	f2fs_up_write(&sbi->cp_global_sem);
 	f2fs_up_write(&sbi->gc_lock);
 	thaw_super(sbi->sb);
-	clear_sbi_flag(sbi, SBI_IS_RESIZEFS);
 	return err;
 }
-- 
2.25.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: Fix the race condition of resize flag between resizefs

[Chao Yu]

On 2022/10/18 10:45, Zhang Qilong wrote:
> Because the set/clear SBI_IS_RESIZEFS flag not between any locks,
> In the following case:
>    thread1			thread2
>     ->ioctl(resizefs)
>      ->set RESIZEFS flag		 ->ioctl(resizefs)
>      ...                   	  ->set RESIZEFS flag
>      ->clear RESIZEFS flag
>      				  ->resizefs stream
> 				    # No RESIZEFS flag in the stream
> 
> Also before freeze_super, the resizefs not started, we should not set
> the SBI_IS_RESIZEFS flag.
> 
> So move the set/clear SBI_IS_RESIZEFS flag between the cp_mutex and
> gc_lock.
> 
> Fixes: b4b10061ef98 ("f2fs: refactor resize_fs to avoid meta updates in progress")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu]

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?

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

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

[zhangqilong via Linux-f2fs-devel]

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

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

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

[Chao Yu]

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?

__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

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

[zhangqilong via Linux-f2fs-devel]

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

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

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

[Chao Yu]

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?

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

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

[zhangqilong via Linux-f2fs-devel]

> 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

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

[Chao Yu]

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

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,


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

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

[zhangqilong via Linux-f2fs-devel]

> 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()

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

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

[Chao Yu]

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