Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Chao Yu]

On 2022/10/6 0:06, Daeho Jeong wrote:
> On Wed, Oct 5, 2022 at 6:46 AM Chao Yu <chao@xxxxxxxxxx> wrote:
> > On 2022/10/5 1:13, Daeho Jeong wrote:
> > > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > >
> > > We need to make sure i_size doesn't change until atomic write commit is
> > > successful and restore it when commit is failed.
> > >
> > > Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > > ---
> > > v4: move i_size update after clearing atomic file flag in
> > >       f2fs_abort_atomic_write()
> > > v3: make sure inode is clean while atomic writing
> > > ---
> > >    fs/f2fs/f2fs.h    |  1 +
> > >    fs/f2fs/file.c    | 18 +++++++++++-------
> > >    fs/f2fs/inode.c   |  3 +++
> > >    fs/f2fs/segment.c |  7 +++++--
> > >    4 files changed, 20 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index dee7b67a17a6..539da7f12cfc 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -821,6 +821,7 @@ struct f2fs_inode_info {
> > >        unsigned int i_cluster_size;            /* cluster size */
> > >
> > >        unsigned int atomic_write_cnt;
> > > +     loff_t original_i_size;         /* original i_size before atomic write */
> > >    };
> > >
> > >    static inline void get_extent_info(struct extent_info *ext,
> > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > > index 5efe0e4a725a..ce2336d2f688 100644
> > > --- a/fs/f2fs/file.c
> > > +++ b/fs/f2fs/file.c
> > > @@ -1989,6 +1989,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> > >        struct f2fs_inode_info *fi = F2FS_I(inode);
> > >        struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > >        struct inode *pinode;
> > > +     loff_t isize;
> > >        int ret;
> > >
> > >        if (!inode_owner_or_capable(mnt_userns, inode))
> > > @@ -2047,7 +2048,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> > >                f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> > >                goto out;
> > >        }
> > > -     f2fs_i_size_write(fi->cow_inode, i_size_read(inode));
> > > +
> > > +     f2fs_write_inode(inode, NULL);
> > > +
> > > +     isize = i_size_read(inode);
> > > +     fi->original_i_size = isize;
> > > +     f2fs_i_size_write(fi->cow_inode, isize);
> > >
> > >        spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> > >        sbi->atomic_files++;
> > > @@ -2087,16 +2093,14 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
> > >
> > >        if (f2fs_is_atomic_file(inode)) {
> > >                ret = f2fs_commit_atomic_write(inode);
> > > -             if (ret)
> > > -                     goto unlock_out;
> > > -
> > > -             ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> > >                if (!ret)
> > > -                     f2fs_abort_atomic_write(inode, false);
> > > +                     ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> > > +
> > > +             f2fs_abort_atomic_write(inode, ret);
> > >        } else {
> > >                ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
> > >        }
> > > -unlock_out:
> > > +
> > >        inode_unlock(inode);
> > >        mnt_drop_write_file(filp);
> > >        return ret;
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index cde0a3dc80c3..64d7772b4cd9 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -30,6 +30,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))
> > > +             return;
> >
> > One question, after f2fs_inode_dirtied(), atomic_file is added to
> > inode_list[DIRTY_META], and it will be flushed by checkpoint()
> > triggered in between write() and atomic_commit ioctl, is it not
> > expected that inode w/ new i_size will be persisted?
>
> Isn't it okay if we move f2fs_is_atomic_file() ahead of f2fs_inode_dirtied()?

Fine to me.

But another question is, now it allows GC to migrate blocks belong
to atomic files, so, during migration, it may update extent cache,
once largest extent was updated, it will mark inode dirty, but after
this patch, it may lose the extent change? thoughts?

> > Should write_end() skip updating atomic_file's i_size and let
> > atomic_commit() update it if there is no error?
>
> In this case, the user can't see the changed i_size while writing an
> atomic file.

Oh, right, please ignore this comment...

Thanks,

> > Thanks,
> >
> > > +
> > >        mark_inode_dirty_sync(inode);
> > >    }
> > >
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 460048f3c850..abb55cd418c1 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -193,14 +193,17 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > >        if (!f2fs_is_atomic_file(inode))
> > >                return;
> > >
> > > -     if (clean)
> > > -             truncate_inode_pages_final(inode->i_mapping);
> > >        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_FILE);
> > >
> > > +     if (clean) {
> > > +             truncate_inode_pages_final(inode->i_mapping);
> > > +             f2fs_i_size_write(inode, fi->original_i_size);
> > > +     }
> > > +
> > >        spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> > >        sbi->atomic_files--;
> > >        spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);