install(1): Avoid unncessary fstatfs() calls and use mmap() based on size

[arichardson]

Summary

According to git blame the trymmap() function was added in 1996 to skip
mmap() calls for NFS file systems. However, nowadays mmap() should be
perfectly safe even on NFS. Importantly, onl ufs and cd9660 file systems
were whitelisted so we don't use mmap() on ZFS. It also prevents the use
of mmap() when bootstrapping from macOS/Linux since on those systems the
trymmap() function was always returning zero due to the missing MFSNAMELEN
define.

This change keeps the trymmap() function but changes it to check whether
using mmap() can reduce the number of system calls that are required.
Nevertheless, using mmap() only reduces the number of system calls if we
need multiple read() syscalls, i.e. if the file size is > MAXBSIZE. However,
mmap() is more expensive than read() so this sets the threshold at 4 fewer
syscalls. Additionally, for larger file size mmap() can significantly increase
the number of page faults, so avoid it in that case.

It's unclear whether using mmap() is ever faster than a read with an appropriate
buffer size, but this change at least removes two unnecessary system calls
for every file that is installed.

Test Plan

installworld still works, but too much noise to measure a performance difference. Number of syscalls is reduced though.

[emaste]

reduce the number of system.

missing "calls" here it looks like. No objection to this change.

[brooks]

Add a couple people to potentially comment on what the max size to mmap should actually be.

[markj]

In D26041#578375, @brooks wrote:

Add a couple people to potentially comment on what the max size to mmap should actually be.

I'm not sure that we really need a limit. I believe the page fault handler will apply a "sequential" heuristic and move faulted pages into the inactive queue once the scan is done with them.

Have you benchmarked a plain install(1) invocation on a file <= 8MB? Does using mmap() help all that much?

I'm not certain but on ZFS using mmap might result in some memory bloat since the page cache and ARC are not unified, especially if the file being read was recently written, as I'd expect during a build+install.

[arichardson]

In D26041#578409, @markj wrote:

In D26041#578375, @brooks wrote:

Add a couple people to potentially comment on what the max size to mmap should actually be.

I'm not sure that we really need a limit. I believe the page fault handler will apply a "sequential" heuristic and move faulted pages into the inactive queue once the scan is done with them.

Have you benchmarked a plain install(1) invocation on a file <= 8MB? Does using mmap() help all that much?

I'm not certain but on ZFS using mmap might result in some memory bloat since the page cache and ARC are not unified, especially if the file being read was recently written, as I'd expect during a build+install.

I haven't done any real benchmarking, doing installworld it's all just noise. I just saw all these fstatfs syscalls and wondered where they come from.
I think doing the 64k reads() is probably also fine since it should avoid the page faults caused by mmap().

[markj]

In D26041#578454, @arichardson wrote:

In D26041#578409, @markj wrote:

In D26041#578375, @brooks wrote:

Add a couple people to potentially comment on what the max size to mmap should actually be.

I'm not sure that we really need a limit. I believe the page fault handler will apply a "sequential" heuristic and move faulted pages into the inactive queue once the scan is done with them.

Have you benchmarked a plain install(1) invocation on a file <= 8MB? Does using mmap() help all that much?

I'm not certain but on ZFS using mmap might result in some memory bloat since the page cache and ARC are not unified, especially if the file being read was recently written, as I'd expect during a build+install.

I haven't done any real benchmarking, doing installworld it's all just noise. I just saw all these fstatfs syscalls and wondered where they come from.
I think doing the 64k reads() is probably also fine since it should avoid the page faults caused by mmap().

In a quick test on UFS I don't see anything that looks like a significant difference between mmap() and read() for a few different file sizes. For mmap() we can set MAP_PREFAULT_READ to avoid page faults, but that doesn't seem to make a difference for small files. So I'm not really convinced yet that the mmap() path helps with anything. cp(1) has a similar heuristic btw, though it doesn't use fstatfs().

usr.bin/xinstall/xinstall.c

1526 ↗

(On Diff #75729)

Why not just remove the fd parameter?

[arichardson]

Remove fd parameter and update comments

[arichardson]

I guess the better solution would be to use copy_file_range(), but that won't speed up the "are these files identical" check.

[markj]

I have no objection to the change, it brings install(1)'s logic closer to that used in cp(1).

Do you have a benchmark which demonstrates a difference when read() is used instead of mmap() for small files?

[arichardson]

In D26041#590583, @markj wrote:

I have no objection to the change, it brings install(1)'s logic closer to that used in cp(1).

Do you have a benchmark which demonstrates a difference when read() is used instead of mmap() for small files?

No I don't and I doubt I'll have time to test this in the near future. My guess is that it probably doesn't make much of a difference for small files.

[markj]

In D26041#590584, @arichardson wrote:

In D26041#590583, @markj wrote:

I have no objection to the change, it brings install(1)'s logic closer to that used in cp(1).

Do you have a benchmark which demonstrates a difference when read() is used instead of mmap() for small files?

No I don't and I doubt I'll have time to test this in the near future. My guess is that it probably doesn't make much of a difference for small files.

Indeed, so I'm not sure I'd even bother handling that as a separate case for now.

[arichardson]

Unless anyone objects, I plan to commit this change at the end of this week (or earlier if I get an approval).