Skip to content

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Feb 7, 2025

Add a function to copy via reflink if possible or fall back to the normal io.Copy().
c/image uses this function internally[1] however I like to export this and use it in podman and I think this package here in c/storage is the most logical location for it. Note compared to the c/image version I am calling unix.IoctlFileClone() directly instead of the raw syscall function.

[1] https://github.com/containers/image/tree/main/internal/reflink

@Luap99
Copy link
Member Author

Luap99 commented Feb 7, 2025

@mtrmac @giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Feb 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Feb 7, 2025
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks! I agree c/storage is the right place for this.

I wonder whether we want the “…OrCopy” part; it seems somewhat attractive to me to have drivers/copy.CopyRegularToFile call this new function. (And there’s… friction: that function needs to explicitly implement doCopyWithFileRange only because it relies on pools.Copy, which defeats the equivalent of doCopyWithFileRange inside the standard library.)

I think we can start with this …OrCopy function for now, and we can always add a reflink-only function later — I’m pointing this out in case others think differently.

Add a function to copy via reflink if possible or fall back to the
normal io.Copy().
c/image uses this function internally[1] however I like to export this
and use it in podman and I think this package here in c/storage is the
most logical location for it. Note compared to the c/image version I am
calling unix.IoctlFileClone() directly instead of the raw syscall
function.

[1] https://github.com/containers/image/tree/main/internal/reflink

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@cgwalters
Copy link
Contributor

On modern Linux copy_file_range automatically uses reflinks if available, but is also generally useful since if reflinks aren't available, it avoids the copy to and from userspace for the file data. So I think it should be generally preferred over the FICLONE ioctl nowadays.

BTW Rust automatically uses that by default via specialization for its std::io::copy

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@openshift-ci openshift-ci bot added the lgtm label Feb 10, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 13a758f into containers:main Feb 10, 2025
20 checks passed
@Luap99 Luap99 deleted the reflink branch February 10, 2025 20:53
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.

4 participants