MIME-Version: 1.0
References: <CAK896s5szH8qbDGBDo74hyz5iM4QrjbMJkMjh-f1sZh6K0ozTg@mail.gmail.com>
 <CAD+ocbyQQym54ZkY9Jdidju_bw9Pj32sGM3qigABCu-Uj3n4Hw@mail.gmail.com>
In-Reply-To: <CAD+ocbyQQym54ZkY9Jdidju_bw9Pj32sGM3qigABCu-Uj3n4Hw@mail.gmail.com>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Sun, 14 Nov 2021 18:59:18 -0800
Message-ID: <CAD+ocbzBg8bnBZcVcQ-kU=zMZWYzGbw=HC34Zb=f9zHXyos8XQ@mail.gmail.com>
Subject: Re: [Question] ext4: different behavior of fsync when use fast commit
To:     =?UTF-8?B?5bC55qyj?= <yinxin.x@bytedance.com>
Cc:     "Theodore Y. Ts'o" <tytso@mit.edu>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Nov 14, 2021 at 6:54 PM harshad shirwadkar
<harshadshirwadkar@gmail.com> wrote:
>
> Hi Xin,
>
> Thanks for your email and steps to reproduce the issue.
>
> > But fast commit did change the behavior of fsync in ext4,  is this as e=
xpected ?
> No this is not expected. Fast commits should not change behavior of
> fsync, so thanks for spotting it.
>
> After taking a deeper look at the issue, I think the problem is that
> fast commit intentionally avoids committing directory inodes and
> instead just records that "file F has been added to / deleted from
> directory D". The recovery code does the actual work of updating the
> directory . It saves us space in the precious fast commit area.
>
> While it is okay to skip "addition of dentry" or "deletion of dentry"
> events on a directory, it is not okay to skip "creation of directory".
To clarify and rephrase this better: While it is okay to skip
recording of directory inode during addition of dentry or deletion of
dentry events on a directory (which is the reason why
ext4_fc_track_link() and ext4_fc_track_unlink() pass "enqueue =3D 0" to
ext4_fc_track_template()), it is not okay to skip recording of the
directory inode during creation of directory event - which is what
fast commit code was unintentionally doing.
> So, you're right, we should be passing "enqueue =3D 1" to
> ext4_fc_track_template() which would tell it to also add the inode to
> "the modified inodes" queue. Once the inode is in the modified inode
> queue, commit routine first commits the inode and records addition of
> dentry to its parent inode.
>
> Please feel free to send a patch to fix this.
>
> Thanks,
> Harshad
>
>
> On Wed, Nov 10, 2021 at 11:10 PM =E5=B0=B9=E6=AC=A3 <yinxin.x@bytedance.c=
om> wrote:
> >
> > Hi,
> >
> >
> > Recently, I=E2=80=98m doing some testing with fast commit feature , and=
 found
> > there is a slight difference on fsync compared with the normal
> > journaling scheme.
> >
> > Here is the example:
> >
> > -mkdir test/
> >
> > -create&write test/a.txt
> >
> > -fsync test/a.txt
> >
> > -crash (before a full commit)
> >
> >
> >
> > If fast commit is used then =E2=80=9Ca.txt=E2=80=9D will lost. While th=
e normal
> > journaling can recover it.
> >
> > Refer to the description of fsync [1],  fsync will not guarantee the
> > parent directory to be persisted. So I think it is not an issue.
> >
> > But fast commit did change the behavior of fsync in ext4,  is this as e=
xpected ?
> >
> >
> >
> > For the root cause of this difference, I found that fast commit will
> > not add a EXT4_FC_TAG_CREAT tag for directory creation.
> >
> > In func ext4_fc_commit_dentry_updates(), only directories in s_fc_q
> > list can be added with EXT4_FC_TAG_CREAT,but seams the newly created
> > directory inode has no change to be added to s_fc_q.
> >
> >
> >
> > Shall we just change the =E2=80=9Cenqueue=E2=80=9D param of ext4_fc_tra=
ck_template()
> > to 1 , which in __ext4_fc_track_create()?  And make fast commit record
> > all the inode creation, and do the same things as normal journaling.
> >
> >
> >
> > [1] https://man7.org/linux/man-pages/man2/fdatasync.2.html
> >
> >
> >
> > BR,
> >
> > Xin Yin