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

Implementation of durable file rename #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions unliftio/ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog for unliftio

## 0.2.14

* Add `UnliftIO.IO.File.renameFileDurable`

## 0.2.13

* Add `UnliftIO.STM.orElse`
Expand Down
2 changes: 1 addition & 1 deletion unliftio/package.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: unliftio
version: 0.2.12.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snoyberg FYI Looks like the 0.2.13 went missing.

version: 0.2.14
synopsis: The MonadUnliftIO typeclass for unlifting monads to IO (batteries included)
description: Please see the documentation and README at <https://www.stackage.org/package/unliftio>
homepage: https://github.com/fpco/unliftio/tree/master/unliftio#readme
Expand Down
26 changes: 26 additions & 0 deletions unliftio/src/UnliftIO/IO/File.hs
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,14 @@ module UnliftIO.IO.File
, withBinaryFileDurable
, withBinaryFileDurableAtomic
, ensureFileDurable
, renameFileDurable
)
where

import Data.ByteString as B (ByteString, writeFile)
import Control.Monad.IO.Unlift
import UnliftIO.IO (Handle, IOMode(..), withBinaryFile)
import UnliftIO.Directory (renameFile)

#if WINDOWS

Expand All @@ -102,6 +104,8 @@ withBinaryFileDurable = withBinaryFile
withBinaryFileDurableAtomic = withBinaryFile
withBinaryFileAtomic = withBinaryFile

renameFileDurable = renameFile

#else

import qualified Data.ByteString as B (hPut)
Expand All @@ -119,8 +123,30 @@ writeBinaryFileAtomic fp bytes =
withBinaryFileDurable = Posix.withBinaryFileDurable
withBinaryFileDurableAtomic = Posix.withBinaryFileDurableAtomic
withBinaryFileAtomic = Posix.withBinaryFileAtomic

renameFileDurable = Posix.renameFileDurable
#endif

-- | When a file is renamed, it is necessary to execute @fsync()@ on the
-- directory that contains the file now and afterwards on the directory where
-- the file was before, so that the rename is durable.
--
-- Remark: This is also atomic if both locations of the file are on the same
-- filesystem. However, it could happen that the operation leads to data loss,
-- if a crash happens after the rename and before the first fsync finishes. This
-- 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

--
-- === Cross-Platform support
--
-- This function is a noop on Windows platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include a @since, as well as a changelog entry and minor version bump. Also: @nh2 or @lehins, could one of you take a look?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, added those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could one of you take a look?

Will do!

-- @since 0.2.14
renameFileDurable :: MonadIO m => FilePath -> FilePath -> m ()
-- Implementation is at the top of the module

-- | After a file is closed, this function opens it again and executes @fsync()@
-- internally on both the file and the directory that contains it. Note that this function
-- is intended to work around the non-durability of existing file APIs, as opposed to
Expand Down
12 changes: 12 additions & 0 deletions unliftio/src/UnliftIO/IO/File/Posix.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module UnliftIO.IO.File.Posix
, withBinaryFileDurableAtomic
, withBinaryFileAtomic
, ensureFileDurable
, renameFileDurable
)
where

Expand Down Expand Up @@ -39,6 +40,7 @@ import System.IO.Error (ioeGetErrorType, isAlreadyExistsError,
import qualified System.Posix.Files as Posix
import System.Posix.Internals (CFilePath, c_close, c_safe_open, withFilePath)
import System.Posix.Types (CMode(..), Fd(..), FileMode)
import UnliftIO.Directory (renameFile)
import UnliftIO.Exception
import UnliftIO.IO
import UnliftIO.MVar
Expand Down Expand Up @@ -580,3 +582,13 @@ withBinaryFileAtomic filePath iomode action =
liftIO $ atomicTempFileRename Nothing mFileMode eTmpFile filePath
pure res


-- | See `renameFileDurable`
renameFileDurable ::
MonadIO m => FilePath -> FilePath -> m ()
renameFileDurable oldName newName =
liftIO $ withDirectory (takeDirectory oldName) $ \oldDirFd ->
withDirectory (takeDirectory newName) $ \newDirFd -> do
renameFile oldName newName
fsyncDirectoryFd "renameFileDurable" newDirFd
fsyncDirectoryFd "renameFileDurable" oldDirFd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible optimisation:

For the common case that oldName and newName are in the same directory, do only 1 withDirectory and do only 1 fsync().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.