From 6e4162c60c8dc2647bcfa512b889934806939ce8 Mon Sep 17 00:00:00 2001
From: Alex Richardson <arichardson@FreeBSD.org>
Date: Wed, 14 Oct 2020 12:28:41 +0000
Subject: [PATCH] install(1): Avoid unncessary fstatfs() calls and use mmap()
 based on size

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.
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.

Reviewed By:	markj
Differential Revision: https://reviews.freebsd.org/D26041
---
 usr.bin/xinstall/xinstall.c | 44 ++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
index 9531427b9990..114614abd16f 100644
--- a/usr.bin/xinstall/xinstall.c
+++ b/usr.bin/xinstall/xinstall.c
@@ -148,7 +148,7 @@ static void	metadata_log(const char *, const char *, struct timespec *,
 		    const char *, const char *, off_t);
 static int	parseid(const char *, id_t *);
 static int	strip(const char *, int, const char *, char **);
-static int	trymmap(int);
+static int	trymmap(size_t);
 static void	usage(void);
 
 int
@@ -1087,7 +1087,7 @@ compare(int from_fd, const char *from_name __unused, size_t from_len,
 		if (do_digest)
 			digest_init(&ctx);
 		done_compare = 0;
-		if (trymmap(from_fd) && trymmap(to_fd)) {
+		if (trymmap(from_len) && trymmap(to_len)) {
 			p = mmap(NULL, from_len, PROT_READ, MAP_SHARED,
 			    from_fd, (off_t)0);
 			if (p == MAP_FAILED)
@@ -1248,13 +1248,8 @@ copy(int from_fd, const char *from_name, int to_fd, const char *to_name,
 
 	digest_init(&ctx);
 
-	/*
-	 * Mmap and write if less than 8M (the limit is so we don't totally
-	 * trash memory on big files.  This is really a minor hack, but it
-	 * wins some CPU back.
-	 */
 	done_copy = 0;
-	if (size <= 8 * 1048576 && trymmap(from_fd) &&
+	if (trymmap((size_t)size) &&
 	    (p = mmap(NULL, (size_t)size, PROT_READ, MAP_SHARED,
 		    from_fd, (off_t)0)) != MAP_FAILED) {
 		nw = write(to_fd, p, size);
@@ -1523,20 +1518,23 @@ usage(void)
  *	return true (1) if mmap should be tried, false (0) if not.
  */
 static int
-trymmap(int fd)
+trymmap(size_t filesize)
 {
-/*
- * The ifdef is for bootstrapping - f_fstypename doesn't exist in
- * pre-Lite2-merge systems.
- */
-#ifdef MFSNAMELEN
-	struct statfs stfs;
-
-	if (fstatfs(fd, &stfs) != 0)
-		return (0);
-	if (strcmp(stfs.f_fstypename, "ufs") == 0 ||
-	    strcmp(stfs.f_fstypename, "cd9660") == 0)
-		return (1);
-#endif
-	return (0);
+	/*
+	 * This function existed to skip mmap() for NFS file systems whereas
+	 * nowadays mmap() should be perfectly safe. 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 set 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.
+	 *
+	 * Note: the 8MB limit is not based on any meaningful benchmarking
+	 * results, it is simply reusing the same value that was used before
+	 * and also matches bin/cp.
+	 *
+	 * XXX: Maybe we shouldn't bother with mmap() at all, since we use
+	 * MAXBSIZE the syscall overhead of read() shouldn't be too high?
+	 */
+	return (filesize > 4 * MAXBSIZE && filesize < 8 * 1024 * 1024);
 }