Conversation
| -- === Cross-Platform support | ||
| -- | ||
| -- This function is a noop on Windows platforms. | ||
| -- |
There was a problem hiding this comment.
Thanks for the feedback, added those.
There was a problem hiding this comment.
could one of you take a look?
Will do!
| -- | ||
| -- === Cross-Platform support | ||
| -- | ||
| -- This function is a noop on Windows platforms. |
There was a problem hiding this comment.
I think this text needs to be adjusted: On ensureFileDurable it's right because there it does nothing, but for renameFileDurable it certainly still does the rename, it just doens't make it durable.
| -- is because on an async filesystem the write of the old directory might | ||
| -- already written to disk and the change on the new directory is not. It the | ||
| -- function call returns, the change is durable. Nevertheless, this will not | ||
| -- happen on filesystems using journaling, that is, allmost all modern filesystems. |
There was a problem hiding this comment.
Is it not a bit confusing to say "This is also atomic"? Because it should be only atomic on the same filesystem -- rename() does not work at all across different file systems (on Linux), where we get from System.Directory.renameFile:
Prelude System.Directory> renameFile "testfile" "mnt/testfile"
*** Exception: testfile: renameFile:renamePath:rename: unsupported operation (Invalid cross-device link)
| @@ -1,5 +1,5 @@ | |||
| name: unliftio | |||
| version: 0.2.12.1 | |||
| withDirectory (takeDirectory newName) $ \newDirFd -> do | ||
| renameFile oldName newName | ||
| fsyncDirectoryFd "renameFileDurable" newDirFd | ||
| fsyncDirectoryFd "renameFileDurable" oldDirFd |
There was a problem hiding this comment.
Possible optimisation:
For the common case that oldName and newName are in the same directory, do only 1 withDirectory and do only 1 fsync().
| withDirectory (takeDirectory newName) $ \newDirFd -> do | ||
| renameFile oldName newName | ||
| fsyncDirectoryFd "renameFileDurable" newDirFd | ||
| fsyncDirectoryFd "renameFileDurable" oldDirFd |
There was a problem hiding this comment.
Another possible improvement could be to use renameat(), with is directory-FD based renaming.
That would be even better than what we have here, because then the fsync() would operate on the same FD as the rename does, giving even better guarantees (see also https://stackoverflow.com/questions/37288453/calling-fsync2-after-close2/50158433#50158433 for a related topic on files).
That is not necessary for this PR though, having a renameFileDurable implemented this way is already much better than not having it.
lehins
left a comment
lehins
left a comment
There was a problem hiding this comment.
I am sceptical that a simple renameFile will be enough for durability.
In order for the fsync to work in order to guarantee durability on the actual file change we need to open that file with openat in the directory and then do the atomic renameat followed by an fsync on the file AND the containing directory Fd. In the case of rename we don't need to open the file, but renameat might still be necessary. I didn't find anything online about it, so I might be wrong and a simple renameFile followed by fsync on directories might be sufficient.
Another side note is that renameFile only works on files and because of that performs extra checks that the file path is not a directory, which are themselves not atomic.
Just to be safe what I recommend is adding an actual renameFilePathAtomicDurable (renames are guaranteed to be atomic, since they aren't possible across devices, as @nh2 noted in his comment: https://github.com/fpco/unliftio/pull/60/files#r419109086) That function will work on both files and directories alike. I started on this approach here: lehins@d98f605
If @nh2, @snoyberg and @clkamp you guys agree that this is the right approach @clkamp feel free to pick up from that commit and merge it into you work here.
Here is a pretty useful SO question on the topic: https://stackoverflow.com/questions/3764822/how-to-durably-rename-a-file-in-posix
I also agree with all of the comments @nh2 made and want to add:
- I'd appreciate if formatting and indentation was kept consistent with the rest of the code
- At least a couple of tests added to the test suite
4 participants
For some use cases, one does not only need atomic and or durable writing of files, but also durable rename of files. A standard use case for this is delivery of mails to a Maildir (especially on a unix that is not linux, so that anonymous temp files are not available), where a new mail is first delivered to a temporary directory and then renamed to its final location.
This pull request implements a durable renaming of files.