Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing fsync in atomic create #32

Open
photoszzt opened this issue Sep 7, 2019 · 2 comments
Open

missing fsync in atomic create #32

photoszzt opened this issue Sep 7, 2019 · 2 comments

Comments

@photoszzt
Copy link

I think this code is not atomic.

gg/src/util/path.cc

Lines 188 to 209 in febdf22

void atomic_create( const string & contents, const path & dst,
const bool set_mode, const mode_t target_mode )
{
string tmp_file_name;
{
UniqueFile tmp_file { dst.string() };
tmp_file_name = tmp_file.name();
if ( contents.size() > 0 ) {
tmp_file.fd().write( contents );
}
if ( set_mode ) {
CheckSystemCall( "fchmod", fchmod( tmp_file.fd().fd_num(), target_mode ) );
}
/* allow block to end so the UniqueFile gets closed() before rename. */
/* not 100% sure readers will see fully-written file appear atomically otherwise */
}
rename( tmp_file_name, dst.string() );
}

Two fsync is missing. You need to fsync the temp file and fsync the containing directory after the rename. See: https://lwn.net/Articles/457667/

@JustinAzoff
Copy link

I don't thing consistency in the face of a kernel panic or power failure is a requirement based on how that code is used. Adding 2 fsyncs to every file creation would just slow builds down.

@photoszzt
Copy link
Author

@JustinAzoff The problem is if I don't put these two fsyncs, the program fails. It complaints that the file is missing when the file is created but the directory is not fsync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants