Page MenuHomeFreeBSD

UFS: write inode block for fdatasync(2) if pointers in inode where allocated
ClosedPublic

Authored by kib on May 30 2020, 4:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 11 2022, 7:34 AM
Unknown Object (File)
Dec 11 2022, 6:37 AM
Unknown Object (File)
Dec 2 2022, 9:09 AM
Subscribers

Details

Summary

The fdatasync() description in POSIX specifies that

all I/O operations shall be completed as defined for synchronized I/O data integrity completion.

and then the explanation of Synchronized I/O Data Integrity Completion

The write is complete only when the data specified in the write request is successfully transferred and all file system information required to retrieve the data is successfully transferred.

For UFS this means that all pointers must be on disk. Indirect pointers already contribute to the list of dirty data blocks, so only direct blocks and root pointers for indirect blocks, both of which reside in inode block, should be taken care of. In ffs_balloc(), mark the inode with the new flag IN_IBLKDATA that specifies that ffs_syncvnode() call to ffs_update() needs to flush the inode block.

N.B. I will do additional pass after this change, to redefine waitfor argument for ffs_update() symbolically.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.May 30 2020, 4:34 PM
kib added a reviewer: mckusick.
kib added a subscriber: tmunro.

This seems correct at a high level. It's required to read the data back. (For what it's worth, failure to distinguish between data integrity meta-data relating to file extension and frippery like mtime seems to have been a problem on other popular OSs' fdatasync(2) implementations in the past, which I think is what leads Postgres to trust fdatasync(2) only when it knows there has been a full fsync(2) since the last file size change. It would be cool to be able to save on some I/Os be being less paranoid.)

sys/ufs/ufs/inode.h
139 ↗(On Diff #72451)

"\14b12" in this bitmap decoding thing should probably be "\14iblkdata".

Overall this looks like a correct change. As noted in my inline comment, I do not think that you should change the semantics of waitfor.

sys/ufs/ffs/ffs_vnops.c
420 ↗(On Diff #72451)

Instead of changing the waitfor option to ffs_update(), I suggest that you just put the conditional here instead of in ffs_update():

} else if ((ip->i_flags & IN_BLKDATA) != 0) {
        error = ffs_update(vp, 1);
kib marked 2 inline comments as done.

Test for IN_IBLKDATA in ffs_syncvnode().
Update bit-decode string.

This looks good to go.

This revision is now accepted and ready to land.Jun 4 2020, 4:01 AM