MIME-Version: 1.0
References: <20211224065728.5820-1-yinxin.x@bytedance.com> <YdZgYvC4K87PiMfO@mit.edu>
In-Reply-To: <YdZgYvC4K87PiMfO@mit.edu>
From:   Xin Yin <yinxin.x@bytedance.com>
Date:   Fri, 7 Jan 2022 16:49:01 +0800
Message-ID: <CAK896s7kJ47Q857u-+vBsera95tSJkxMy2Qhqk-91YjbR3QSdA@mail.gmail.com>
Subject: Re: [External] Re: [PATCH RESEND] ext4:fix different behavior of
 fsync when use fast commit
To:     "Theodore Ts'o" <tytso@mit.edu>
Cc:     harshadshirwadkar@gmail.com, linux-ext4@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks for the explanation and direction , I will do another fix for this issue.


Best Regards,
Xin Yin


On Thu, Jan 6, 2022 at 11:22 AM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Dec 24, 2021 at 02:57:28PM +0800, Xin Yin wrote:
> > For the follow test example:
> > -mkdir test/
> > -create&write test/a.txt
> > -fsync test/a.txt
> > -crash (before a full commit)
> >
> > If fast commit is used then "a.txt" will lost, while the normal
> > journaling can recover it.
>
> The problem is that your proposed fix:
>
> > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> > index 3deb97b22ca4..4b843648ffe5 100644
> > --- a/fs/ext4/fast_commit.c
> > +++ b/fs/ext4/fast_commit.c
> > @@ -423,7 +423,7 @@ void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
> >       args.op = EXT4_FC_TAG_CREAT;
> >
> >       ret = ext4_fc_track_template(handle, inode, __track_dentry_update,
> > -                                     (void *)&args, 0);
> > +                                     (void *)&args, 1);
> >       trace_ext4_fc_track_create(inode, dentry, ret);
> >  }
>
> affects both file creations as well as directory creations (mkdir).
> Putting the inode on the fast commit list is something that is meant
> for files, and means that on a fast commit we need to force the data
> blocks out.  So it seems that isn't the right fix for the problem.
>
> Why do something really simple?  Look at the parent directory's inode,
> and check its i_sync_tid.  If it's not equal to
> handle->h_transaction->t_tid, then it's safe to do the fast commit.
> If it's equal to the current transaction, we can either force a full
> commit.
>
> Optionally, in the case where i_sync_tid == current tid, since there's
> a chance that the parent directory's inode could have been freshly
> fetched from disk (see __ext4_iget() in fs/ext4/inode.c), we could
> compare its i_crtime against ktime_get_real_seconds(), and if the
> inode was created in the last 2*journal->j_commit_interval/HZ seconds,
> it's safe to do a fast commit; otherwise do a full commit.
>
> Cheers,
>
>                                         - Ted