[libc++] use copy_file_range for fs::copy #109211
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Jannik Glückert (Jannik2099) ChangesHi, this optimizes This behaviour is in line with Boost.Filesystem and libstdc++ (where I implemented this some time ago). It's also the default behaviour of the standard file copy operations in .NET Core, Java, and (hopefully) soon python, and in the I don't have the opportunity to test this on FreeBSD, but I think the FreeBSD folks will want a look at this. Sadly I don't know any by name, perhaps someone could CC the relevant person(s). As for the Lastly I am not sure if the largefile handling (see usage of Full diff: https://github.com/llvm/llvm-project/pull/109211.diff 2 Files Affected:
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index 48c5111a0acbf6..3f97d3e730a42c 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -173,6 +173,12 @@ if (APPLE AND LLVM_USE_SANITIZER)
endif()
endif()
+include(CheckCXXSymbolExists)
+check_cxx_symbol_exists("copy_file_range" "unistd.h" LIBCXX_USE_COPY_FILE_RANGE)
+if(LIBCXX_USE_COPY_FILE_RANGE)
+ list(APPEND LIBCXX_COMPILE_FLAGS "-D_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE")
+endif()
+
split_list(LIBCXX_COMPILE_FLAGS)
split_list(LIBCXX_LINK_FLAGS)
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index d771f200973528..42d16c0546ae10 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -32,6 +32,7 @@
# include <dirent.h>
# include <sys/stat.h>
# include <sys/statvfs.h>
+# include <sys/types.h>
# include <unistd.h>
#endif
#include <fcntl.h> /* values for fchmodat */
@@ -178,8 +179,35 @@ void __copy(const path& from, const path& to, copy_options options, error_code*
namespace detail {
namespace {
+#if defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE)
+bool copy_file_impl_copy_file_range(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
+ size_t count = read_fd.get_stat().st_size;
+ // a zero-length file is either empty, or not copyable by this syscall
+ // return early to avoid the syscall cost
+ if (count == 0) {
+ ec = {EINVAL, generic_category()};
+ return false;
+ }
+ do {
+ ssize_t res;
+ // do not modify the fd positions as copy_file_impl_sendfile may be called after a partial copy
+ off_t off_in = 0;
+ off_t off_out = 0;
+
+ if ((res = ::copy_file_range(read_fd.fd, &off_in, write_fd.fd, &off_out, count, 0)) == -1) {
+ ec = capture_errno();
+ return false;
+ }
+ count -= res;
+ } while (count > 0);
+
+ ec.clear();
+
+ return true;
+}
+#endif
#if defined(_LIBCPP_FILESYSTEM_USE_SENDFILE)
-bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
+bool copy_file_impl_sendfile(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
size_t count = read_fd.get_stat().st_size;
do {
ssize_t res;
@@ -194,6 +222,36 @@ bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_cod
return true;
}
+#endif
+
+#if defined(_LIBCPP_FILESYSTEM_USE_SENDFILE)
+bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
+ bool has_copied = false;
+# if defined(_LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE)
+ has_copied = copy_file_impl_copy_file_range(read_fd, write_fd, ec);
+ if (has_copied) {
+ return true;
+ }
+ // EINVAL: src and dst are the same file (this is not cheaply
+ // detectable from userspace)
+ // EINVAL: copy_file_range is unsupported for this file type by the
+ // underlying filesystem
+ // ENOTSUP: undocumented, can arise with old kernels and NFS
+ // EOPNOTSUPP: filesystem does not implement copy_file_range
+ // ETXTBSY: src or dst is an active swapfile (nonsensical, but allowed
+ // with normal copying)
+ // EXDEV: src and dst are on different filesystems that do not support
+ // cross-fs copy_file_range
+ // ENOENT: undocumented, can arise with CIFS
+ // ENOSYS: unsupported by kernel or blocked by seccomp
+ if (ec.value() != EINVAL && ec.value() != ENOTSUP && ec.value() != EOPNOTSUPP && ec.value() != ETXTBSY &&
+ ec.value() != EXDEV && ec.value() != ENOENT && ec.value() != ENOSYS) {
+ return false;
+ }
+ ec.clear();
+# endif
+ return copy_file_impl_sendfile(read_fd, write_fd, ec);
+}
#elif defined(_LIBCPP_FILESYSTEM_USE_COPYFILE)
bool copy_file_impl(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
struct CopyFileState {
|
CC @emaste @Jannik2099 Note that the CI environment does contain a FreeBSD setup, which shows a failure (a somewhat boring one): (There are also a couple other similar failing cases in the same CI run on buildkite.) |
|
Thanks! Yeah, I forgot to slap on Another note, how TOCTOU-safe should this be? Right now I fstat both in the copy_file_range and in the sendfile impl. If the file size changes inbetween and copy_file_range fails halfway through, this may lead to inconsistent output. |
Jannik2099
force-pushed
the
copy_file_range
branch
from
4e7d8b9 to
a5dc5bf
Compare
|
So I looked at the FreeBSD failure again... FreeBSD does have I'll think of a more robust check. |
so turns out that was on purpose, because FreeBSD I'll have to restructure this a bit then. Unrelated, I also took the opportunity to support copying special zero-length files on linux (such as those in |
Jannik2099
force-pushed
the
copy_file_range
branch
from
a624e29 to
3bc0f7c
Compare
|
Ok, I think this is ready for review now. To summarize: Add This covers the following scenarios:
I have also added a test for the I am not super happy with how |
|
Please rebase onto main to re-trigger CI. The CI instability should be resolved now. |
|
(I'll get to this review in my queue, for now just trying to get CI back on track) |
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Show resolved
Hide resolved
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Outdated
Show resolved
Hide resolved
Jannik2099
force-pushed
the
copy_file_range
branch
from
0f3176c to
bdf6774
Compare
|
The CI failure in https://github.com/llvm/llvm-project/actions/runs/11822050795/job/32940244678?pr=109211 got me a tad confused. Is some special magic required to get iostreams working in the |
|
@ldionne ping I think all the remaining CI failures are unrelated, except for the one mentioned above. Though I don't know how to fix this. |
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.copy_file/copy_file_procfs.pass.cpp
Show resolved
Hide resolved
NACK, see my review above |
Sorry, I don't understand. Are you saying that something still needs to be addressed in this PR? |
I am saying that your commit 8f314b9 completely changes the semantics. You changed it to two definitions of The intended behaviour is for one |
|
Oh, geez, now I understand the intent. That's subtle. I assumed the copy-file-range implementation and the sendfile implementations were disjoint. |
There was a problem hiding this comment.
I just pushed something that should fix my mistake.
Can you double-check and confirm that this does what you originally intended? I think we're pretty much back to your patch without my edits, except I added a comment.
If that patch is fine with you, it's fine with me in the current state. Sorry for the mixup, I was trying to help expedite this patch by doing what I thought were NFC changes (but were not).
No worries! LGTM |
opportunistically use copy_file_range (Linux, FreeBSD) where possible. This allows for fast copies via reflinks, and server side copies for network filesystems. Fall back to sendfile if not supported. Signed-off-by: Jannik Glückert <[email protected]>
Virtual linux filesystems such as /proc and /sys may contain files that appear as zero length, but do contain data. These require a traditional userspace read + write loop. Signed-off-by: Jannik Glückert <[email protected]>
…E_SENDFILE disjoint
Jannik2099
force-pushed
the
copy_file_range
branch
from
74b1b09 to
79185fd
Compare
|
Ugh, I forgot that Bionic does not provide a Also, there's still the issue that the no-locale build now fails due to use of |
Can't you just check for
No, we don't have such a thing. Instead, I'd disable it for the no-localization build for now. I'm working on changes that should make |
Signed-off-by: Jannik Glückert <[email protected]>
Jannik2099
force-pushed
the
copy_file_range
branch
from
dc01a05 to
55fe8ca
Compare
|
Thanks for the patch! |
|
@Jannik2099 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
We have this error now |
|
Looks like on linux this code must compile with _FILE_OFFSET_BITS 64 |
largefile handling isn't that simple on linux sadly, especially once multiple libcs are involved. I've changed it to just use the linux-specific |
|
We got the following build error for the cross builds (win to armv7 linux) https://lab.llvm.org/buildbot/#/builders/38/builds/1661 |
|
|
The is #121855 for that |
PR llvm#109211 introduced a build break on systems with glibc < 2.27, since copy_file_range was only introduced after that version. A version check is added to prevent this breakage.
|
We got another error related to this on the sanitizer buildbot (https://lab.llvm.org/buildbot/#/builders/66/builds/8314) Seems like this doesn't build on 32bit |
sorry, this is also fixed in the pending #121855 |
Fix for issues reported in #109211
|
I merged #121855, thanks @Jannik2099 for the quick fix! |
PR #109211 introduced a build break on systems with glibc < 2.27, since copy_file_range was only introduced after that version. A version check is added to prevent this breakage.
Fix for issues reported in llvm/llvm-project#109211
Hi,
this optimizes
std::filesystem::copy_fileto use thecopy_file_rangesyscall (Linux and FreeBSD) when available. It allows for reflinks on filesystems such as btrfs, zfs and xfs, and server-side copy for network filesystems such as NFS.This behaviour is in line with Boost.Filesystem and libstdc++ (where I implemented this some time ago). It's also the default behaviour of the standard file copy operations in .NET Core, Java, and (hopefully) soon python, and in the
cpprogram from GNU coreutils.I don't have the opportunity to test this on FreeBSD, but I think the FreeBSD folks will want a look at this. Sadly I don't know any by name, perhaps someone could CC the relevant person(s).
As for the
LIBCXX_USE_COPY_FILE_RANGEcmake check, I was unsure where to put it, and this is probably not the desired location.Lastly I am not sure if the largefile handling (see usage of
off_t) is correct.