-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
copy_file_range: use FICLONERANGE when possible #12489
Conversation
This is a follow-up from ziglang#12476: instead of adding a new abstraction to clone files, start with `copy_file_range`. They use the same mechanism in the kernel. Benefits of FICLONERANGE vs `copy_file_range(2)`: - O(1). - CoW: so if files are not modified, it will not take additional space. However, it is more restricted than copy_file_range: source and destination must be on the same partition, and only some file systems implement this. As of Linux 5.19 those are btrfs, cifs, nfs, ocfs2, overlayfs and xfs[1]. [1]: https://elixir.bootlin.com/linux/v5.19/A/ident/remap_file_range
aa4df54
to
9178a10
Compare
.dest_offset = off_out, | ||
}; | ||
while (true) { | ||
const rc = system.ioctl(fd_out, linux.T.FICLONERANGE, @ptrToInt(&arg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you benchmark the impact when used on filesystems which do not support CoW clones? In that case, this adds an extra system call for each run
As of at least Ubuntu 20.04, the default filesystem is ext4 which does not support CoW clones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have put this to the commit message; I am not overly concerned with the extra syscall, since coreutils is doing reflinking by default for cp
; so I punted on the costs.
I see some other issues with the PR, marking as draft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out the number of syscalls. We may get rid of one in #12491
Linux kernel refuses to accept the argument, returns EINVAL for anything I give it. I will try with a C version, but not today. |
Here is the C version:
Observations:
Tried on Linux 5.15.0-40 (ubuntu) x86_64 with XFS and btrfs. I may dig into the kernel code separately (to report and/or fix), but for now it's suffice to say that the FICLONERANGE is not stable enough for use in zig. |
This is a follow-up from #12476: instead of adding a new abstraction to
clone files, start with
copy_file_range
. They use the same mechanismin the kernel.
Benefits of FICLONERANGE vs
copy_file_range(2)
:However, it is more restricted than copy_file_range: source and
destination must be on the same partition, and only some file systems
implement this. As of Linux 5.19 those are btrfs, cifs, nfs, ocfs2,
overlayfs and xfs1.
Note: I removed
flags
fromcopy_file_range
(that must be 0 as of writing). If we want to retainflags
, this change shouldn't be as-is, and we probably needos.clone_file_range
. If we addclone_file_range
, should that function have fallbacks, like thecopy_file_range
does now?