Page MenuHomeFreeBSD

cp: Simplify the common case.
ClosedPublic

Authored by des on Jan 31 2023, 2:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 20 2023, 8:40 AM
Unknown Object (File)
Dec 12 2023, 5:08 PM
Unknown Object (File)
Oct 18 2023, 2:41 PM
Unknown Object (File)
Jul 25 2023, 1:22 AM
Unknown Object (File)
Jul 25 2023, 12:27 AM
Unknown Object (File)
Jul 3 2023, 4:06 AM
Unknown Object (File)
May 10 2023, 6:31 PM
Unknown Object (File)
May 2 2023, 6:36 AM

Details

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 requested review of this revision.Jan 31 2023, 2:16 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...

This revision was automatically updated to reflect the committed changes.

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.

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

No, it does not.