linux-ext4 - Re: [Question] ext4: different behavior of fsync when use fast commit

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] [day] [month] [year] [list]

Message-ID: <CAD+ocbzBg8bnBZcVcQ-kU=zMZWYzGbw=HC34Zb=f9zHXyos8XQ@mail.gmail.com>
Date:   Sun, 14 Nov 2021 18:59:18 -0800
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     尹欣 <yinxin.x@...edance.com>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [Question] ext4: different behavior of fsync when use fast commit

On Sun, Nov 14, 2021 at 6:54 PM harshad shirwadkar
<harshadshirwadkar@...il.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 expected ?
> 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 = 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 = 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 尹欣 <yinxin.x@...edance.com> wrote:
> >
> > Hi,
> >
> >
> > Recently, I‘m 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 “a.txt” will lost. While the 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 expected ?
> >
> >
> >
> > 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 “enqueue” param of ext4_fc_track_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