jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8264744: (fs) Use file cloning in Linux and macOS versions of FileChannel transfer and Files copy methods

Open bplb opened this issue 2 years ago • 14 comments

Add file cloning to java.nio.channels.FileChannel::transferTo and java.nio.file.Files.copy(Path,Path).

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Issue

  • JDK-8264744: (fs) Use file cloning in Linux and macOS versions of FileChannel transfer and Files copy methods

Reviewers

  • @mkarg (no known github.com user name / role) ⚠️ Review applies to d2a862c4

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9486/head:pull/9486
$ git checkout pull/9486

Update a local copy of the PR:
$ git checkout pull/9486
$ git pull https://git.openjdk.org/jdk pull/9486/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9486

View PR using the GUI difftool:
$ git pr show -t 9486

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9486.diff

bplb avatar Jul 13 '22 23:07 bplb

:wave: Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Jul 13 '22 23:07 bridgekeeper[bot]

On Linux, the copy_file_range(2) system call already includes file cloning if available, so there is no need to add an explicit call to ioctl(2) with request code FICLONE or FICLONERANGE unless copy_file_range() is unavailable. Therefore the Linux ioctl_ficlone branch in cloneFile0() is conditional, and an invocation of copy_file_range() is prepended in directCopy0() on Linux. Note that as FileChannel::transferFrom was already calling copy_file_range(), it was already using cloning on Linux when possible.

On macOS, the available cloning system calls are clonefile(2) and copyfile(3) with for the latter COPYFILE_CLONE set in flags. There is no cloning call which accepts file descriptors as the target cannot already exist when the system call is invoked. There is also no call on macOS to clone a range of a file, hence there is no cloning added for transferFrom() nor transferTo() on macOS.

The througput of Files::copy is greatly increased by using cloning, at the expense of course of the first subsequent write to the target path being slower due to copy-on-write (CoW).

                            throughput (ops/s)
                APFS (macOS)         BTRFS (Linux)         EXT4 (Linux)
count         before   after       before   after        before   after
10240        1578.971 2902.128   24331.624 31074.073   27722.815 27574.891
51200        1849.027 2917.996   19295.797 31313.13    18590.463 18650.098
102400       1757.527 2899.018   15874.133 31243.225   13242.939 13235.246
512000       1195.215 2818.547    5856.964 30972.607    3998.696  3903.092
1048568       829.433 2169.738    3283.841 31097.633    2056.693  2014.939
10485760      126.736 2336.032     236.58  30754.849    154.103    153.356
104857600      15.014 2361.208      22.352 31123.14      14.864     14.746
1073741824      2.484 2335.799       1.549 30682.197      1.408      1.41

The throughput within a CoW file system (APFS and BTRFS) is much higher, especially for large files, and that within a non-CoW file system (EXT4) is effectively unchanged.

As APFS supports CoW, existing tests should cover these changes. For Linux, a dynamic search for file systems supporting CoW is added in the CopyAndMove test.

It is unclear whether the Linux branch in cloneFile0 should attempt to use ioctl_ficlone if copy_file_range is undefined, as currently proposed, or simply always return IOS_UNAVAILABLE. It is likewise unclear whether ioctl_ficlonerange should be used in transferFrom0 and transferTo0 if copy_file_range is undefined (it currently is not).

In the initial version of this patch, cloning is enabled by default. In the issue description the possibility of adding a (JDK-specific?) CopyOption or system property to disable cloning was mentioned. There is also the opposite approach of cloning being disabled by default, with a CopyOption or system property required to enable it.

bplb avatar Jul 13 '22 23:07 bplb

@bplb The following label will be automatically applied to this pull request:

  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar Jul 13 '22 23:07 openjdk[bot]

Webrevs

mlbridge[bot] avatar Jul 14 '22 17:07 mlbridge[bot]

This is good work but it is putting a lot of platform specific code in UnixCopyFile. I think we'll need to some up front refactoring to support this so that the copy_file_range and clonefile implementation are Linux and macOS specific source files. This should allow more of the code to be written in Java and will allow most of the native code to go away.

AlanBateman avatar Jul 14 '22 17:07 AlanBateman

@bplb Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

openjdk-notifier[bot] avatar Jul 15 '22 02:07 openjdk-notifier[bot]

This version looks much better but I think it will need another round of two to get the code into the right place. As it stands, the change mean that some of the macOS specific file copy code is in UnixCopyFile and the reset is in BsdNativeDispatcher.

For macOS I suspect we'll end up with BsdCopyFile extending UnixCopyFile add adding support for fcopyfile and clonefile. Similarly LinuxCopyFile will extend UnixCopyFile and add support for copy_file_range.

A few specific issues on this version:

  • LinuxFileSystemProvider.clone leaks a file descriptor if creating the target file fails
  • LinuxFileSystemProvider.clone checks if the file exists but that check doesn't make sense here
  • LinuxNativeDispatcher.ioctl_ficlone uses Blocker but I would have expected that in the caller, not here
  • BsdFileSystemProvider.clone uses noFollowLinks. I think it would be clearer everywhere to invert that to followLinks. It may be better to map it to the flag at the Java level too.
  • I wonder if FICLONE is available at build time to avoid the hardcoded value (main concern is whether it has the same value on all Linux ports)

AlanBateman avatar Jul 15 '22 10:07 AlanBateman

I agree about having OS-specific CopyFile classes instead of OS-specific providers. I don’t like the IOStatus being in the provider and the call of provider.clone() seems odd.

  • LinuxNativeDispatcher.ioctl_ficlone uses Blocker emulating the calls in UnixNativeDispatcher.
  • FICLONE is the descendent of BTRFS_IOC_CLONE and has the same numerical value so I think it is fairly safe.

Agreed about the other points.

bplb avatar Jul 15 '22 14:07 bplb

I skimmed the latest patch and I think the main thing that still needs to be worked out is how to integtrate "clonefile". Right now the current prototype adds sun.nio.fs.CloneFile but that doesn't fit into the provider architecture. I think we are getting close to the point with these changes where UnixNativeDispatcher will need to be changed to define instance methods rather than static methods. I'll add comments soon with suggestions for how this could be integrated.

AlanBateman avatar Aug 08 '22 12:08 AlanBateman

I skimmed the latest patch and I think the main thing that still needs to be worked out is how to integtrate "clonefile". Right now the current prototype adds sun.nio.fs.CloneFile but that doesn't fit into the provider architecture. [...]

I don't like this approach that much either but is better than two prior versions I did not publish which respectively used a singleton version of UnixCopyFile and a FunctionalInterface for clonefile.

bplb avatar Aug 08 '22 16:08 bplb

@bplb this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout Files-copy-clone-8264744
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Aug 10 '22 01:08 openjdk[bot]

Thanks for the update. One thing that I should have pointed out in previous comments is UnixFileSystem.copyNonPosixAttributes. This is overridden in LinuxFileSystem and others and I think gives you an example of how to add the clone method without needing fileCopier. Would you be able to try that? I suspect the patch will reduce if that is used as the template.

AlanBateman avatar Aug 10 '22 15:08 AlanBateman

[...] I think gives you an example of how to add the clone method without needing fileCopier. Would you be able to try that? I suspect the patch will reduce if that is used as the template.

I'll take a look. If it improves things, then so much the better.

bplb avatar Aug 10 '22 15:08 bplb

clone() moved to {Bsd,Linux,Unix}FileSystem; {Bsd,Linux}CopyFile removed.

bplb avatar Aug 10 '22 17:08 bplb

I took another pass over the current patch. It has improved significantly but I think we will need to restructure UnixCopyFile very significantly to allow for both clone implementations. The main issue is that the macOS clonefile is forcing the patch to clone with file paths and this breaks the architecture where the source and target files are opened once.

One thing we could do is separate the update to FileChannelImpl.c to use copy_file_range to a separate issue. It doesn't need to be in this PR.

Another thing we could do is separate out the use of ioctl(FICLONE) on Linux. That is essentially an alternative transfer implementation so if we move the transfer to UnixFileSystem then the Linux implementation can use ioctl(FICLONE) or sendfile64, the macOS implementation can use fcopyfile, and the "portable" Unix implementation can use the user-space transfer loop. The implementation specific can fallback to the Unix transfer if needed. If we do this then it gives us more time to think about the troublesome macOS clonefile.

AlanBateman avatar Aug 17 '22 13:08 AlanBateman

The main issue is that the macOS clonefile is forcing the patch to clone with file paths and this breaks the architecture where the source and target files are opened once.

Yes, this is annoying.

One thing we could do is separate the update to FileChannelImpl.c to use copy_file_range to a separate issue. It doesn't need to be in this PR.

I agree with that.

Another thing we could do is separate out the use of ioctl(FICLONE) on Linux. [...]

This will require more thought.

bplb avatar Aug 17 '22 14:08 bplb

I've skimmed through ecce2093. There is a lot of refactoring (which is good) and it also introduces the use of clone on macOS. What would you think about separating this two? Let's get the Linux and BSD/macOS separation out of the way and that would allow us to discuss the semantics issue and agree when it is safe to use clone.

AlanBateman avatar Aug 30 '22 11:08 AlanBateman

What would you think about separating this two?

That would be fine with me, but I think the BSD / Linux separation should be done under a different issue with the clonefile work remaining under this one. The addition of copy_file_range function is also more pertinent to this issue than to refactoring however so I don't know under which that should go.

bplb avatar Aug 30 '22 14:08 bplb

That would be fine with me, but I think the BSD / Linux separation should be done under a different issue with the clonefile work remaining under this one.

That's okay, I think the main important point is that the refactoring is worthy on its own and if we get that queued up before the clonefile then it will make it easier to discuss clonefile.

AlanBateman avatar Aug 30 '22 15:08 AlanBateman

That would be fine with me, but I think the BSD / Linux separation should be done under a different issue with the clonefile work remaining under this one.

That's okay, I think the main important point is that the refactoring is worthy on its own [...].

Probably then I'll file two new issues: one for the refactoring and a sub-task for copy_file_range and change the topic of the current sub-task to clonefile. The sub-tasks will both be blocked by the refactoring.

bplb avatar Aug 30 '22 15:08 bplb

The updated implementation changes in 62461a2d look okay but I'm not sure about the updates to the CopyAndMove test. If I read it correctly, it tries to find the mount point where a COW file system is mounted and use it as an additional test directory. I don't think we should want tests trying to use these locations as test directories, even if running with permissions.

AlanBateman avatar Sep 03 '22 13:09 AlanBateman

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264744: (fs) Use file cloning in Linux version of Files::copy method

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 54 new commits pushed to the master branch:

  • 85d4b49151e9529051f1ed344749a487d3e92165: 8283929: GHA: Add RISC-V build config
  • 98d85e6f594bf34b97407c470b14791af0a2bc53: 8292579: (tz) Update Timezone Data to 2022c
  • 7c96608d9a2f3ea552b2daf1fe0772a0faac46c6: 8293403: JfrResolution::on_jvmci_resolution crashes when caller is null
  • c05015bc93916303ff1f16dec5d9391d2d773f41: 8291736: find_method_handle_intrinsic leaks Method*
  • b2067e63da116235740f5891f6218c9e1fd7b527: 8291725: Leftover marks when VM shutdown aborts bitmap clearing make mixed gc fail
  • 6a1e98cbf7369745409094e4b209602ac76f8ff3: 8293213: G1: Remove redundant assertion in G1RemSet::clean_card_before_refine
  • a92c1ff700925b400ee92057ae3dc3030487a886: 8287912: GTK L&F : Background of tree icons are red
  • 272745b374533b8ddec31df6ae88388049c19738: 8293340: Remove unused _code in {Zero,Template}InterpreterGenerator
  • 26f2a978f6c601e677a236054be4ddc39d9b6e55: 8290561: Coalesce incubator-module warnings for single-file source-code programs
  • 8e22f2bb403d71c103629f29996e78a54eafe0ad: 8293361: GHA: dump config.log in case of configure failure
  • ... and 44 more: https://git.openjdk.org/jdk/compare/0a4d0cee9ffd77eaa26493f20bac4bfaccd48c3b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Sep 06 '22 17:09 openjdk[bot]

/integrate

bplb avatar Sep 06 '22 18:09 bplb

Going to push as commit da596182a494a36d37030f18328e52e525fc3565. Since your change was applied there have been 54 commits pushed to the master branch:

  • 85d4b49151e9529051f1ed344749a487d3e92165: 8283929: GHA: Add RISC-V build config
  • 98d85e6f594bf34b97407c470b14791af0a2bc53: 8292579: (tz) Update Timezone Data to 2022c
  • 7c96608d9a2f3ea552b2daf1fe0772a0faac46c6: 8293403: JfrResolution::on_jvmci_resolution crashes when caller is null
  • c05015bc93916303ff1f16dec5d9391d2d773f41: 8291736: find_method_handle_intrinsic leaks Method*
  • b2067e63da116235740f5891f6218c9e1fd7b527: 8291725: Leftover marks when VM shutdown aborts bitmap clearing make mixed gc fail
  • 6a1e98cbf7369745409094e4b209602ac76f8ff3: 8293213: G1: Remove redundant assertion in G1RemSet::clean_card_before_refine
  • a92c1ff700925b400ee92057ae3dc3030487a886: 8287912: GTK L&F : Background of tree icons are red
  • 272745b374533b8ddec31df6ae88388049c19738: 8293340: Remove unused _code in {Zero,Template}InterpreterGenerator
  • 26f2a978f6c601e677a236054be4ddc39d9b6e55: 8290561: Coalesce incubator-module warnings for single-file source-code programs
  • 8e22f2bb403d71c103629f29996e78a54eafe0ad: 8293361: GHA: dump config.log in case of configure failure
  • ... and 44 more: https://git.openjdk.org/jdk/compare/0a4d0cee9ffd77eaa26493f20bac4bfaccd48c3b...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Sep 06 '22 18:09 openjdk[bot]

@bplb Pushed as commit da596182a494a36d37030f18328e52e525fc3565.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar Sep 06 '22 18:09 openjdk[bot]

Thank you! I'm the one who filed the original webbug report (some day I must figure out how to get direct access to the JDK bug tracker).

This wasn't a theoretical request, it was motivated by a real product that my company has just launched called Conveyor. Conveyor is a sort of jpackage++ and is structured as a parallel build system in which intermediate steps are cached. The different tasks sometimes have to copy large files around (like JDKs, JARs etc) and using kernel level COW cloning will yield a large boost to performance on filesystems that support it.

mikehearn avatar Sep 06 '22 20:09 mikehearn

Mailing list message from Brian Burkhalter on nio-dev:

On Sep 6, 2022, at 1:04 PM, Mike Hearn <duke at openjdk.org<mailto:duke at openjdk.org>> wrote:

Thank you! I'm the one who filed the original webbug report (some day I must figure out how to get direct access to the JDK bug tracker).

This wasn't a theoretical request, it was motivated by a real product that my company has just launched called [Conveyor](https://conveyor.hydraulic.dev/). Conveyor is a sort of jpackage++ and is structured as a parallel build system in which intermediate steps are cached. The different tasks sometimes have to copy large files around (like JDKs, JARs etc) and using kernel level COW cloning will yield a large boost to performance on filesystems that support it.

You?re welcome! We are pleased that you are happy with the change. If eventually you have any performance improvement to report, subjective or objective, please post it to nio-dev.

Thanks,

Brian -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/nio-dev/attachments/20220908/72d8b6be/attachment.htm>

mlbridge[bot] avatar Sep 09 '22 00:09 mlbridge[bot]