pwd_mkdb opens files with O_SYNC instead of calling fsync
ClosedPublic
Actions

Authored by dwmalone on Feb 4 2016, 9:53 AM.

Details

Reviewers

bapt
garga
gnn
vangyzen

Commits

rS295925: Following revision r295924, the changes to a db file should be fsynced
rS295924: If we close or sync a hash-based db file, make sure to call fsync to

Summary

The right fix for this shouldn't be the use of O_SYNC, but calling
fsync before the files are moved into place (plus calling fsync on
the directory, which is a good idea).

In fact, pwd_mkdb calls the close method on the db, which the man
page for dbopen claims should flush the file. However, looking at
the sync and close methods in lib/libc/db/hash, it seems the code
for flushing is missing, which is probably the real bug here.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dwmalone updated this revision to Diff 12999.Feb 4 2016, 9:53 AM

dwmalone retitled this revision from to pwd_mkdb opens files with O_SYNC instead of calling fsync.

dwmalone updated this object.

dwmalone edited the test plan for this revision. (Show Details)

dwmalone added reviewers: garga, gnn, bapt.

I had assumed that buffers are flushed before the file is closed (there is a comment to that effect in hash.c:hdestroy() but I have not looked at this code in detail).

In my tests, the db file came out identical with or without O_SYNC.

BTW: There is one further command that uses dbopen(..., O_SYNC) and can be sped up by a significant factor (25.5 seconds --> 0.6 seconds in my tests with the full termcap file) and that command is cap_mkdb.

ARC does not like me and since its a trivial change I just point you at src/usr.bin/cap_mkdb/cap_mkdb.c line 122. Please update the diff to include the removal of O_SYNC from that file to.

looks ok to me, and from my testing it does the job. @garga can you confirm that is also fixes the issue you found?

vangyzen accepted this revision.Feb 11 2016, 5:27 PM

vangyzen added a reviewer: vangyzen.

vangyzen added a subscriber: vangyzen.

vangyzen added inline comments.