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

gh-81340: Use copy_file_range in shutil.copyfile copy functions #93152

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

Conversation

illia-v
Copy link
Contributor

@illia-v illia-v commented May 23, 2022

This makes shutil.copyfile prefer the copy_file_range system call on Linux.

copy_file_range gives filesystems an opportunity to implement the use of reflinks or server-side copy, but we cannot determine whether any of them are implemented. Therefore, I added an allow_reflink argument in case anyone wants to disable copy-on-write.

GNU Coreutils enables copy-on-write by default, that is why I set allow_reflink to true by default (unlike @vstinner proposed in #81338).

Note, there is a known copy_file_range bug for copying from special filesystems like procfs and sysfs. It seems not to be fixed yet, so we have to check for a silent copy_file_range fail as Coreutils and Go do.

@illia-v
Copy link
Contributor Author

illia-v commented May 23, 2022

@giampaolo I saw that you are marked as a code owner for shutil, but GitHub did not request your review for some reason.

**/*shutil @giampaolo

Would you like to take a look?

@giampaolo
Copy link
Contributor

@illia-v to my understanding copy_file_range() on Linux enables the so called "server side copy", not "reflink" / "copy on write". I posted a patch some time ago, but I should update it, see:
#81340

As for reflink / CoW copy, I also submitted a patch some time ago, but I wanted to add Windows support before merging it.
#81338

To my understanding reflink / CoW copy on Linux is achieved by using FICLONE, not copy_file_range().

@illia-v
Copy link
Contributor Author

illia-v commented May 23, 2022

@vstinner
Copy link
Member

cc @pablogsal

Doc/library/shutil.rst Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
Lib/shutil.py Outdated Show resolved Hide resolved
Lib/shutil.py Outdated Show resolved Hide resolved
Doc/library/shutil.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

@giampaolo copy_file_range does try cloning. Take a look at https://stackoverflow.com/a/65518879 and https://github.com/torvalds/linux/blob/1e57930e9f4083ad5854ab6eadffe790a8167fb4/fs/read_write.c#L1491-L1507.

Would you mind to create a first PR to mention it in copy_file_range() documentation? https://docs.python.org/dev/library/os.html#os.copy_file_range

@vstinner
Copy link
Member

copyfile(src, dst, *, follow_symlinks=True, allow_reflink=True)

It terms of API, I would prefer to change the parameter name to just reflink=True. IMO it's clear enough that reflink=True will not use CoW if it's not supported or cannot be used. Python has a long habit of trying to use the most efficient method, but falls back on the most compatible code if it doesn't work.

For example, socket.sendfile() tries to use sendfile() but falls back on send() if sendfile() doesn't work or is not available.

@vstinner
Copy link
Member

The Unix cp command has 3 modes for --reflink=WHEN:

  • always: "perform a lightweight copy (...) if this is not possible the copy fails"
  • never (False in your PR)
  • auto (True in your PR)

Here you only propose 2 modes. Do we need to implement the 3rd "always" mode?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Your PR is quite big. Would it be possible to extract the tests refactoring and start with a first PR just for that?

@vstinner
Copy link
Member

I see a lot of confusion between which function/syscall is used, server-side copy, and copy of write. There are expectections, hopes, and ... disillusions :-) IMO the doc should be completed to better explain differences between server-side copy and copy-on-write, and attempt to document when and how one method is attempted, when it can fail, etc.

@illia-v
Copy link
Contributor Author

illia-v commented May 23, 2022

I am glad the issue has received this attention.

@vstinner thanks for valid points, especially one about the three modes. I think it will be nice to support all of them in Python too.

But copy_file_range has no way to specify that reflink is required.

  • cp --reflink=always command seems to use ioctl(dest_fd, FICLONE, src_fd) (1 & 2) as the only copying method or fail if the call is not possible;
  • cp --reflink=auto tries ioctl(dest_fd, FICLONE, src_fd) firstly, then copy_file_range, then others;
  • cp --reflink=never skips both ioctl(dest_fd, FICLONE, src_fd) and copy_file_range.

I will create a PR to extend os.copy_file_range documentation. Then I will need to experiment a bit with FICLONE.

Do you think it is possible to launch the three-mode reflink functionality for Linux firstly and add support for other OSes in later pull requests?
I am most interested in supporting this on Linux, but I may consider writing some code for other platforms later.

@vstinner
Copy link
Member

Do you think it is possible to launch the three-mode reflink functionality for Linux firstly and add support for other OSes in later pull requests?

If possible, the API should be the same and should be usable on all platform. For example, "as an user, I would like to call shutil.copyfile() with the same arguments on all platforms and get a similar behavior on all platforms". For example, the API should attempt to use server-side copy and/or use Copy-on-Write, but silently switch to regular read+write copy if no modern copy function is available on the OS or on the source/destination filesystems.

For the os module, it's fine to have different functions depending on the OS. But the shutil module is more a high-level module with a (mostly) portable API.

@illia-v
Copy link
Contributor Author

illia-v commented May 24, 2022

@giampaolo copy_file_range does try cloning. Take a look at https://stackoverflow.com/a/65518879 and https://github.com/torvalds/linux/blob/1e57930e9f4083ad5854ab6eadffe790a8167fb4/fs/read_write.c#L1491-L1507.

Would you mind to create a first PR to mention it in copy_file_range() documentation? https://docs.python.org/dev/library/os.html#os.copy_file_range

Done in #93182, please review it.

@illia-v illia-v marked this pull request as draft May 26, 2022 10:21
@illia-v
Copy link
Contributor Author

illia-v commented Jun 3, 2022

I am swaying away from the idea in favor of a separate reflink function #81338 (comment).

@thesamesam
Copy link
Contributor

thesamesam commented Jul 6, 2023

I am swaying away from the idea in favor of a separate reflink function #81338 (comment).

The problem with that is you don't get the automatic free win, which is contrary to what others are doing. e.g. GNOME's glib (not glibc), KDE's kio, and even GCC's libstdc++ upgrade automatically. GCC 14 will automatically use copy_file_range if available for filesystem::copy_file.

There doesn't appear to be much precedent for carving this out into its own opt-in functionality?

EDIT: It's been pointed out to me that Emacs, of all things, unconditionally uses copy_file_range if the system supports it for copy-file.

@barneygale
Copy link
Contributor

@illia-v are you still interested in working on this? I wonder if we even need the allow_reflinks argument - perhaps we could enable use of copy_file_range() (where available) without adding any new arguments?

@illia-v
Copy link
Contributor Author

illia-v commented Jun 3, 2024

@barneygale yes, I am interested and can revise the changes this week

@illia-v illia-v marked this pull request as ready for review June 5, 2024 16:10
@illia-v
Copy link
Contributor Author

illia-v commented Jun 5, 2024

@barneygale I dropped allow_reflinks, please take a look

Doc/library/shutil.rst Outdated Show resolved Hide resolved
Lib/shutil.py Show resolved Hide resolved
@barneygale barneygale requested a review from vstinner June 5, 2024 19:16
@illia-v illia-v requested a review from barneygale June 5, 2024 19:32
Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

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

LGTM

@giampaolo
Copy link
Contributor

giampaolo commented Jun 7, 2024

As I've previously stated in #81338 (comment), personally I consider copy and CoW / clone 2 distinct operations. E.g. one may want to occupy disk space right now instead of later. For this reason I think shutil.copyfile should give the possibility to disable this behavior. cp command defaults to --reflink=auto, but it allows to disable CoW with --reflink=never. The problem with copy_file_range() is that it doesn't allow you to opt-out from CoW, which is why back in 2020 I was more keen on providing a separate shutil.reflink function using FICLONE. But then again, copy_file_range() does provide fast server-side copy in some circumstances, which is something you always want. What a mess...

It appears to me that the tool which solved all these controversies is cp via --reflink=auto/always/never. I have the feeling that shutil.copyfile (and possibly also pathlib.Path.copy / #73991) should try to follow its lead and replicate that. E.g. given the signature:

def copyfile(..., reflink=None)

We may do:

  • reflink=None == cp --reflink=auto == use copy_file_range() - on failure use sendfile() or read/write
  • reflink=True == cp --reflink=always == use FICLONE and fail on error
  • reflink=False == cp --reflink=never == use sendfile() or read/write

But if we do this, I think it would make sense to expose shutil.reflink() first, which would also support macOS and Windows.

My 2 cents and sorry for not replying earlier on this topic.

@barneygale
Copy link
Contributor

There are some compelling arguments and examples from other languages in #81338. Personally I'm persuaded that we should enable it (without an opt-out), rather than second-guessing what the OS provides and what other language runtimes are doing. We have plenty of time til 3.14 ships if we need to back it out. But this is by no means my area of expertise, so I'm happy to be overruled.

barneygale added a commit that referenced this pull request Jun 14, 2024
Add a `Path.copy()` method that copies the content of one file to another.

This method is similar to `shutil.copyfile()` but differs in the following ways:

- Uses `fcntl.FICLONE` where available (see GH-81338)
- Uses `os.copy_file_range` where available (see GH-81340)
- Uses `_winapi.CopyFile2` where available, even though this copies more metadata than the other implementations. This makes `WindowsPath.copy()` more similar to `shutil.copy2()`.

The method is presently _less_ specified than the `shutil` functions to allow OS-specific optimizations that might copy more or less metadata.

Incorporates code from GH-81338 and GH-93152.

Co-authored-by: Eryk Sun <[email protected]>
@zooba
Copy link
Member

zooba commented Jun 17, 2024

Personally I'm persuaded that we should enable it (without an opt-out), rather than second-guessing what the OS provides and what other language runtimes are doing.

This seems alright to me, too. We're expecting/hoping Windows will start doing transparent CoW when doing file copies, so that ought to give an idea pretty quickly if there's a genuine need to avoid it, or if we'd be adding the opt-out without justification.

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
Add a `Path.copy()` method that copies the content of one file to another.

This method is similar to `shutil.copyfile()` but differs in the following ways:

- Uses `fcntl.FICLONE` where available (see pythonGH-81338)
- Uses `os.copy_file_range` where available (see pythonGH-81340)
- Uses `_winapi.CopyFile2` where available, even though this copies more metadata than the other implementations. This makes `WindowsPath.copy()` more similar to `shutil.copy2()`.

The method is presently _less_ specified than the `shutil` functions to allow OS-specific optimizations that might copy more or less metadata.

Incorporates code from pythonGH-81338 and pythonGH-93152.

Co-authored-by: Eryk Sun <[email protected]>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
Add a `Path.copy()` method that copies the content of one file to another.

This method is similar to `shutil.copyfile()` but differs in the following ways:

- Uses `fcntl.FICLONE` where available (see pythonGH-81338)
- Uses `os.copy_file_range` where available (see pythonGH-81340)
- Uses `_winapi.CopyFile2` where available, even though this copies more metadata than the other implementations. This makes `WindowsPath.copy()` more similar to `shutil.copy2()`.

The method is presently _less_ specified than the `shutil` functions to allow OS-specific optimizations that might copy more or less metadata.

Incorporates code from pythonGH-81338 and pythonGH-93152.

Co-authored-by: Eryk Sun <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Add a `Path.copy()` method that copies the content of one file to another.

This method is similar to `shutil.copyfile()` but differs in the following ways:

- Uses `fcntl.FICLONE` where available (see pythonGH-81338)
- Uses `os.copy_file_range` where available (see pythonGH-81340)
- Uses `_winapi.CopyFile2` where available, even though this copies more metadata than the other implementations. This makes `WindowsPath.copy()` more similar to `shutil.copy2()`.

The method is presently _less_ specified than the `shutil` functions to allow OS-specific optimizations that might copy more or less metadata.

Incorporates code from pythonGH-81338 and pythonGH-93152.

Co-authored-by: Eryk Sun <[email protected]>
@illia-v
Copy link
Contributor Author

illia-v commented Jul 31, 2024

@barneygale based on your experience of having #119058 present in main for more than a month, can we merge this too?

@barneygale
Copy link
Contributor

There have been no releases from main in that time, and the first alpha of 3.14 isn't due until October, so I wouldn't read too much into the lack of pathlib.Path.copy() bug reports.

@illia-v
Copy link
Contributor Author

illia-v commented Aug 5, 2024

@barneygale thank you for the response. Can we risk merging this PR before the alpha to be able to discover issues (if any) in early stages of 3.14?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants