Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Chao Yu]

On 2022/10/1 4:01, Daeho Jeong wrote:
> On Fri, Sep 30, 2022 at 9:04 AM Daeho Jeong <daeho43@xxxxxxxxx> wrote:
> > > > > > > Hi Daeho,
> > > > > > >
> > > > > > > isize should be updated after tagging inode as atomic_write one?
> > > > > > > otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
> > > > > > > latter checkpoint may persist that change? IIUC...
> > > > > > >
> > > > > > > Thanks,
> > > > > >
> > > > > > Hi Chao,
> > > > > >
> > > > > > The first patch of this patchset prevents the inode page from being
> > > > > > updated as dirty for atomic file cases.
> > > > > > Is there any other chances for the inode page to be marked as dirty?
> > > > >
> > > > > I mean:
> > > > >
> > > > > Thread A                                Thread B
> > > > > - f2fs_ioc_start_atomic_write
> > > > >     - f2fs_i_size_write(inode, 0)
> > > > >      - f2fs_mark_inode_dirty_sync
> > > > >                                           - checkpoint
> > > > >                                            - persist inode with incorrect zero isize
> > > > >
> > > > >     - set_inode_flag(inode, FI_ATOMIC_FILE)
> > > > >
> > > > > Am I missing something?
> > > >
> > > > So, f2fs_mark_inode_dirty_sync() will not work for atomic files
> > > > anymore, which means it doesn't make the inode dirty.
> > > > Plz, refer to the first patch of this patchset. Or I might be confused
> > > > with something. :(
> > >
> > > I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
> > > as dirty.
> > >
> > > Thanks,
> >
> > Oh, I was confused that f2fs_update_inode() is called in
> > f2fs_mark_inode_dirty_sync().
> > That is called in f2fs_write_inode(). Let me fix this.
>
> Hmm, I think the issue was already there before my patch.
> So, how about making the inode flushed and clean if the inode is
> already dirty when starting atomic write?

What I mean is:

---
 fs/f2fs/file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e4b6e51086a3..31b229678b1d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2053,6 +2053,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)

 	isize = i_size_read(inode);
 	fi->original_i_size = isize;
+
+	set_inode_flag(inode, FI_ATOMIC_FILE);
+
 	if (truncate) {
 		set_inode_flag(inode, FI_ATOMIC_REPLACE);
 		truncate_inode_pages_final(inode->i_mapping);
@@ -2063,7 +2066,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);
 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
--


Let's set FI_ATOMIC_FILE flag before f2fs_i_size_write(inode, 0), so
- f2fs_ioc_start_atomic_write
 - f2fs_i_size_write(, 0)
  - f2fs_mark_inode_dirty_sync
   check f2fs_is_atomic_file() and return correctly.

And for the case the inode is dirty before f2fs_i_size_write(, 0), we
can call f2fs_write_inode() to flush dirty feilds into inode page, and
make inode clean.

> > Thanks,