julia icon indicating copy to clipboard operation
julia copied to clipboard

use uv_fs_copyfile for cp

Open stevengj opened this issue 7 months ago • 17 comments

This PR changes cp, and other uses of the internal function Base.sendfile(srcpath, destpath), to use the uv_fs_copyfile function.

This should be faster and more reliable, hopefully — on many filesystems, uv_fs_copyfile can just create a copy-on-write link. Hopefully, this should fix longstanding problems with cp of large files: fixes #56537 (macos), fixes #39868 (linux), fixes #30723.

In particular, I noticed two problems with our previous sendfile implementation, which called a lower-level sendfile function on the file descriptors that in turn called uv_fs_sendfile. First, uv_fs_sendfile takes a Csize_t argument for the number of bytes, which is clearly too small on 32-bit systems. Second, it assumes that the return value of uv_fs_sendfile is the number of bytes written, but the return value is a Cint, which is clearly too small for the number of bytes in a large file (> 2GiB). The PR therefore fixed the uv_fs_sendfile call to pass at most typemax(Cssize_t) (which is the maximum value on a 32-bit Unix system, since the underlying libc sendfile returns an ssize_t) in a single call (writing larger files in chunks), and second to only use the return value as an error code — if uv_fs_sendfile succeeds, it looks like we can assume that it wrote the requested number of bytes.

In principle, we could delete this lower-level Base.sendfile method completely, since it is undocumented and we don't use it. I couldn't find any external packages that use it either. But I left it in for now, to be conservative.

I'm still seeing a test failure with this PR related to file mode: on my macos system, it seems to be ignoring the umask for the permissions of the copied file? I'm not sure why — libuv's copyfile implementation on Unix should call open with the mode of the source file, which should mask out the umask, no? See also #27295.

cc @vtjnash, who suggested using uv_fs_copyfile in https://github.com/JuliaLang/julia/pull/27295#pullrequestreview-123846212.

stevengj avatar Sep 26 '25 08:09 stevengj

The failing tests are due to the mode not respecting umask, as noted above. @vtjnash, any advice on why libuv might be ignoring the umask, here?

stevengj avatar Sep 26 '25 13:09 stevengj

It is implemented to copy permissions, not the umask: https://github.com/libuv/libuv/pull/4396

vtjnash avatar Sep 26 '25 15:09 vtjnash

So should our cp do the same? Or should I do an explicit chown and chmod with the umask after copying?

Or should I submit a PR to libuv to add a flag to have copyfile to not copy ownership and permissions? (But it seems like this could introduce an inconsistency with Windows, according to the issue linked below.)

@vtjnash filed https://github.com/libuv/libuv/issues/3125 requesting that libuv copy over the permissions and ownership to match cp, so I'm guessing that you would advocate that we change our behavior as well? But the umask treatment in #27295 was clearly intentional by @staticfloat at the suggestion of @KristofferC; would this be considered a breaking change?

stevengj avatar Sep 27 '25 00:09 stevengj

IIRC, we don't care, but want to ensure it is consistent across platforms whichever way it goes so it isn't a weird gotcha when testing different platforms

vtjnash avatar Sep 28 '25 00:09 vtjnash

IMO we should generally follow whatever coreutils does, so that our cp() is similar to their cp.

staticfloat avatar Sep 28 '25 00:09 staticfloat

IMO we should generally follow whatever coreutils does, so that our cp() is similar to their cp.

uv_fs_copyfile seems similar to coreutils cp --preserve.

Our existing behavior of applying the umask is similar to the default cp behavior in applying umask, but it differs in other ways; in particular, if the destination file already exists then cp preserves its existing permissions.

In the absence of [the --preserve] option, the permissions of existing destination files are unchanged. Each new file is created with the mode of the corresponding source file minus the set-user-ID, set-group-ID, and sticky bits as the create mode; the operating system then applies either the umask or a default ACL, possibly resulting in a more restrictive file mode.

whereas I believe we do not do this (in force=true mode we rm the file before copying).

stevengj avatar Sep 28 '25 06:09 stevengj

I think it's fine to be similar to cp --preserve, as long as we document that we're going to try and preserve permissions, that seems like a totally reasonable default.

staticfloat avatar Sep 30 '25 03:09 staticfloat

Okay, I've changed the docstring and test to document that it acts like cp -p, at least for files (ignoring the question of directories for now).

This doesn't seem like a breaking change since the previous behavior was undocumented.

stevengj avatar Sep 30 '25 05:09 stevengj

CI failure seems to be an unrelated Error in testset precompile. Update: now CI is green.

stevengj avatar Oct 01 '25 01:10 stevengj

hi all, this worked for me.

floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> ./julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.13.0-DEV.1204 (2025-09-30)
 _/ |\__'_|_|_|\__'_|  |  sgj/uv_fs_copyfile/fc4d77672f (fork: 4 commits, 33 days)
|__/                   |

julia> versioninfo()
Julia Version 1.13.0-DEV.1204
Commit fc4d77672f (2025-09-30 05:27 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin24.6.0)
  CPU: 10 × Apple M1 Pro
  WORD_SIZE: 64
  LLVM: libLLVM-20.1.8 (ORCJIT, apple-m1)
  GC: Built with stock GC
Threads: 1 default, 1 interactive, 1 GC (on 8 virtual cores)



julia> cp("/Users/floswald/Dropbox (Personal)/Apps/JPE-packages/JPE/Green-20220767/2/replication-package/Ben Sand - Union_Rep_Package.zip", "/Users/floswald/Downloads/test.zip")
"/Users/floswald/Downloads/test.zip"

julia> exit()
floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> julia
The latest version of Julia in the `release` channel is 1.12.1+0.aarch64.apple.darwin14. You currently have `1.11.4+0.aarch64.apple.darwin14` installed. Run:

  juliaup update

to install Julia 1.12.1+0.aarch64.apple.darwin14 and update the `release` channel to that version.
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.11.4 (2025-03-10)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |


julia> cp("/Users/floswald/Dropbox (Personal)/Apps/JPE-packages/JPE/Green-20220767/2/replication-package/Ben Sand - Union_Rep_Package.zip", "/Users/floswald/Downloads/test.zip", force = true)
ERROR: IOError: sendfile: Unknown system error -184335295 (Unknown system error -184335295)
Stacktrace:
 [1] uv_error
   @ ./libuv.jl:106 [inlined]
 [2] sendfile(dst::Base.Filesystem.File, src::Base.Filesystem.File, src_offset::Int64, bytes::Int64)
   @ Base.Filesystem ./filesystem.jl:224
 [3] sendfile(src::String, dst::String)
   @ Base.Filesystem ./file.jl:1131
 [4] cp(src::String, dst::String; force::Bool, follow_symlinks::Bool)
   @ Base.Filesystem ./file.jl:386
 [5] top-level scope
   @ REPL[2]:1

julia> 

floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> du -h /Users/floswald/Downloads/test.zip 
3.8G	/Users/floswald/Downloads/test.zip

floswald avatar Oct 28 '25 08:10 floswald

CI failure looks unrelated:

Error in testset REPL:
Error During Test at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZQ07RJYVY.0/build/default-grannysmith-C07ZQ07RJYVY-0/julialang/julia-master/julia-4aa2ef83b2/share/julia/stdlib/v1.14/REPL/test/repl.jl:1862
  Got exception outside of a @test
  failed process: Process(`/Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZQ07RJYVY.0/build/default-grannysmith-C07ZQ07RJYVY-0/julialang/julia-master/julia-4aa2ef83b2/bin/julia --startup-file=no -e 'using REPL; print(REPL.Pkg_promptf())'`, ProcessExited(1)) [1]

stevengj avatar Nov 23 '25 01:11 stevengj

We discussed this on triage today. My primary point was that I'm not a fan of cp semantics in general and if we're changing the semantics anyway, we should maybe think about fixing that as well. To me, cp is in a family with mv and rm in that it (in my mental model) operates on file entries as opposed to file contents. In particular, I find it surprising that cp changes the contents of any hardlink'ed files rather than replacing the file entry. I also don't like that any concurrent readers can see partial file. I know that GNU coreutils behaves that way unless you pass --remove-destination (equivalently -f in BSD, but not GNU).

It would be my strong preference that our cp implementation default to performing an atomic, attribute-preserving, copy at the file entry level (by creating a new file/dir tree at a temporary name in the directory and them swapping or moving it into place). I think that is the sanest semantics for it to have. I think we could still keep the sendfile function that overwrites the destination file if somebody wants those semantics, but I don't know that they should be the default.

Keno avatar Dec 05 '25 04:12 Keno

@Keno, there may be a misunderstanding here — we currently already do rm an existing destination file (or directory) in force=true mode before calling either uv_fs_copyfile (or sendfile, in the old version): see https://github.com/JuliaLang/julia/blob/2a920801860621fcfdd4c672c7533592aa06bba3/base/file.jl#L358

The only difference with what you are suggesting is that it does not do this atomically, by copying to a temporary file and calling something like _mv_replace (which calls rename if possible), though even _mv_replace is not atomic in all cases (and directories seem harder).

I agree that atomic cp is worth aspiring to, but it doesn't sound like we can guarantee it in all cases? And if we can't guarantee it, it doesn't seem like we need to document (or implement) it now. It will be something that can be improved later, no?

So is the only change required for this PR that we additionally document (the current behavior) that force=true removes any existing destination rather than overwriting it?

stevengj avatar Dec 05 '25 17:12 stevengj

@Keno's thought was that if we're already changing cp semantics (slightly), it's worth seeing if we can make a second minor semantic change to be atomic. If this turns out to be difficult (which I think it likely does), IMO we should just go with this PR basically as is.

oscardssmith avatar Dec 06 '25 03:12 oscardssmith

@Keno, there may be a misunderstanding here — we currently already do rm an existing destination file (or directory) in force=true mode before calling either uv_fs_copyfile (or sendfile, in the old version): see

That was indeed not clear to me. Somebody on the call said we had GNU semantics, but I may have misunderstood. But if we already have BSD semantics, then I agree the atomicity is the only issue, so that needn't hold up this PR since we can always strengthen the semantics later.

and directories seem harder

We discussed this on the call and concluded that we should be able to use RENAME_EXCHANGE on Linux. MacOS has RENAME_SWAP. Windows can do it as well. FreeBSD doesn't seem to have it though.

Keno avatar Dec 06 '25 05:12 Keno

Note also that the fact that force=true first removes any destination file was already documented before this PR. The docstring says:

Copy the file, link, or directory from src to dst. force=true will first remove an existing dst.

stevengj avatar Dec 07 '25 12:12 stevengj

I said would I write my thoughts here after triage then got sidetracked hunting down libuv bugs. Woops.

I agree with @oscardssmith that fixing cp on large files is important enough that we should merge this as soon as possible, even if I'd prefer to emulate the behaviour of POSIX cp for permissions.

Strengthening cp to make it atomic in some situations would be the wrong choice if we can't actually make a precise guarantee. I'd always prefer to have the standard library provide the highest performance primitives that can compose; copy-and-swap would both struggle to provide such a guarantee and would be useless for implementing a protocols on top of it:

  • On Windows, I still don't know how we'd implement this. Could @keno clarify? (BSD was already brought up as a platform where we can't do this).
  • I mentioned on the triage call that even if we were to call cp on directories "atomic", it would be impractically slow to make it atomic in the presence of crashes. IMO when it comes to deleting files it is confusing for users to provide atomicity with respect to other processes but allow situations where we lose data in a crash.
  • Given how many users hit problems with cp on very large files, we shouldn't force them to create a third file in order to overwrite a very large existing file.

The design of cp as-is already has a ton of issues as a result of trying to emulate coreutils:

  • All of our isdir/isfile/islink uses, especially in checkfor_mv_cp_tree have TOCTOU problems. We can overwrite existing files even force=false.
  • follow_symlinks explodes on cycles (coreutils does cycle finding and bails out).
  • cptree would need to use dirfds on Unix to have any kind of reasonable atomicity.

xal-0 avatar Dec 08 '25 18:12 xal-0