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