Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Chao Yu]

On 2024/9/4 1:07, Daeho Jeong wrote:
> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@xxxxxxxxxx> wrote:
> > On 2024/8/27 4:23, Daeho Jeong wrote:
> > > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > >
> > > Keep atomic file clean while updating and make it dirtied during commit
> > > in order to avoid unnecessary and excessive inode updates in the previous
> > > fix.
> > >
> > > Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> > > Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> > > ---
> > >    fs/f2fs/f2fs.h    |  3 +--
> > >    fs/f2fs/inode.c   | 10 ++++++----
> > >    fs/f2fs/segment.c | 10 ++++++++--
> > >    3 files changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 465b2fd50c70..5a7f6fa8b585 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -801,7 +801,7 @@ enum {
> > >        FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> > >        FI_ALIGNED_WRITE,       /* enable aligned write */
> > >        FI_COW_FILE,            /* indicate COW file */
> > > -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
> > > +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
> > >        FI_ATOMIC_REPLACE,      /* indicate atomic replace */
> > >        FI_OPENED_FILE,         /* indicate file has been opened */
> > >        FI_MAX,                 /* max flag, never be used */
> > > @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
> > >        case FI_INLINE_DOTS:
> > >        case FI_PIN_FILE:
> > >        case FI_COMPRESS_RELEASED:
> > > -     case FI_ATOMIC_COMMITTED:
> > >                f2fs_mark_inode_dirty_sync(inode, true);
> > >        }
> > >    }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 1eb250c6b392..5dd3e55d2be2 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> > >        if (f2fs_inode_dirtied(inode, sync))
> >
> > It will return directly here if inode was dirtied, so it may missed to set
> > FI_ATOMIC_DIRTIED flag?
>
> Is it possible for it to be already dirty, since we already made it
> clean with f2fs_write_inode() when we started the atomic write?

Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
check atomic_file status, and may dirty inode after we started atomic
write, so we'd better detect such race condition and break ioctl to
avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
f2fs_mark_inode_dirty_sync() to detect any other missing cases?

Thanks,

> > Thanks,
> >
> > >                return;
> > >
> > > +     if (f2fs_is_atomic_file(inode)) {
> > > +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > > +             return;
> > > +     }
> > > +
> > >        mark_inode_dirty_sync(inode);
> > >    }
> > >
> > > @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
> > >        ri->i_gid = cpu_to_le32(i_gid_read(inode));
> > >        ri->i_links = cpu_to_le32(inode->i_nlink);
> > >        ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> > > -
> > > -     if (!f2fs_is_atomic_file(inode) ||
> > > -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> > > -             ri->i_size = cpu_to_le64(i_size_read(inode));
> > > +     ri->i_size = cpu_to_le64(i_size_read(inode));
> > >
> > >        if (et) {
> > >                read_lock(&et->lock);
> > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > > index 78c3198a6308..2b5391b229a8 100644
> > > --- a/fs/f2fs/segment.c
> > > +++ b/fs/f2fs/segment.c
> > > @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > >                truncate_inode_pages_final(inode->i_mapping);
> > >
> > >        release_atomic_write_cnt(inode);
> > > -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > >        clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > >        clear_inode_flag(inode, FI_ATOMIC_FILE);
> > > +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> > > +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > > +             f2fs_mark_inode_dirty_sync(inode, true);
> > > +     }
> > >        stat_dec_atomic_inode(inode);
> > >
> > >        F2FS_I(inode)->atomic_write_task = NULL;
> > > @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> > >                sbi->revoked_atomic_block += fi->atomic_write_cnt;
> > >        } 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);