* [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED
@ 2025-03-26 8:46 Zhiguo Niu
2025-03-26 9:26 ` Chao Yu
0 siblings, 1 reply; 6+ messages in thread
From: Zhiguo Niu @ 2025-03-26 8:46 UTC (permalink / raw)
To: jaegeuk, chao
Cc: linux-f2fs-devel, linux-kernel, daehojeong, niuzhiguo84,
zhiguo.niu, ke.wang, Hao_hao.Wang
Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
adds the processing of FI_ATOMIC_DIRTIED in the following two positions,
[1]
f2fs_commit_atomic_write
- __f2fs_commit_atomic_write
- sbi->committed_atomic_block += fi->atomic_write_cnt;
- set_inode_flag(inode, FI_ATOMIC_COMMITTED);
- if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
- clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
- f2fs_mark_inode_dirty_sync(inode, true);
- }
[2]
f2fs_abort_atomic_write
- if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
- clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
- f2fs_mark_inode_dirty_sync(inode, true);
- }
but [1] seems to be redundant:
The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync
still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set
when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and
then do the repeating action of setting FI_ATOMIC_DIRTIED?
So is it enough to do this only in [2]?
Cc: Daeho Jeong <daehojeong@google.com>
Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit")
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
fs/f2fs/segment.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 396ef71..d4ea3af 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
} else {
sbi->committed_atomic_block += fi->atomic_write_cnt;
set_inode_flag(inode, FI_ATOMIC_COMMITTED);
- if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
- clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
- f2fs_mark_inode_dirty_sync(inode, true);
- }
}
__complete_revoke_list(inode, &revoke_list, ret ? true : false);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED 2025-03-26 8:46 [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED Zhiguo Niu @ 2025-03-26 9:26 ` Chao Yu 2025-03-26 10:51 ` Zhiguo Niu 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2025-03-26 9:26 UTC (permalink / raw) To: Zhiguo Niu, daehojeong, jaegeuk Cc: chao, linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang, Hao_hao.Wang On 3/26/25 16:46, Zhiguo Niu wrote: > Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > adds the processing of FI_ATOMIC_DIRTIED in the following two positions, > [1] > f2fs_commit_atomic_write > - __f2fs_commit_atomic_write > - sbi->committed_atomic_block += fi->atomic_write_cnt; > - set_inode_flag(inode, FI_ATOMIC_COMMITTED); > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > - f2fs_mark_inode_dirty_sync(inode, true); > - } > [2] > f2fs_abort_atomic_write > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > - f2fs_mark_inode_dirty_sync(inode, true); > - } > > but [1] seems to be redundant: > The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync > still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set > when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and > then do the repeating action of setting FI_ATOMIC_DIRTIED? > So is it enough to do this only in [2]? Hi Zhiguo, I checked the code again, finally, I got this, could you please take a look? Ping Daeho as well. Subject: [PATCH] f2fs: fix to set atomic write status more clear 1. After we start atomic write in a database file, before committing all data, we'd better not set inode w/ vfs dirty status to avoid redundant updates, instead, we only set inode w/ atomic dirty status. 2. After we commit all data, before committing metadata, we need to clear atomic dirty status, and set vfs dirty status to allow vfs flush dirty inode. Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/inode.c | 4 +++- fs/f2fs/segment.c | 10 ++++++---- fs/f2fs/super.c | 4 +++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index 5c8634eaef7b..f5991e8751b9 100644 --- a/fs/f2fs/inode.c +++ b/fs/f2fs/inode.c @@ -34,7 +34,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) if (f2fs_inode_dirtied(inode, sync)) return; - if (f2fs_is_atomic_file(inode)) + /* only atomic file w/ FI_ATOMIC_COMMITTED can be set vfs dirty */ + if (f2fs_is_atomic_file(inode) && + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) return; mark_inode_dirty_sync(inode); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index dc360b4b0569..28659a71891a 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -376,10 +376,12 @@ static int __f2fs_commit_atomic_write(struct inode *inode) } else { sbi->committed_atomic_block += fi->atomic_write_cnt; set_inode_flag(inode, FI_ATOMIC_COMMITTED); - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); - f2fs_mark_inode_dirty_sync(inode, true); - } + + f2fs_bug_on(sbi, !is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)); + + /* clear atomic dirty status and set vfs dirty status */ + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); + f2fs_mark_inode_dirty_sync(inode, true); } __complete_revoke_list(inode, &revoke_list, ret ? true : false); diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 9a42a1323f42..a5cc9f6ee16a 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1532,7 +1532,9 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync) } spin_unlock(&sbi->inode_lock[DIRTY_META]); - if (!ret && f2fs_is_atomic_file(inode)) + /* if atomic write is not committed, set inode w/ atomic dirty */ + if (!ret && f2fs_is_atomic_file(inode) && + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) set_inode_flag(inode, FI_ATOMIC_DIRTIED); return ret; -- 2.48.1 > > Cc: Daeho Jeong <daehojeong@google.com> > Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > --- > fs/f2fs/segment.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 396ef71..d4ea3af 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode) > } else { > sbi->committed_atomic_block += fi->atomic_write_cnt; > set_inode_flag(inode, FI_ATOMIC_COMMITTED); > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > - f2fs_mark_inode_dirty_sync(inode, true); > - } > } > > __complete_revoke_list(inode, &revoke_list, ret ? true : false); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED 2025-03-26 9:26 ` Chao Yu @ 2025-03-26 10:51 ` Zhiguo Niu 2025-03-26 12:10 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Zhiguo Niu @ 2025-03-26 10:51 UTC (permalink / raw) To: Chao Yu Cc: Zhiguo Niu, daehojeong, jaegeuk, linux-f2fs-devel, linux-kernel, ke.wang, Hao_hao.Wang Chao Yu <chao@kernel.org> 于2025年3月26日周三 17:26写道： > > On 3/26/25 16:46, Zhiguo Niu wrote: > > Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > > adds the processing of FI_ATOMIC_DIRTIED in the following two positions, > > [1] > > f2fs_commit_atomic_write > > - __f2fs_commit_atomic_write > > - sbi->committed_atomic_block += fi->atomic_write_cnt; > > - set_inode_flag(inode, FI_ATOMIC_COMMITTED); > > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > > - f2fs_mark_inode_dirty_sync(inode, true); > > - } > > [2] > > f2fs_abort_atomic_write > > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > > - f2fs_mark_inode_dirty_sync(inode, true); > > - } > > > > but [1] seems to be redundant: > > The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync > > still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set > > when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and > > then do the repeating action of setting FI_ATOMIC_DIRTIED? > > So is it enough to do this only in [2]? > > Hi Zhiguo, > > I checked the code again, finally, I got this, could you please take > a look? > > Ping Daeho as well. > > Subject: [PATCH] f2fs: fix to set atomic write status more clear > > 1. After we start atomic write in a database file, before committing > all data, we'd better not set inode w/ vfs dirty status to avoid > redundant updates, instead, we only set inode w/ atomic dirty status. > > 2. After we commit all data, before committing metadata, we need to > clear atomic dirty status, and set vfs dirty status to allow vfs flush > dirty inode. > Hi Chao, these looks more clear. > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/inode.c | 4 +++- > fs/f2fs/segment.c | 10 ++++++---- > fs/f2fs/super.c | 4 +++- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > index 5c8634eaef7b..f5991e8751b9 100644 > --- a/fs/f2fs/inode.c > +++ b/fs/f2fs/inode.c > @@ -34,7 +34,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) > if (f2fs_inode_dirtied(inode, sync)) > return; > > - if (f2fs_is_atomic_file(inode)) > + /* only atomic file w/ FI_ATOMIC_COMMITTED can be set vfs dirty */ > + if (f2fs_is_atomic_file(inode) && > + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) > return; > > mark_inode_dirty_sync(inode); > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index dc360b4b0569..28659a71891a 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -376,10 +376,12 @@ static int __f2fs_commit_atomic_write(struct inode *inode) > } else { > sbi->committed_atomic_block += fi->atomic_write_cnt; > set_inode_flag(inode, FI_ATOMIC_COMMITTED); > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > - f2fs_mark_inode_dirty_sync(inode, true); > - } > + > + f2fs_bug_on(sbi, !is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)); but FI_ATOMIC_DIRTIED may not be set when atomic file is committing? thanks! > + > + /* clear atomic dirty status and set vfs dirty status */ > + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > + f2fs_mark_inode_dirty_sync(inode, true); > } > > __complete_revoke_list(inode, &revoke_list, ret ? true : false); > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 9a42a1323f42..a5cc9f6ee16a 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1532,7 +1532,9 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync) > } > spin_unlock(&sbi->inode_lock[DIRTY_META]); > > - if (!ret && f2fs_is_atomic_file(inode)) > + /* if atomic write is not committed, set inode w/ atomic dirty */ > + if (!ret && f2fs_is_atomic_file(inode) && > + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) > set_inode_flag(inode, FI_ATOMIC_DIRTIED); > > return ret; > -- > 2.48.1 > > > > > > Cc: Daeho Jeong <daehojeong@google.com> > > Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > > --- > > fs/f2fs/segment.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > index 396ef71..d4ea3af 100644 > > --- a/fs/f2fs/segment.c > > +++ b/fs/f2fs/segment.c > > @@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode) > > } else { > > sbi->committed_atomic_block += fi->atomic_write_cnt; > > set_inode_flag(inode, FI_ATOMIC_COMMITTED); > > - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > > - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > > - f2fs_mark_inode_dirty_sync(inode, true); > > - } > > } > > > > __complete_revoke_list(inode, &revoke_list, ret ? true : false); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED 2025-03-26 10:51 ` Zhiguo Niu @ 2025-03-26 12:10 ` Chao Yu 2025-03-26 18:51 ` [f2fs-dev] " Daeho Jeong 0 siblings, 1 reply; 6+ messages in thread From: Chao Yu @ 2025-03-26 12:10 UTC (permalink / raw) To: Zhiguo Niu Cc: chao, Zhiguo Niu, daehojeong, jaegeuk, linux-f2fs-devel, linux-kernel, ke.wang, Hao_hao.Wang On 3/26/25 18:51, Zhiguo Niu wrote: > Chao Yu <chao@kernel.org> 于2025年3月26日周三 17:26写道： >> >> On 3/26/25 16:46, Zhiguo Niu wrote: >>> Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") >>> adds the processing of FI_ATOMIC_DIRTIED in the following two positions, >>> [1] >>> f2fs_commit_atomic_write >>> - __f2fs_commit_atomic_write >>> - sbi->committed_atomic_block += fi->atomic_write_cnt; >>> - set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>> - f2fs_mark_inode_dirty_sync(inode, true); >>> - } >>> [2] >>> f2fs_abort_atomic_write >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>> - f2fs_mark_inode_dirty_sync(inode, true); >>> - } >>> >>> but [1] seems to be redundant: >>> The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync >>> still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set >>> when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and >>> then do the repeating action of setting FI_ATOMIC_DIRTIED? >>> So is it enough to do this only in [2]? >> >> Hi Zhiguo, >> >> I checked the code again, finally, I got this, could you please take >> a look? >> >> Ping Daeho as well. >> >> Subject: [PATCH] f2fs: fix to set atomic write status more clear >> >> 1. After we start atomic write in a database file, before committing >> all data, we'd better not set inode w/ vfs dirty status to avoid >> redundant updates, instead, we only set inode w/ atomic dirty status. >> >> 2. After we commit all data, before committing metadata, we need to >> clear atomic dirty status, and set vfs dirty status to allow vfs flush >> dirty inode. >> > Hi Chao, > these looks more clear. >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/inode.c | 4 +++- >> fs/f2fs/segment.c | 10 ++++++---- >> fs/f2fs/super.c | 4 +++- >> 3 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> index 5c8634eaef7b..f5991e8751b9 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -34,7 +34,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) >> if (f2fs_inode_dirtied(inode, sync)) >> return; >> >> - if (f2fs_is_atomic_file(inode)) >> + /* only atomic file w/ FI_ATOMIC_COMMITTED can be set vfs dirty */ >> + if (f2fs_is_atomic_file(inode) && >> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) >> return; >> >> mark_inode_dirty_sync(inode); >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >> index dc360b4b0569..28659a71891a 100644 >> --- a/fs/f2fs/segment.c >> +++ b/fs/f2fs/segment.c >> @@ -376,10 +376,12 @@ static int __f2fs_commit_atomic_write(struct inode *inode) >> } else { >> sbi->committed_atomic_block += fi->atomic_write_cnt; >> set_inode_flag(inode, FI_ATOMIC_COMMITTED); >> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >> - f2fs_mark_inode_dirty_sync(inode, true); >> - } >> + >> + f2fs_bug_on(sbi, !is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)); > but FI_ATOMIC_DIRTIED may not be set when atomic file is committing? > thanks! inc_valid_block_count() will set FI_ATOMIC_DIRTIED for inode at least? - __f2fs_commit_atomic_write - __replace_atomic_write_block - inc_valid_block_count - f2fs_i_blocks_write - f2fs_mark_inode_dirty_sync Thanks, >> + >> + /* clear atomic dirty status and set vfs dirty status */ >> + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >> + f2fs_mark_inode_dirty_sync(inode, true); >> } >> >> __complete_revoke_list(inode, &revoke_list, ret ? true : false); >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >> index 9a42a1323f42..a5cc9f6ee16a 100644 >> --- a/fs/f2fs/super.c >> +++ b/fs/f2fs/super.c >> @@ -1532,7 +1532,9 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync) >> } >> spin_unlock(&sbi->inode_lock[DIRTY_META]); >> >> - if (!ret && f2fs_is_atomic_file(inode)) >> + /* if atomic write is not committed, set inode w/ atomic dirty */ >> + if (!ret && f2fs_is_atomic_file(inode) && >> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) >> set_inode_flag(inode, FI_ATOMIC_DIRTIED); >> >> return ret; >> -- >> 2.48.1 >> >> >>> >>> Cc: Daeho Jeong <daehojeong@google.com> >>> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> >>> --- >>> fs/f2fs/segment.c | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index 396ef71..d4ea3af 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode) >>> } else { >>> sbi->committed_atomic_block += fi->atomic_write_cnt; >>> set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>> - f2fs_mark_inode_dirty_sync(inode, true); >>> - } >>> } >>> >>> __complete_revoke_list(inode, &revoke_list, ret ? true : false); >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED 2025-03-26 12:10 ` Chao Yu @ 2025-03-26 18:51 ` Daeho Jeong 2025-03-27 3:21 ` Chao Yu 0 siblings, 1 reply; 6+ messages in thread From: Daeho Jeong @ 2025-03-26 18:51 UTC (permalink / raw) To: Chao Yu Cc: Zhiguo Niu, daehojeong, ke.wang, linux-kernel, linux-f2fs-devel, Zhiguo Niu, jaegeuk, Hao_hao.Wang On Wed, Mar 26, 2025 at 5:12 AM Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> wrote: > > On 3/26/25 18:51, Zhiguo Niu wrote: > > Chao Yu <chao@kernel.org> 于2025年3月26日周三 17:26写道： > >> > >> On 3/26/25 16:46, Zhiguo Niu wrote: > >>> Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > >>> adds the processing of FI_ATOMIC_DIRTIED in the following two positions, > >>> [1] > >>> f2fs_commit_atomic_write > >>> - __f2fs_commit_atomic_write > >>> - sbi->committed_atomic_block += fi->atomic_write_cnt; > >>> - set_inode_flag(inode, FI_ATOMIC_COMMITTED); > >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > >>> - f2fs_mark_inode_dirty_sync(inode, true); > >>> - } > >>> [2] > >>> f2fs_abort_atomic_write > >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > >>> - f2fs_mark_inode_dirty_sync(inode, true); > >>> - } > >>> > >>> but [1] seems to be redundant: > >>> The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync > >>> still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set > >>> when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and > >>> then do the repeating action of setting FI_ATOMIC_DIRTIED? > >>> So is it enough to do this only in [2]? > >> > >> Hi Zhiguo, > >> > >> I checked the code again, finally, I got this, could you please take > >> a look? > >> > >> Ping Daeho as well. > >> > >> Subject: [PATCH] f2fs: fix to set atomic write status more clear > >> > >> 1. After we start atomic write in a database file, before committing > >> all data, we'd better not set inode w/ vfs dirty status to avoid > >> redundant updates, instead, we only set inode w/ atomic dirty status. > >> > >> 2. After we commit all data, before committing metadata, we need to > >> clear atomic dirty status, and set vfs dirty status to allow vfs flush > >> dirty inode. > >> > > Hi Chao, > > these looks more clear. > >> Signed-off-by: Chao Yu <chao@kernel.org> > >> --- > >> fs/f2fs/inode.c | 4 +++- > >> fs/f2fs/segment.c | 10 ++++++---- > >> fs/f2fs/super.c | 4 +++- > >> 3 files changed, 12 insertions(+), 6 deletions(-) > >> > >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > >> index 5c8634eaef7b..f5991e8751b9 100644 > >> --- a/fs/f2fs/inode.c > >> +++ b/fs/f2fs/inode.c > >> @@ -34,7 +34,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) > >> if (f2fs_inode_dirtied(inode, sync)) > >> return; > >> > >> - if (f2fs_is_atomic_file(inode)) > >> + /* only atomic file w/ FI_ATOMIC_COMMITTED can be set vfs dirty */ > >> + if (f2fs_is_atomic_file(inode) && > >> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) > >> return; > >> > >> mark_inode_dirty_sync(inode); > >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >> index dc360b4b0569..28659a71891a 100644 > >> --- a/fs/f2fs/segment.c > >> +++ b/fs/f2fs/segment.c > >> @@ -376,10 +376,12 @@ static int __f2fs_commit_atomic_write(struct inode *inode) > >> } else { > >> sbi->committed_atomic_block += fi->atomic_write_cnt; > >> set_inode_flag(inode, FI_ATOMIC_COMMITTED); > >> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > >> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > >> - f2fs_mark_inode_dirty_sync(inode, true); > >> - } > >> + > >> + f2fs_bug_on(sbi, !is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)); > > but FI_ATOMIC_DIRTIED may not be set when atomic file is committing? > > thanks! > > inc_valid_block_count() will set FI_ATOMIC_DIRTIED for inode at least? > > - __f2fs_commit_atomic_write > - __replace_atomic_write_block > - inc_valid_block_count > - f2fs_i_blocks_write > - f2fs_mark_inode_dirty_sync Maybe, no write scenario? open -> atomic_write_start -> no write -> commit? > > Thanks, > > >> + > >> + /* clear atomic dirty status and set vfs dirty status */ > >> + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > >> + f2fs_mark_inode_dirty_sync(inode, true); > >> } > >> > >> __complete_revoke_list(inode, &revoke_list, ret ? true : false); > >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >> index 9a42a1323f42..a5cc9f6ee16a 100644 > >> --- a/fs/f2fs/super.c > >> +++ b/fs/f2fs/super.c > >> @@ -1532,7 +1532,9 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync) > >> } > >> spin_unlock(&sbi->inode_lock[DIRTY_META]); > >> > >> - if (!ret && f2fs_is_atomic_file(inode)) > >> + /* if atomic write is not committed, set inode w/ atomic dirty */ > >> + if (!ret && f2fs_is_atomic_file(inode) && > >> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) > >> set_inode_flag(inode, FI_ATOMIC_DIRTIED); > >> > >> return ret; > >> -- > >> 2.48.1 > >> > >> > >>> > >>> Cc: Daeho Jeong <daehojeong@google.com> > >>> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") > >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> > >>> --- > >>> fs/f2fs/segment.c | 4 ---- > >>> 1 file changed, 4 deletions(-) > >>> > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >>> index 396ef71..d4ea3af 100644 > >>> --- a/fs/f2fs/segment.c > >>> +++ b/fs/f2fs/segment.c > >>> @@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode) > >>> } else { > >>> sbi->committed_atomic_block += fi->atomic_write_cnt; > >>> set_inode_flag(inode, FI_ATOMIC_COMMITTED); > >>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { > >>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); > >>> - f2fs_mark_inode_dirty_sync(inode, true); > >>> - } > >>> } > >>> > >>> __complete_revoke_list(inode, &revoke_list, ret ? true : false); > >> > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [f2fs-dev] [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED 2025-03-26 18:51 ` [f2fs-dev] " Daeho Jeong @ 2025-03-27 3:21 ` Chao Yu 0 siblings, 0 replies; 6+ messages in thread From: Chao Yu @ 2025-03-27 3:21 UTC (permalink / raw) To: Daeho Jeong Cc: chao, Zhiguo Niu, daehojeong, ke.wang, linux-kernel, linux-f2fs-devel, Zhiguo Niu, jaegeuk, Hao_hao.Wang On 3/27/25 02:51, Daeho Jeong wrote: > On Wed, Mar 26, 2025 at 5:12 AM Chao Yu via Linux-f2fs-devel > <linux-f2fs-devel@lists.sourceforge.net> wrote: >> >> On 3/26/25 18:51, Zhiguo Niu wrote: >>> Chao Yu <chao@kernel.org> 于2025年3月26日周三 17:26写道： >>>> >>>> On 3/26/25 16:46, Zhiguo Niu wrote: >>>>> Commit fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") >>>>> adds the processing of FI_ATOMIC_DIRTIED in the following two positions, >>>>> [1] >>>>> f2fs_commit_atomic_write >>>>> - __f2fs_commit_atomic_write >>>>> - sbi->committed_atomic_block += fi->atomic_write_cnt; >>>>> - set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> - f2fs_mark_inode_dirty_sync(inode, true); >>>>> - } >>>>> [2] >>>>> f2fs_abort_atomic_write >>>>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> - f2fs_mark_inode_dirty_sync(inode, true); >>>>> - } >>>>> >>>>> but [1] seems to be redundant: >>>>> The atomic file flag FI_ATOMIC_FILE is still set here, so f2fs_mark_inode_dirty_sync >>>>> still does not set the dirty state to vfs. If FI_ATOMIC_DIRTIED was originally set >>>>> when atomic file is committing, then FI_ATOMIC_DIRTIED is just cleared here, and >>>>> then do the repeating action of setting FI_ATOMIC_DIRTIED? >>>>> So is it enough to do this only in [2]? >>>> >>>> Hi Zhiguo, >>>> >>>> I checked the code again, finally, I got this, could you please take >>>> a look? >>>> >>>> Ping Daeho as well. >>>> >>>> Subject: [PATCH] f2fs: fix to set atomic write status more clear >>>> >>>> 1. After we start atomic write in a database file, before committing >>>> all data, we'd better not set inode w/ vfs dirty status to avoid >>>> redundant updates, instead, we only set inode w/ atomic dirty status. >>>> >>>> 2. After we commit all data, before committing metadata, we need to >>>> clear atomic dirty status, and set vfs dirty status to allow vfs flush >>>> dirty inode. >>>> >>> Hi Chao, >>> these looks more clear. >>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>> --- >>>> fs/f2fs/inode.c | 4 +++- >>>> fs/f2fs/segment.c | 10 ++++++---- >>>> fs/f2fs/super.c | 4 +++- >>>> 3 files changed, 12 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>>> index 5c8634eaef7b..f5991e8751b9 100644 >>>> --- a/fs/f2fs/inode.c >>>> +++ b/fs/f2fs/inode.c >>>> @@ -34,7 +34,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) >>>> if (f2fs_inode_dirtied(inode, sync)) >>>> return; >>>> >>>> - if (f2fs_is_atomic_file(inode)) >>>> + /* only atomic file w/ FI_ATOMIC_COMMITTED can be set vfs dirty */ >>>> + if (f2fs_is_atomic_file(inode) && >>>> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) >>>> return; >>>> >>>> mark_inode_dirty_sync(inode); >>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>> index dc360b4b0569..28659a71891a 100644 >>>> --- a/fs/f2fs/segment.c >>>> +++ b/fs/f2fs/segment.c >>>> @@ -376,10 +376,12 @@ static int __f2fs_commit_atomic_write(struct inode *inode) >>>> } else { >>>> sbi->committed_atomic_block += fi->atomic_write_cnt; >>>> set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>> - f2fs_mark_inode_dirty_sync(inode, true); >>>> - } >>>> + >>>> + f2fs_bug_on(sbi, !is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)); >>> but FI_ATOMIC_DIRTIED may not be set when atomic file is committing? >>> thanks! >> >> inc_valid_block_count() will set FI_ATOMIC_DIRTIED for inode at least? >> >> - __f2fs_commit_atomic_write >> - __replace_atomic_write_block >> - inc_valid_block_count >> - f2fs_i_blocks_write >> - f2fs_mark_inode_dirty_sync > > Maybe, no write scenario? open -> atomic_write_start -> no write -> commit? Oh, You're right. Thanks, > >> >> Thanks, >> >>>> + >>>> + /* clear atomic dirty status and set vfs dirty status */ >>>> + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>> + f2fs_mark_inode_dirty_sync(inode, true); >>>> } >>>> >>>> __complete_revoke_list(inode, &revoke_list, ret ? true : false); >>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>> index 9a42a1323f42..a5cc9f6ee16a 100644 >>>> --- a/fs/f2fs/super.c >>>> +++ b/fs/f2fs/super.c >>>> @@ -1532,7 +1532,9 @@ int f2fs_inode_dirtied(struct inode *inode, bool sync) >>>> } >>>> spin_unlock(&sbi->inode_lock[DIRTY_META]); >>>> >>>> - if (!ret && f2fs_is_atomic_file(inode)) >>>> + /* if atomic write is not committed, set inode w/ atomic dirty */ >>>> + if (!ret && f2fs_is_atomic_file(inode) && >>>> + !is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) >>>> set_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>> >>>> return ret; >>>> -- >>>> 2.48.1 >>>> >>>> >>>>> >>>>> Cc: Daeho Jeong <daehojeong@google.com> >>>>> Fixes: fccaa81de87e ("f2fs: prevent atomic file from being dirtied before commit") >>>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> >>>>> --- >>>>> fs/f2fs/segment.c | 4 ---- >>>>> 1 file changed, 4 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>>> index 396ef71..d4ea3af 100644 >>>>> --- a/fs/f2fs/segment.c >>>>> +++ b/fs/f2fs/segment.c >>>>> @@ -376,10 +376,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode) >>>>> } else { >>>>> sbi->committed_atomic_block += fi->atomic_write_cnt; >>>>> set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>>> - if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>>> - clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> - f2fs_mark_inode_dirty_sync(inode, true); >>>>> - } >>>>> } >>>>> >>>>> __complete_revoke_list(inode, &revoke_list, ret ? true : false); >>>> >> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-27 3:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-26 8:46 [RFC PATCH] f2fs: remove some redundant flow about FI_ATOMIC_DIRTIED Zhiguo Niu 2025-03-26 9:26 ` Chao Yu 2025-03-26 10:51 ` Zhiguo Niu 2025-03-26 12:10 ` Chao Yu 2025-03-26 18:51 ` [f2fs-dev] " Daeho Jeong 2025-03-27 3:21 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox