linux-kernel - Re: [External] Re: [PATCH 2/2] ext4: fast commit may miss file actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK896s5EV9AOenyPrHJFk7=R--RGhH0eWeVU55vnScSJmM6qwA@mail.gmail.com>
Date:   Sun, 9 Jan 2022 11:06:46 +0800
From:   Xin Yin <yinxin.x@...edance.com>
To:     harshad shirwadkar <harshadshirwadkar@...il.com>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH 2/2] ext4: fast commit may miss file actions

Thanks, you are right , the EXT4_MF_FC_COMMITTING is not needed after
this change , I will do the cleanup for it , and send a v2 patches
set.

Thanks,
Xin Yin

On Sat, Jan 8, 2022 at 4:14 AM harshad shirwadkar
<harshadshirwadkar@...il.com> wrote:
>
> Makes sense. With this change, we don't really need
> EXT4_MF_FC_COMMITTING flag anymore. So, we can drop it. But other than
> that, this patch looks good.
>
> Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
>
> On Fri, Jan 7, 2022 at 4:13 AM Xin Yin <yinxin.x@...edance.com> wrote:
> >
> > in the follow scenario:
> > 1. jbd start transaction n
> > 2. task A get new handle for transaction n+1
> > 3. task A do some actions and add inode to FC_Q_MAIN fc_q
> > 4. jbd complete transaction n and clear FC_Q_MAIN fc_q
> > 5. task A call fsync
> >
> > fast commit will lost the file actions during a full commit.
> >
> > we should also add updates to staging queue during a full commit.
> > and in ext4_fc_cleanup(), when reset a inode's fc track range, check
> > it's i_sync_tid, if it bigger than current transaction tid, do not
> > rest it, or we will lost the track range.
> >
> > Signed-off-by: Xin Yin <yinxin.x@...edance.com>
> > ---
> >  fs/ext4/fast_commit.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> > index 3673d4798af3..4cea92aec7c4 100644
> > --- a/fs/ext4/fast_commit.c
> > +++ b/fs/ext4/fast_commit.c
> > @@ -365,7 +365,8 @@ static int ext4_fc_track_template(
> >         spin_lock(&sbi->s_fc_lock);
> >         if (list_empty(&EXT4_I(inode)->i_fc_list))
> >                 list_add_tail(&EXT4_I(inode)->i_fc_list,
> > -                               (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_COMMITTING)) ?
> > +                               (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
> > +                                sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ?
> >                                 &sbi->s_fc_q[FC_Q_STAGING] :
> >                                 &sbi->s_fc_q[FC_Q_MAIN]);
> >         spin_unlock(&sbi->s_fc_lock);
> > @@ -418,7 +419,8 @@ static int __track_dentry_update(struct inode *inode, void *arg, bool update)
> >         node->fcd_name.len = dentry->d_name.len;
> >
> >         spin_lock(&sbi->s_fc_lock);
> > -       if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_COMMITTING))
> > +       if (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
> > +               sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING)
> >                 list_add_tail(&node->fcd_list,
> >                                 &sbi->s_fc_dentry_q[FC_Q_STAGING]);
> >         else
> > @@ -1202,7 +1204,8 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)
> >                 list_del_init(&iter->i_fc_list);
> >                 ext4_clear_inode_state(&iter->vfs_inode,
> >                                        EXT4_STATE_FC_COMMITTING);
> > -               ext4_fc_reset_inode(&iter->vfs_inode);
> > +               if (iter->i_sync_tid <= tid)
> > +                       ext4_fc_reset_inode(&iter->vfs_inode);
> >                 /* Make sure EXT4_STATE_FC_COMMITTING bit is clear */
> >                 smp_mb();
> >  #if (BITS_PER_LONG < 64)
> > --
> > 2.20.1
> >