cp: Simplify the common case.
ClosedPublic
Actions

Authored by des on Jan 31 2023, 2:16 PM.

Details

Reviewers

rmacklem
mav

Group Reviewers

Klara

Commits

rG6c85042afcbb: cp: Simplify the common case.

Summary

The allocated buffer is only used in the fallback case, so move it there. The argument for passing it in from the caller was that if malloc(3) were to fail, we'd want it to fail before we started copying anything, but firstly, it was already not in the right place to ensure that, and secondly, malloc(3) never fails (except in very contrived circumstances, such as an unreasonable RLIMIT_AS or RLIMIT_DATA).

Remove the mmap(2) option. It is almost never beneficial, especially when the alternative is copy_file_range(2), and it adds needless complexity and indentation.

MFC after: 1 week
Sponsored by: Klara, Inc.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

des created this revision.Jan 31 2023, 2:16 PM

Herald added a subscriber: imp. · View Herald TranscriptJan 31 2023, 2:16 PM

des requested review of this revision.Jan 31 2023, 2:16 PM

Harbormaster completed remote builds in B49353: Diff 116103.Jan 31 2023, 2:16 PM

des mentioned this in D38191: cp: Add a -S option which preserves holes in sparse files..Jan 31 2023, 2:17 PM

des edited the summary of this revision. (Show Details)Jan 31 2023, 2:26 PM

From perspective of ZFS I am all for the removal of mmap() use. ZFS does not benefit from it, but only suffers badly from created buffer cache pressure. We have it disabled on TrueNAS. I was tempted to remove it, but didn't want to open large discussion. I guess it may be beneficial for UFS and any FS using buffer cache, avoiding one memory copy. But I think it should be handled by copy_file_range() inside the kernel in a way the most efficient for specific file system(s).

This revision is now accepted and ready to land.Jan 31 2023, 2:39 PM

I agree with mav@'s comments and looks good to me.
(I'll admit I didn't look at the code in detail.)

I will also note that using mmap(2) is not
efficient for NFS either.

In D38291#870022, @mav wrote:

From perspective of ZFS I am all for the removal of mmap() use. ZFS does not benefit from it, but only suffers badly from created buffer cache pressure. We have it disabled on TrueNAS. I was tempted to remove it, but didn't want to open large discussion. I guess it may be beneficial for UFS and any FS using buffer cache, avoiding one memory copy. But I think it should be handled by copy_file_range() inside the kernel in a way the most efficient for specific file system(s).

I agree completely. copy_file_range() is the new way to optimize. I'm not at all sure that there's any remaining benefit from the mmap optimization...

Closed by commit rG6c85042afcbb: cp: Simplify the common case. (authored by des). · Explain WhyFeb 2 2023, 2:47 PM

This revision was automatically updated to reflect the committed changes.

des added a commit: rG6c85042afcbb: cp: Simplify the common case..

The copyright may be on the weak side for this, but this violates my rights. Notice D28958.

I had hoped to get D28958 in at the same time @imp was handling D35908, but that didn't happen.

Notable FreeBSD's copy_file_range() is recent. Nothing before FreeBSD 13.0 (old for active development, but newish for support) had copy_file_range().

I had been considering proposing doing copy_fallback(-1, -1); early as a way to allocate the buffer early.