From: Chao Yu <chao@kernel.org> To: Daeho Jeong <daeho43@gmail.com> Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com, Daeho Jeong <daehojeong@google.com>, syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com Subject: Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts Date: Tue, 31 Jan 2023 19:37:49 +0800 [thread overview] Message-ID: <8f1c15a3-d056-7709-af45-fe7cba56463f@kernel.org> (raw) In-Reply-To: <CACOAw_zhVgS84gOXpfZuvptMgsZDhP3QX2EFm=5CoKibB+3V1A@mail.gmail.com> Hi Daeho, On 2023/1/31 0:34, Daeho Jeong wrote: > Hi Chao, > > I read your patch series now and I like it. Thank you for checking the patches. :) > However, how about a race condition between start_atomic_write and > abort_atomic_write? Yup, I noticed that issue, I guess we can avoid this race condition by covering these two flows w/ i_atomic_sem. > abort_atomic_write is called without inode_lock in closing filp scenarios. > What do you think about this? I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's page cache if atomic_write is committed or aborted to avoid caching obsolete page? Thanks, > > Thanks, > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: >> >> Hi Daeho, Jaegeuk, >> >> Please take a look at patchset in below link: >> >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t >> >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me >> know your preference. :) >> >> One comment as below. >> >> On 2023/1/13 8:49, Daeho Jeong wrote: >>> From: Daeho Jeong <daehojeong@google.com> >>> >>> To fix a race condition between atomic write aborts, I use the inode >>> lock and make COW inode to be re-usable thoroughout the whole >>> atomic file inode lifetime. >>> >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>> --- >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- >>> fs/f2fs/inode.c | 11 +++++++++-- >>> fs/f2fs/segment.c | 3 --- >>> fs/f2fs/super.c | 2 -- >>> 4 files changed, 37 insertions(+), 22 deletions(-) >>> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index ecbc8c135b49..ff072a9ed258 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) >>> atomic_read(&inode->i_writecount) != 1) >>> return 0; >>> >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + >>> return 0; >>> } >>> >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) >>> * until all the writers close its file. Since this should be done >>> * before dropping file lock, it needs to do in ->flush. >>> */ >>> - if (F2FS_I(inode)->atomic_write_task == current) >>> + if (F2FS_I(inode)->atomic_write_task == current) { >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + } >>> return 0; >>> } >>> >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> goto out; >>> } >>> >>> - /* Create a COW inode for atomic write */ >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> - if (IS_ERR(pinode)) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - ret = PTR_ERR(pinode); >>> - goto out; >>> - } >>> + /* Check if the inode already has a COW inode */ >>> + if (fi->cow_inode == NULL) { >>> + /* Create a COW inode for atomic write */ >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> + if (IS_ERR(pinode)) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + ret = PTR_ERR(pinode); >>> + goto out; >>> + } >>> >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> - iput(pinode); >>> - if (ret) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - goto out; >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> + iput(pinode); >>> + if (ret) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + goto out; >>> + } >>> + >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> + } else { >>> + /* Reuse the already created COW inode */ >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); >>> } >>> >>> f2fs_write_inode(inode, NULL); >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> stat_inc_atomic_inode(inode); >>> >>> set_inode_flag(inode, FI_ATOMIC_FILE); >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> >>> isize = i_size_read(inode); >>> fi->original_i_size = isize; >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>> index ff6cf66ed46b..4921f7209e28 100644 >>> --- a/fs/f2fs/inode.c >>> +++ b/fs/f2fs/inode.c >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >>> void f2fs_evict_inode(struct inode *inode) >>> { >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; >>> + struct f2fs_inode_info *fi = F2FS_I(inode); >>> + nid_t xnid = fi->i_xattr_nid; >>> int err = 0; >>> >>> f2fs_abort_atomic_write(inode, true); >>> >>> + if (fi->cow_inode) { >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + iput(fi->cow_inode); >>> + fi->cow_inode = NULL; >>> + } >>> + >>> trace_f2fs_evict_inode(inode); >>> truncate_inode_pages_final(&inode->i_data); >>> >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) >>> stat_dec_inline_inode(inode); >>> stat_dec_compr_inode(inode); >>> stat_sub_compr_blocks(inode, >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); >>> + atomic_read(&fi->i_compr_blocks)); >>> >>> if (likely(!f2fs_cp_error(sbi) && >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index ae3c4e5474ef..536d7c674b04 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) >>> if (!f2fs_is_atomic_file(inode)) >>> return; >>> >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - iput(fi->cow_inode); >>> - fi->cow_inode = NULL; >>> release_atomic_write_cnt(inode); >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 1f812b9ce985..10463f084d30 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) >>> atomic_inc(&inode->i_count); >>> spin_unlock(&inode->i_lock); >>> >>> - f2fs_abort_atomic_write(inode, true); >> >> In order to avoid caching obsolete page of cow_inode, how about truncating >> them here? >> >> if (f2fs_is_atomic_file() && cow_inode) >> truncate_inode_pages_final(&cow_inode->i_data); >> >> Thanks, >> >>> - >>> /* should remain fi->extent_tree for writepage */ >>> f2fs_destroy_extent_node(inode); >>>
WARNING: multiple messages have this Message-ID (diff)
From: Chao Yu <chao@kernel.org> To: Daeho Jeong <daeho43@gmail.com> Cc: Daeho Jeong <daehojeong@google.com>, kernel-team@android.com, linux-kernel@vger.kernel.org, syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts Date: Tue, 31 Jan 2023 19:37:49 +0800 [thread overview] Message-ID: <8f1c15a3-d056-7709-af45-fe7cba56463f@kernel.org> (raw) In-Reply-To: <CACOAw_zhVgS84gOXpfZuvptMgsZDhP3QX2EFm=5CoKibB+3V1A@mail.gmail.com> Hi Daeho, On 2023/1/31 0:34, Daeho Jeong wrote: > Hi Chao, > > I read your patch series now and I like it. Thank you for checking the patches. :) > However, how about a race condition between start_atomic_write and > abort_atomic_write? Yup, I noticed that issue, I guess we can avoid this race condition by covering these two flows w/ i_atomic_sem. > abort_atomic_write is called without inode_lock in closing filp scenarios. > What do you think about this? I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's page cache if atomic_write is committed or aborted to avoid caching obsolete page? Thanks, > > Thanks, > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: >> >> Hi Daeho, Jaegeuk, >> >> Please take a look at patchset in below link: >> >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t >> >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me >> know your preference. :) >> >> One comment as below. >> >> On 2023/1/13 8:49, Daeho Jeong wrote: >>> From: Daeho Jeong <daehojeong@google.com> >>> >>> To fix a race condition between atomic write aborts, I use the inode >>> lock and make COW inode to be re-usable thoroughout the whole >>> atomic file inode lifetime. >>> >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>> --- >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- >>> fs/f2fs/inode.c | 11 +++++++++-- >>> fs/f2fs/segment.c | 3 --- >>> fs/f2fs/super.c | 2 -- >>> 4 files changed, 37 insertions(+), 22 deletions(-) >>> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index ecbc8c135b49..ff072a9ed258 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) >>> atomic_read(&inode->i_writecount) != 1) >>> return 0; >>> >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + >>> return 0; >>> } >>> >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) >>> * until all the writers close its file. Since this should be done >>> * before dropping file lock, it needs to do in ->flush. >>> */ >>> - if (F2FS_I(inode)->atomic_write_task == current) >>> + if (F2FS_I(inode)->atomic_write_task == current) { >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + } >>> return 0; >>> } >>> >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> goto out; >>> } >>> >>> - /* Create a COW inode for atomic write */ >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> - if (IS_ERR(pinode)) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - ret = PTR_ERR(pinode); >>> - goto out; >>> - } >>> + /* Check if the inode already has a COW inode */ >>> + if (fi->cow_inode == NULL) { >>> + /* Create a COW inode for atomic write */ >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> + if (IS_ERR(pinode)) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + ret = PTR_ERR(pinode); >>> + goto out; >>> + } >>> >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> - iput(pinode); >>> - if (ret) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - goto out; >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> + iput(pinode); >>> + if (ret) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + goto out; >>> + } >>> + >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> + } else { >>> + /* Reuse the already created COW inode */ >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); >>> } >>> >>> f2fs_write_inode(inode, NULL); >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> stat_inc_atomic_inode(inode); >>> >>> set_inode_flag(inode, FI_ATOMIC_FILE); >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> >>> isize = i_size_read(inode); >>> fi->original_i_size = isize; >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>> index ff6cf66ed46b..4921f7209e28 100644 >>> --- a/fs/f2fs/inode.c >>> +++ b/fs/f2fs/inode.c >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >>> void f2fs_evict_inode(struct inode *inode) >>> { >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; >>> + struct f2fs_inode_info *fi = F2FS_I(inode); >>> + nid_t xnid = fi->i_xattr_nid; >>> int err = 0; >>> >>> f2fs_abort_atomic_write(inode, true); >>> >>> + if (fi->cow_inode) { >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + iput(fi->cow_inode); >>> + fi->cow_inode = NULL; >>> + } >>> + >>> trace_f2fs_evict_inode(inode); >>> truncate_inode_pages_final(&inode->i_data); >>> >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) >>> stat_dec_inline_inode(inode); >>> stat_dec_compr_inode(inode); >>> stat_sub_compr_blocks(inode, >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); >>> + atomic_read(&fi->i_compr_blocks)); >>> >>> if (likely(!f2fs_cp_error(sbi) && >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index ae3c4e5474ef..536d7c674b04 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) >>> if (!f2fs_is_atomic_file(inode)) >>> return; >>> >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - iput(fi->cow_inode); >>> - fi->cow_inode = NULL; >>> release_atomic_write_cnt(inode); >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 1f812b9ce985..10463f084d30 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) >>> atomic_inc(&inode->i_count); >>> spin_unlock(&inode->i_lock); >>> >>> - f2fs_abort_atomic_write(inode, true); >> >> In order to avoid caching obsolete page of cow_inode, how about truncating >> them here? >> >> if (f2fs_is_atomic_file() && cow_inode) >> truncate_inode_pages_final(&cow_inode->i_data); >> >> Thanks, >> >>> - >>> /* should remain fi->extent_tree for writepage */ >>> f2fs_destroy_extent_node(inode); >>> _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2023-01-31 11:38 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-13 0:49 [f2fs-dev] [PATCH] f2fs: synchronize atomic write aborts Daeho Jeong 2023-01-13 0:49 ` Daeho Jeong 2023-01-28 2:07 ` [f2fs-dev] " Chao Yu 2023-01-28 2:07 ` Chao Yu 2023-01-30 16:34 ` Daeho Jeong 2023-01-30 16:34 ` Daeho Jeong 2023-01-31 11:37 ` Chao Yu [this message] 2023-01-31 11:37 ` Chao Yu 2023-01-31 19:13 ` Daeho Jeong 2023-01-31 19:13 ` Daeho Jeong 2023-01-31 21:38 ` Daeho Jeong 2023-01-31 21:38 ` Daeho Jeong 2023-01-31 21:57 ` Daeho Jeong 2023-01-31 21:57 ` Daeho Jeong 2023-01-31 22:34 ` Daeho Jeong 2023-01-31 22:34 ` Daeho Jeong 2023-02-01 1:40 ` Chao Yu 2023-02-01 1:40 ` Chao Yu 2023-02-01 4:33 ` Daeho Jeong 2023-02-01 4:33 ` Daeho Jeong 2023-02-09 18:18 Daeho Jeong 2023-02-13 9:47 ` Chao Yu 2023-02-13 9:47 ` Chao Yu 2023-02-13 20:14 ` Daeho Jeong 2023-02-13 20:14 ` Daeho Jeong 2023-02-14 1:46 ` Chao Yu 2023-02-14 1:46 ` Chao Yu 2023-02-14 1:47 ` Chao Yu 2023-02-14 1:47 ` Chao Yu 2023-02-14 18:10 ` patchwork-bot+f2fs 2023-02-14 18:10 ` patchwork-bot+f2fs
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8f1c15a3-d056-7709-af45-fe7cba56463f@kernel.org \ --to=chao@kernel.org \ --cc=daeho43@gmail.com \ --cc=daehojeong@google.com \ --cc=kernel-team@android.com \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ --cc=syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.