MIME-Version: 1.0
References: <20220107121215.1912-1-yinxin.x@bytedance.com> <20220107121215.1912-3-yinxin.x@bytedance.com>
In-Reply-To: <20220107121215.1912-3-yinxin.x@bytedance.com>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Fri, 7 Jan 2022 12:14:38 -0800
Message-ID: <CAD+ocbxUaQMjhOof-zphqYs2MvtqtM5dLxmfxmRGrj2sORZ+sQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] ext4: fast commit may miss file actions
To:     Xin Yin <yinxin.x@bytedance.com>
Cc:     "Theodore Y. Ts'o" <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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@gmail.com>

On Fri, Jan 7, 2022 at 4:13 AM Xin Yin <yinxin.x@bytedance.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@bytedance.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
>