On Sun, Nov 14, 2021 at 6:54 PM harshad shirwadkar
[off-list ref] wrote:

Hi Xin,

Thanks for your email and steps to reproduce the issue.

quoted

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 尹欣 [off-list ref] wrote:

quoted

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