Change write_ringfile to create a temporary ring file, check and rename. #195

Merged
merged 2 commits into from Nov 17, 2012

2 participants

@jonmeredith

Mitigation for #190 - check the written ringfile before making it usable.

@slfritchie

Jon, how paranoid do you want to be? IIRC, the most paranoid wisdom is:

  1. write .tmp file data
  2. fsync(2) the .tmp file
  3. rename(2) the .tmp file
  4. fsync(2) the parent directory (not 100% certain on if timing of opening the parent dir's fd is important)

If we don't care 100% about the renaming operation happening, then omitting step 4 would be ok.

@jonmeredith

I can't see a way to persuade file:write_file to take an O_SYNC but that's probably a linuxism anyway. I was mostly trying to guard against out of space issues, but it's worth doing right.

I'll switch to an open/write/fsync/close.

Any suggestions on how to do step 4? file:sync/datasync take an IoDevice and

Eshell V5.9.1  (abort with ^G)
1> file:open(".",[read]).
{error,eisdir}

@slfritchie

Given that ring updates aren't really coordinated, and a race with a system crash would still leave the old ring file in place, I think number 4 is optional, especially since the Erlang file driver is picky about dealing with regular files {sigh}.

@slfritchie slfritchie was assigned Nov 16, 2012
@slfritchie

@jonmeredith I've added commit cc6edac. My intent is to use that same func (cut-and-paste, alas) for Bitcask's lock file creation, since we need the same thing there. Whaddya think?

@jonmeredith

Tests good for me +1 merge.

@slfritchie slfritchie merged commit e23dce9 into master Nov 17, 2012

1 check passed

Details default The Travis build passed
@seancribbs seancribbs deleted the jdm-safer-write-ringfile branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment