Re: [External] Re: [PATCH RESEND] ext4:fix different behavior of fsync when use fast commit

[Xin Yin <yinxin.x@bytedance.com>]

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