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

Authored by kib on May 30 2020, 4:34 PM.

Details

Reviewers

Commits

rS361875: Clear the IN_SIZEMOD and IN_IBLKDATA flags only when doing a
rS361814: Further evaluation of the POSIX spec for fdatasync() shows that it
rS361801: Two additional places that need to identify IN_IBLKDATA.
rS361785: UFS: write inode block for fdatasync(2) if pointers in inode where allocated

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 Skipped

Unit

Tests Skipped

Build Status

Buildable 31477

Event Timeline

kib created this revision.May 30 2020, 4:34 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 30 2020, 4:34 PM

kib requested review of this revision.May 30 2020, 4:34 PM

Harbormaster completed remote builds in B31395: Diff 72451.May 30 2020, 4:34 PM

kib edited the summary of this revision. (Show Details)May 30 2020, 4:36 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.)

tmunro mentioned this in D25090: Add O_DSYNC support to open(2) and fcntl(2)..Jun 1 2020, 3:39 AM

tmunro added inline comments.Jun 1 2020, 11:36 AM

sys/ufs/ufs/inode.h
139	"\14b12" in this bitmap decoding thing should probably be "\14iblkdata".

tmunro mentioned this in D25071: aio_fsync(O_DSYNC).Jun 1 2020, 1:10 PM

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	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);