Make mv more atomic by trying rename before deleting dst
#55384
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vtjnash
reviewed
Aug 8, 2024
| function _mv_noreplace(src::AbstractString, dst::AbstractString) | ||
| # Error if dst exists. | ||
| # This check currently has TOCTTOU issues. | ||
| checkfor_mv_cp_cptree(src, dst, "moving"; force=false) |
Member
There was a problem hiding this comment.
I think we could add this as a flags argument to libuv uv_fs_rename, since many kernels seem to support it now (and it could be emulated with this TOCTOU in the event the kernel does not)
Member
Author
There was a problem hiding this comment.
I found a discussion about
adding this to rust at rust-lang/libs-team#131
Also, https://man7.org/linux/man-pages/man2/rename.2.html says "RENAME_NOREPLACE requires support from the underlying filesystem." but I don't understand how to check if a specific file system supports this, or exactly what happens if the filesystem doesn't support the flag.
Member
There was a problem hiding this comment.
Probably EINVAL:
EINVAL The filesystem does not support one of the flags in flags.
lazarusA
pushed a commit
to lazarusA/julia
that referenced
this pull request
Aug 17, 2024
…ng#55384) As noted in JuliaLang#41584 and https://discourse.julialang.org/t/safe-overwriting-of-files/117758/3 `mv` is usually expected to be "best effort atomic". Currently calling `mv` with `force=true` calls `checkfor_mv_cp_cptree(src, dst, "moving"; force=true)` before renaming. `checkfor_mv_cp_cptree` will delete `dst` if exists and isn't the same as `src`. If `dst` is an existing file and julia stops after deleting `dst` but before doing the rename, `dst` will be removed but will not be replaced with `src`. This PR changes `mv` with `force=true` to first try rename, and only delete `dst` if that fails. Assuming file system support and the first rename works, julia stopping will not lead to `dst` being removed without being replaced. This also replaces a stopgap solution from JuliaLang#36638 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As noted in #41584 and https://discourse.julialang.org/t/safe-overwriting-of-files/117758/3
mvis usually expected to be "best effort atomic".Currently calling
mvwithforce=truecallscheckfor_mv_cp_cptree(src, dst, "moving"; force=true)before renaming.checkfor_mv_cp_cptreewill deletedstif exists and isn't the same assrc.If
dstis an existing file and julia stops after deletingdstbut before doing the rename,dstwill be removed but will not be replaced withsrc.This PR changes
mvwithforce=trueto first try rename, and only deletedstif that fails. Assuming file system support and the first rename works, julia stopping will not lead todstbeing removed without being replaced.This also replaces a stopgap solution from #36638 (comment)