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

[libc++] use copy_file_range for fs::copy #109211

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Jannik2099
Copy link

Hi,

this optimizes std::filesystem::copy_file to use the copy_file_range syscall (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 cp program 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_RANGE cmake 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.

@Jannik2099 Jannik2099 requested a review from a team as a code owner September 18, 2024 22:36
Copy link

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 @ followed by their GitHub username.

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 llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 18, 2024

@llvm/pr-subscribers-libcxx

Author: Jannik Glückert (Jannik2099)

Changes

Hi,

this optimizes std::filesystem::copy_file to use the copy_file_range syscall (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 cp program 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_RANGE cmake 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.


Full diff: https://github.com/llvm/llvm-project/pull/109211.diff

2 Files Affected:

  • (modified) libcxx/src/CMakeLists.txt (+6)
  • (modified) libcxx/src/filesystem/operations.cpp (+59-1)
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 {

@mstorsjo
Copy link
Member

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).

CC @emaste

@Jannik2099 Note that the CI environment does contain a FreeBSD setup, which shows a failure (a somewhat boring one):

/home/buildkite/.buildkite-agent/builds/freebsd-test-1/llvm-project/libcxx-ci/libcxx/src/filesystem/operations.cpp:183:6: error: unused function 'copy_file_impl_copy_file_range' [-Werror,-Wunused-function]
  183 | bool copy_file_impl_copy_file_range(FileDescriptor& read_fd, FileDescriptor& write_fd, error_code& ec) {
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

(There are also a couple other similar failing cases in the same CI run on buildkite.)

@Jannik2099
Copy link
Author

Thanks!

Yeah, I forgot to slap on -Werror yesterday, will fix later.

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
Copy link
Author

So I looked at the FreeBSD failure again...

FreeBSD does have sendfile, but it does NOT have sys/sendfile.h, which means libc++ has been doing userspace copies on FreeBSD all this time!

I'll think of a more robust check.

@Jannik2099
Copy link
Author

FreeBSD does have sendfile, but it does NOT have sys/sendfile.h, which means libc++ has been doing userspace copies on FreeBSD all this time!

so turns out that was on purpose, because FreeBSD sendfile cannot send to files - I had assumed it was one of those "ancient unix syscalls that made it's way into every OS".

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 /proc). Funnily enough, we discovered the same deficiency in libstdc++ when I was adding copy_file_range support there.

@Jannik2099
Copy link
Author

Ok, I think this is ready for review now.

To summarize:

Add copy_file_range support on Linux and FreeBSD. fs::copy_file will first try copy_file_range, if unavailable and on linux sendfile, and finally the generic fstream.

This covers the following scenarios:

  • copy_file_range is not implemented by the filesystem or the OS
  • copy_file_range and/or sendfile are unable to copy to/from the source/dest in question
  • fs::copy_file is used on a special linux zero-length file, such as those in /proc - this was previously unsupported.

I have also added a test for the /proc case, there are no such files on FreeBSD to my rudimentary knowledge, so the test is linux exclusive.

I am not super happy with how copy_file_impl looks now - it's stuck halfway between doing #ifdef on (effectively) the target OS, and halfway between doing it on the available syscalls. Feel free to tear apart.

@ldionne
Copy link
Member

ldionne commented Sep 25, 2024

Please rebase onto main to re-trigger CI. The CI instability should be resolved now.

@ldionne
Copy link
Member

ldionne commented Sep 25, 2024

(I'll get to this review in my queue, for now just trying to get CI back on track)

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

Thanks for the patch and sorry for the delay! I have some comments and questions, but overall this patch looks quite good and this is definitely a nice optimization.

@Jannik2099
Copy link
Author

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 generic-no-localization case?

@Jannik2099
Copy link
Author

@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.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment. I've pushed to your branch to reorganize the way I wanted since it was only a minor deviation from what you did -- please LMK if what I did is not OK with you.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM once CI is green.

@Jannik2099
Copy link
Author

LGTM once CI is green.

NACK, see my review above

@ldionne
Copy link
Member

ldionne commented Dec 5, 2024

LGTM once CI is green.

NACK, see my review above

Sorry, I don't understand. Are you saying that something still needs to be addressed in this PR?

@Jannik2099
Copy link
Author

LGTM once CI is green.

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 copy_file_impl that attempt copy_file_range / sendfile first, and fstream second. The definition is chosen at compile time.

The intended behaviour is for one copy_file_impl that attempts copy_file_range (if available for the platform), then sendfile (if available for the platform), and finally fstream

@ldionne
Copy link
Member

ldionne commented Dec 5, 2024

Oh, geez, now I understand the intent. That's subtle. I assumed the copy-file-range implementation and the sendfile implementations were disjoint.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

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).

@Jannik2099
Copy link
Author

Sorry for the mixup

No worries!

LGTM

Jannik2099 and others added 4 commits December 10, 2024 21:57
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]>
@Jannik2099
Copy link
Author

Ugh, I forgot that Bionic does not provide a copy_file_range syscall wrapper. I guess checking for availability of the function at configure time like I did initially is the best option?

Also, there's still the issue that the no-locale build now fails due to use of fstream. Does libc++ have an internal locale-independent fstream I could use here? It'd be silly to drop fs::copy_file support from the no-locale build, but so is duplicating the old implementation which doesn't support /proc files

@ldionne
Copy link
Member

ldionne commented Dec 12, 2024

Ugh, I forgot that Bionic does not provide a copy_file_range syscall wrapper. I guess checking for availability of the function at configure time like I did initially is the best option?

Can't you just check for #ifdef __BIONIC__? I'd like to avoid introducing any CMake coupling just to check that property.

Also, there's still the issue that the no-locale build now fails due to use of fstream. Does libc++ have an internal locale-independent fstream I could use here? It'd be silly to drop fs::copy_file support from the no-locale build, but so is duplicating the old implementation which doesn't support /proc files

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 fstream available even when localization is disabled, so that problem would be solved then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants