> -----Original Message----- > From: Chao Yu <[email protected]> > Sent: Wednesday, March 9, 2022 11:33 AM > To: 常凤楠 <[email protected]>; Fengnan Chang > <[email protected]>; [email protected] > Cc: [email protected] > 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 <[email protected]> > >> Sent: Monday, March 7, 2022 6:30 PM > >> To: 常凤楠 <[email protected]>; Fengnan Chang > >> <[email protected]>; [email protected] > >> Cc: [email protected] > >> 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 <[email protected]> > >>>> Sent: Monday, March 7, 2022 9:43 AM > >>>> To: Fengnan Chang <[email protected]>; [email protected] > >>>> Cc: [email protected]; 常凤楠 > >>>> <[email protected]> > >>>> 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 <[email protected]> > >>>>> --- > >>>>> 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 [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
