[f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[Fengnan Chang]

When compressed file has blocks, start atomic write will be failed, but
still set FI_ATOMIC_FILE flag, if write partial cluster and commit
atomic write will cause data corruption.
Fixes: 4c8ff7095bef (f2fs: support data compression)
Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite whole cluster)

Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
---
 fs/f2fs/data.c | 4 +---
 fs/f2fs/file.c | 3 ++-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 6b5f389ba998..5cbee4ed0982 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 		int ret;
 
 		*fsdata = NULL;
-
-		if (len == PAGE_SIZE)
+		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
 			goto repeat;
 
 		ret = f2fs_prepare_compress_overwrite(inode, pagep,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cfdc41f87f5d..26a648ef9e1f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 
 	inode_lock(inode);
 
-	f2fs_disable_compressed_file(inode);
+	if (!f2fs_disable_compressed_file(inode))
+		return -EINVAL;
 
 	if (f2fs_is_atomic_file(inode)) {
 		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))
-- 
2.25.1

git send-email --to jaegeuk@kernel.org --to chao@kernel.org --cc linux-f2fs-devel@lists.sourceforge.net --cc changfengnan@vivo.com --rfc 0001-f2fs-fix-compress-file-start-atomic-write-may-cause-.patch


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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[Chao Yu]

On 2022/3/5 12:00, Fengnan Chang wrote:
> When compressed file has blocks, start atomic write will be failed, but

You mean f2fs_ioc_start_atomic_write will succeed, but compressed flag
will be remained in inode?

> still set FI_ATOMIC_FILE flag, if write partial cluster and commit
> atomic write will cause data corruption.

Could you please explain more about why data will be corrupted?

Thanks,

> Fixes: 4c8ff7095bef (f2fs: support data compression)
> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite whole cluster)
> 
> Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
> ---
>   fs/f2fs/data.c | 4 +---
>   fs/f2fs/file.c | 3 ++-
>   2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 6b5f389ba998..5cbee4ed0982 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>   		int ret;
>   
>   		*fsdata = NULL;
> -
> -		if (len == PAGE_SIZE)
> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
>   			goto repeat;
>   
>   		ret = f2fs_prepare_compress_overwrite(inode, pagep,
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index cfdc41f87f5d..26a648ef9e1f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>   
>   	inode_lock(inode);
>   
> -	f2fs_disable_compressed_file(inode);
> +	if (!f2fs_disable_compressed_file(inode))
> +		return -EINVAL;
>   
>   	if (f2fs_is_atomic_file(inode)) {
>   		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))


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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[常凤楠 via Linux-f2fs-devel]

> -----Original Message-----
> From: Chao Yu <chao@kernel.org>
> Sent: Monday, March 7, 2022 9:43 AM
> To: Fengnan Chang <fengnanchang@gmail.com>; jaegeuk@kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net; 常凤楠
> <changfengnan@vivo.com>
> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write may cause
> data corruption
> 
> On 2022/3/5 12:00, Fengnan Chang wrote:
> > When compressed file has blocks, start atomic write will be failed,
> > but
> 
> You mean f2fs_ioc_start_atomic_write will succeed, but compressed flag will
> be remained in inode?

Yes.

> 
> > still set FI_ATOMIC_FILE flag, if write partial cluster and commit
> > atomic write will cause data corruption.
> 
> Could you please explain more about why data will be corrupted?

Step 1:
Create a compressed file ,write 64K data , call fsync(), then the blocks are write as compressed cluster.
Step2:
iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
write page 0 and page 3.
iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal file, 
Step3:
drop cache.
Read page 0-4   -- Since page 0 has a valid block address, read as no-compressed, page 1 and 2 will be compressed data or zero. 

And before f2fs: compress: remove unneeded read when rewrite whole cluster), even Step 2 is not right, but whole
cluster will mark dirty in write_begin, and whole cluster will be re-write as no-compressed cluster, so it's ok.

> 
> Thanks,
> 
> > Fixes: 4c8ff7095bef (f2fs: support data compression)
> > Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite
> > whole cluster)
> >
> > Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
> > ---
> >   fs/f2fs/data.c | 4 +---
> >   fs/f2fs/file.c | 3 ++-
> >   2 files changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> > 6b5f389ba998..5cbee4ed0982 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file, struct
> address_space *mapping,
> >   		int ret;
> >
> >   		*fsdata = NULL;
> > -
> > -		if (len == PAGE_SIZE)
> > +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
> >   			goto repeat;
> >
> >   		ret = f2fs_prepare_compress_overwrite(inode, pagep, diff --git
> > a/fs/f2fs/file.c b/fs/f2fs/file.c index cfdc41f87f5d..26a648ef9e1f
> > 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct
> > file *filp)
> >
> >   	inode_lock(inode);
> >
> > -	f2fs_disable_compressed_file(inode);
> > +	if (!f2fs_disable_compressed_file(inode))
> > +		return -EINVAL;
> >
> >   	if (f2fs_is_atomic_file(inode)) {
> >   		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))

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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[Chao Yu]

On 2022/3/7 15:54, 常凤楠 wrote:
> 
>> -----Original Message-----
>> From: Chao Yu <chao@kernel.org>
>> Sent: Monday, March 7, 2022 9:43 AM
>> To: Fengnan Chang <fengnanchang@gmail.com>; jaegeuk@kernel.org
>> Cc: linux-f2fs-devel@lists.sourceforge.net; 常凤楠
>> <changfengnan@vivo.com>
>> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write may cause
>> data corruption
>>
>> On 2022/3/5 12:00, Fengnan Chang wrote:
>>> When compressed file has blocks, start atomic write will be failed,
>>> but
>>
>> You mean f2fs_ioc_start_atomic_write will succeed, but compressed flag will
>> be remained in inode?
> 
> Yes.
> 
>>
>>> still set FI_ATOMIC_FILE flag, if write partial cluster and commit
>>> atomic write will cause data corruption.
>>
>> Could you please explain more about why data will be corrupted?
> 
> Step 1:
> Create a compressed file ,write 64K data , call fsync(), then the blocks are write as compressed cluster.
> Step2:
> iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
> write page 0 and page 3.
> iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal file,
> Step3:
> drop cache.
> Read page 0-4   -- Since page 0 has a valid block address, read as no-compressed, page 1 and 2 will be compressed data or zero.
> 
> And before f2fs: compress: remove unneeded read when rewrite whole cluster), even Step 2 is not right, but whole
> cluster will mark dirty in write_begin, and whole cluster will be re-write as no-compressed cluster, so it's ok.

Ah, after 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite whole cluster),
we expect that f2fs_write_cache_pages() will be called, and f2fs_prepare_compress_overwrite()
can read and set dirty left pages of compressed cluster.

However atomic commit flow won't call f2fs_write_cache_pages()...

Anyway, thanks for the explanation, and, could you please add above comments into commit
description of this patch?

Thanks,

> 
>>
>> Thanks,
>>
>>> Fixes: 4c8ff7095bef (f2fs: support data compression)
>>> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite
>>> whole cluster)
>>>
>>> Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
>>> ---
>>>    fs/f2fs/data.c | 4 +---
>>>    fs/f2fs/file.c | 3 ++-
>>>    2 files changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>> 6b5f389ba998..5cbee4ed0982 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file, struct
>> address_space *mapping,
>>>    		int ret;
>>>
>>>    		*fsdata = NULL;
>>> -
>>> -		if (len == PAGE_SIZE)
>>> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
>>>    			goto repeat;
>>>
>>>    		ret = f2fs_prepare_compress_overwrite(inode, pagep, diff --git
>>> a/fs/f2fs/file.c b/fs/f2fs/file.c index cfdc41f87f5d..26a648ef9e1f
>>> 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct
>>> file *filp)
>>>
>>>    	inode_lock(inode);
>>>
>>> -	f2fs_disable_compressed_file(inode);
>>> +	if (!f2fs_disable_compressed_file(inode))
>>> +		return -EINVAL;
>>>
>>>    	if (f2fs_is_atomic_file(inode)) {
>>>    		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))


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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[常凤楠 via Linux-f2fs-devel]

> -----Original Message-----
> From: Chao Yu <chao@kernel.org>
> Sent: Monday, March 7, 2022 6:30 PM
> To: 常凤楠 <changfengnan@vivo.com>; Fengnan Chang
> <fengnanchang@gmail.com>; jaegeuk@kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write may cause
> data corruption
> 
> On 2022/3/7 15:54, 常凤楠 wrote:
> >
> >> -----Original Message-----
> >> From: Chao Yu <chao@kernel.org>
> >> Sent: Monday, March 7, 2022 9:43 AM
> >> To: Fengnan Chang <fengnanchang@gmail.com>; jaegeuk@kernel.org
> >> Cc: linux-f2fs-devel@lists.sourceforge.net; 常凤楠
> >> <changfengnan@vivo.com>
> >> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write
> >> may cause data corruption
> >>
> >> On 2022/3/5 12:00, Fengnan Chang wrote:
> >>> When compressed file has blocks, start atomic write will be failed,
> >>> but
> >>
> >> You mean f2fs_ioc_start_atomic_write will succeed, but compressed
> >> flag will be remained in inode?
> >
> > Yes.
> >
> >>
> >>> still set FI_ATOMIC_FILE flag, if write partial cluster and commit
> >>> atomic write will cause data corruption.
> >>
> >> Could you please explain more about why data will be corrupted?
> >
> > Step 1:
> > Create a compressed file ,write 64K data , call fsync(), then the blocks are
> write as compressed cluster.
> > Step2:
> > iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
> > write page 0 and page 3.
> > iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal
> > file,
> > Step3:
> > drop cache.
> > Read page 0-4   -- Since page 0 has a valid block address, read as
> no-compressed, page 1 and 2 will be compressed data or zero.
> >
> > And before f2fs: compress: remove unneeded read when rewrite whole
> > cluster), even Step 2 is not right, but whole cluster will mark dirty in
> write_begin, and whole cluster will be re-write as no-compressed cluster, so
> it's ok.
> 
> Ah, after 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite
> whole cluster), we expect that f2fs_write_cache_pages() will be called, and
> f2fs_prepare_compress_overwrite() can read and set dirty left pages of
> compressed cluster.
> 
> However atomic commit flow won't call f2fs_write_cache_pages()...
> 
> Anyway, thanks for the explanation, and, could you please add above
> comments into commit description of this patch?

Yes, I'll add more comments in next version. 
And I want to know why atomic write need disable compressed file, I didn't see any special is incompatible with compression.

Thanks.
> 
> Thanks,
> 
> >
> >>
> >> Thanks,
> >>
> >>> Fixes: 4c8ff7095bef (f2fs: support data compression)
> >>> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when
> >>> rewrite whole cluster)
> >>>
> >>> Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
> >>> ---
> >>>    fs/f2fs/data.c | 4 +---
> >>>    fs/f2fs/file.c | 3 ++-
> >>>    2 files changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> >>> 6b5f389ba998..5cbee4ed0982 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file,
> >>> struct
> >> address_space *mapping,
> >>>    		int ret;
> >>>
> >>>    		*fsdata = NULL;
> >>> -
> >>> -		if (len == PAGE_SIZE)
> >>> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
> >>>    			goto repeat;
> >>>
> >>>    		ret = f2fs_prepare_compress_overwrite(inode, pagep, diff --git
> >>> a/fs/f2fs/file.c b/fs/f2fs/file.c index cfdc41f87f5d..26a648ef9e1f
> >>> 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct
> >>> file *filp)
> >>>
> >>>    	inode_lock(inode);
> >>>
> >>> -	f2fs_disable_compressed_file(inode);
> >>> +	if (!f2fs_disable_compressed_file(inode))
> >>> +		return -EINVAL;
> >>>
> >>>    	if (f2fs_is_atomic_file(inode)) {
> >>>    		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))

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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[Chao Yu]

On 2022/3/7 19:07, 常凤楠 wrote:
>> -----Original Message-----
>> From: Chao Yu <chao@kernel.org>
>> Sent: Monday, March 7, 2022 6:30 PM
>> To: 常凤楠 <changfengnan@vivo.com>; Fengnan Chang
>> <fengnanchang@gmail.com>; jaegeuk@kernel.org
>> Cc: linux-f2fs-devel@lists.sourceforge.net
>> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write may cause
>> data corruption
>>
>> On 2022/3/7 15:54, 常凤楠 wrote:
>>>
>>>> -----Original Message-----
>>>> From: Chao Yu <chao@kernel.org>
>>>> Sent: Monday, March 7, 2022 9:43 AM
>>>> To: Fengnan Chang <fengnanchang@gmail.com>; jaegeuk@kernel.org
>>>> Cc: linux-f2fs-devel@lists.sourceforge.net; 常凤楠
>>>> <changfengnan@vivo.com>
>>>> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write
>>>> may cause data corruption
>>>>
>>>> On 2022/3/5 12:00, Fengnan Chang wrote:
>>>>> When compressed file has blocks, start atomic write will be failed,
>>>>> but
>>>>
>>>> You mean f2fs_ioc_start_atomic_write will succeed, but compressed
>>>> flag will be remained in inode?
>>>
>>> Yes.
>>>
>>>>
>>>>> still set FI_ATOMIC_FILE flag, if write partial cluster and commit
>>>>> atomic write will cause data corruption.
>>>>
>>>> Could you please explain more about why data will be corrupted?
>>>
>>> Step 1:
>>> Create a compressed file ,write 64K data , call fsync(), then the blocks are
>> write as compressed cluster.
>>> Step2:
>>> iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
>>> write page 0 and page 3.
>>> iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as normal
>>> file,
>>> Step3:
>>> drop cache.
>>> Read page 0-4   -- Since page 0 has a valid block address, read as
>> no-compressed, page 1 and 2 will be compressed data or zero.
>>>
>>> And before f2fs: compress: remove unneeded read when rewrite whole
>>> cluster), even Step 2 is not right, but whole cluster will mark dirty in
>> write_begin, and whole cluster will be re-write as no-compressed cluster, so
>> it's ok.
>>
>> Ah, after 7eab7a696827 (f2fs: compress: remove unneeded read when rewrite
>> whole cluster), we expect that f2fs_write_cache_pages() will be called, and
>> f2fs_prepare_compress_overwrite() can read and set dirty left pages of
>> compressed cluster.
>>
>> However atomic commit flow won't call f2fs_write_cache_pages()...
>>
>> Anyway, thanks for the explanation, and, could you please add above
>> comments into commit description of this patch?
> 
> Yes, I'll add more comments in next version.
> And I want to know why atomic write need disable compressed file, I didn't see any special is incompatible with compression.

I guess there is no incompatibility issue, however, as most database file will
be updated frequently, write amplification due to compression cluster design
may hurt database update performance, any particular scenario of using compressed
db file?

Thanks,

> 
> Thanks.
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>> Fixes: 4c8ff7095bef (f2fs: support data compression)
>>>>> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when
>>>>> rewrite whole cluster)
>>>>>
>>>>> Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
>>>>> ---
>>>>>     fs/f2fs/data.c | 4 +---
>>>>>     fs/f2fs/file.c | 3 ++-
>>>>>     2 files changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>>>> 6b5f389ba998..5cbee4ed0982 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file *file,
>>>>> struct
>>>> address_space *mapping,
>>>>>     		int ret;
>>>>>
>>>>>     		*fsdata = NULL;
>>>>> -
>>>>> -		if (len == PAGE_SIZE)
>>>>> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
>>>>>     			goto repeat;
>>>>>
>>>>>     		ret = f2fs_prepare_compress_overwrite(inode, pagep, diff --git
>>>>> a/fs/f2fs/file.c b/fs/f2fs/file.c index cfdc41f87f5d..26a648ef9e1f
>>>>> 100644
>>>>> --- a/fs/f2fs/file.c
>>>>> +++ b/fs/f2fs/file.c
>>>>> @@ -2009,7 +2009,8 @@ static int f2fs_ioc_start_atomic_write(struct
>>>>> file *filp)
>>>>>
>>>>>     	inode_lock(inode);
>>>>>
>>>>> -	f2fs_disable_compressed_file(inode);
>>>>> +	if (!f2fs_disable_compressed_file(inode))
>>>>> +		return -EINVAL;
>>>>>
>>>>>     	if (f2fs_is_atomic_file(inode)) {
>>>>>     		if (is_inode_flag_set(inode, FI_ATOMIC_REVOKE_REQUEST))


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

Re: [f2fs-dev] [RFC PATCH] f2fs: fix compress file start atomic write may cause data corruption

[常凤楠 via Linux-f2fs-devel]

> -----Original Message-----
> From: Chao Yu <chao@kernel.org>
> Sent: Wednesday, March 9, 2022 11:33 AM
> To: 常凤楠 <changfengnan@vivo.com>; Fengnan Chang
> <fengnanchang@gmail.com>; jaegeuk@kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write may cause
> data corruption
> 
> On 2022/3/7 19:07, 常凤楠 wrote:
> >> -----Original Message-----
> >> From: Chao Yu <chao@kernel.org>
> >> Sent: Monday, March 7, 2022 6:30 PM
> >> To: 常凤楠 <changfengnan@vivo.com>; Fengnan Chang
> >> <fengnanchang@gmail.com>; jaegeuk@kernel.org
> >> Cc: linux-f2fs-devel@lists.sourceforge.net
> >> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write
> >> may cause data corruption
> >>
> >> On 2022/3/7 15:54, 常凤楠 wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Chao Yu <chao@kernel.org>
> >>>> Sent: Monday, March 7, 2022 9:43 AM
> >>>> To: Fengnan Chang <fengnanchang@gmail.com>; jaegeuk@kernel.org
> >>>> Cc: linux-f2fs-devel@lists.sourceforge.net; 常凤楠
> >>>> <changfengnan@vivo.com>
> >>>> Subject: Re: [RFC PATCH] f2fs: fix compress file start atomic write
> >>>> may cause data corruption
> >>>>
> >>>> On 2022/3/5 12:00, Fengnan Chang wrote:
> >>>>> When compressed file has blocks, start atomic write will be
> >>>>> failed, but
> >>>>
> >>>> You mean f2fs_ioc_start_atomic_write will succeed, but compressed
> >>>> flag will be remained in inode?
> >>>
> >>> Yes.
> >>>
> >>>>
> >>>>> still set FI_ATOMIC_FILE flag, if write partial cluster and commit
> >>>>> atomic write will cause data corruption.
> >>>>
> >>>> Could you please explain more about why data will be corrupted?
> >>>
> >>> Step 1:
> >>> Create a compressed file ,write 64K data , call fsync(), then the
> >>> blocks are
> >> write as compressed cluster.
> >>> Step2:
> >>> iotcl(F2FS_IOC_START_ATOMIC_WRITE)  --- this should be fail, but not.
> >>> write page 0 and page 3.
> >>> iotcl(F2FS_IOC_COMMIT_ATOMIC_WRITE)  -- page 0 and 3 write as
> normal
> >>> file,
> >>> Step3:
> >>> drop cache.
> >>> Read page 0-4   -- Since page 0 has a valid block address, read as
> >> no-compressed, page 1 and 2 will be compressed data or zero.
> >>>
> >>> And before f2fs: compress: remove unneeded read when rewrite whole
> >>> cluster), even Step 2 is not right, but whole cluster will mark
> >>> dirty in
> >> write_begin, and whole cluster will be re-write as no-compressed
> >> cluster, so it's ok.
> >>
> >> Ah, after 7eab7a696827 (f2fs: compress: remove unneeded read when
> >> rewrite whole cluster), we expect that f2fs_write_cache_pages() will
> >> be called, and
> >> f2fs_prepare_compress_overwrite() can read and set dirty left pages
> >> of compressed cluster.
> >>
> >> However atomic commit flow won't call f2fs_write_cache_pages()...
> >>
> >> Anyway, thanks for the explanation, and, could you please add above
> >> comments into commit description of this patch?
> >
> > Yes, I'll add more comments in next version.
> > And I want to know why atomic write need disable compressed file, I didn't
> see any special is incompatible with compression.
> 
> I guess there is no incompatibility issue, however, as most database file will be
> updated frequently, write amplification due to compression cluster design
> may hurt database update performance, any particular scenario of using
> compressed db file?
There is particular scenario of using compressed db file, just out of curiosity.
> 
> Thanks,
> 
> >
> > Thanks.
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>> Fixes: 4c8ff7095bef (f2fs: support data compression)
> >>>>> Fixes: 7eab7a696827 (f2fs: compress: remove unneeded read when
> >>>>> rewrite whole cluster)
> >>>>>
> >>>>> Signed-off-by: Fengnan Chang <fengnanchang@gmail.com>
> >>>>> ---
> >>>>>     fs/f2fs/data.c | 4 +---
> >>>>>     fs/f2fs/file.c | 3 ++-
> >>>>>     2 files changed, 3 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> >>>>> 6b5f389ba998..5cbee4ed0982 100644
> >>>>> --- a/fs/f2fs/data.c
> >>>>> +++ b/fs/f2fs/data.c
> >>>>> @@ -3358,8 +3358,7 @@ static int f2fs_write_begin(struct file
> >>>>> *file, struct
> >>>> address_space *mapping,
> >>>>>     		int ret;
> >>>>>
> >>>>>     		*fsdata = NULL;
> >>>>> -
> >>>>> -		if (len == PAGE_SIZE)
> >>>>> +		if (len == PAGE_SIZE && !(f2fs_is_atomic_file(inode)))
> >>>>>     			goto repeat;
> >>>>>
> >>>>>     		ret = f2fs_prepare_compress_overwrite(inode, pagep, diff
> >>>>> --git a/fs/f2fs/file.c b/fs/f2fs/file.c index
> >>>>> cfdc41f87f5d..26a648ef9e1f
> >>>>> 100644
> >>>>> --- a/fs/f2fs/file.c
> >>>>> +++ b/fs/f2fs/file.c
> >>>>> @@ -2009,7 +2009,8 @@ static int
> >>>>> f2fs_ioc_start_atomic_write(struct
> >>>>> file *filp)
> >>>>>
> >>>>>     	inode_lock(inode);
> >>>>>
> >>>>> -	f2fs_disable_compressed_file(inode);
> >>>>> +	if (!f2fs_disable_compressed_file(inode))
> >>>>> +		return -EINVAL;
> >>>>>
> >>>>>     	if (f2fs_is_atomic_file(inode)) {
> >>>>>     		if (is_inode_flag_set(inode,
> FI_ATOMIC_REVOKE_REQUEST))

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