Re: svn commit: r366697 - head/usr.bin/xinstall
BTW, I have a WIP patch to xinstall to make it use copy_file_range. The
patch works, but I never wrote a fallback path in case copy_file_range
fails for some reason. Alex, would you be interested to finish the patch?
-Alan
On Wed, Oct 14, 2020 at 7:35 AM Alexander Richardson <
arichard...@freebsd.org> wrote:
> On Wed, 14 Oct 2020 at 14:29, Mateusz Guzik wrote:
> >
> > This should use copy_file_range (also available on Linux).
> >
>
> I agree. I even mentioned this in
> https://reviews.freebsd.org/D26041#589287.
> This change avoids the two unnecessary syscalls, but I agree that
> longer-term install should share the copy_file_range code with cp.
> The only thing that copy_file_range won't speed up is the check
> whether source and target are already identical.
>
> Alex
>
> > On 10/14/20, Alex Richardson wrote:
> > > Author: arichardson
> > > Date: Wed Oct 14 12:28:41 2020
> > > New Revision: 366697
> > > URL: https://svnweb.freebsd.org/changeset/base/366697
> > >
> > > Log:
> > > 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
> > >
> > > Modified:
> > > head/usr.bin/xinstall/xinstall.c
> > >
> > > Modified: head/usr.bin/xinstall/xinstall.c
> > >
> ==
> > > --- head/usr.bin/xinstall/xinstall.c Wed Oct 14 10:12:39 2020
> (r366696)
> > > +++ head/usr.bin/xinstall/xinstall.c Wed Oct 14 12:28:41 2020
> (r366697)
> > > @@ -148,7 +148,7 @@ static void metadata_log(const char *, const
> char *, s
> > > 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,
> > > s
> > > 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,
> > > co
> > >
> > > 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 ||
> > > -
Re: svn commit: r366697 - head/usr.bin/xinstall
On Thu, 15 Oct 2020 at 06:59, Enji Cooper wrote: > > > > On Oct 14, 2020, at 5:28 AM, Alex Richardson > > wrote: > > > > Author: arichardson > > Date: Wed Oct 14 12:28:41 2020 > > New Revision: 366697 > > URL: https://svnweb.freebsd.org/changeset/base/366697 > > > > Log: > > 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 > > * Has this change been tested out with source filesystems other than > UFS/ZFS? Not all filesystems support mmap(2). I've used ext4 and afps, but it doesn't matter since there's a fallback. > * trymmap(..) seems to be less about computing whether or not the > filesystem should use mmap(2) after this change, but how it should use > mmap(2). Seems like a misnamed function call now. There was always fallback code in case mmap fails: https://github.com/freebsd/freebsd/blob/8349de39d23fc152c3782ee3b9eeab3df4610944/usr.bin/xinstall/xinstall.c#L1253 and https://github.com/freebsd/freebsd/blob/8349de39d23fc152c3782ee3b9eeab3df4610944/usr.bin/xinstall/xinstall.c#L1253 so it is really "should I try to use mmap()". Alex ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r366697 - head/usr.bin/xinstall
> On Oct 14, 2020, at 5:28 AM, Alex Richardson wrote: > > Author: arichardson > Date: Wed Oct 14 12:28:41 2020 > New Revision: 366697 > URL: https://svnweb.freebsd.org/changeset/base/366697 > > Log: > 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 * Has this change been tested out with source filesystems other than UFS/ZFS? Not all filesystems support mmap(2). * trymmap(..) seems to be less about computing whether or not the filesystem should use mmap(2) after this change, but how it should use mmap(2). Seems like a misnamed function call now. Cheers, -Enji ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r366697 - head/usr.bin/xinstall
On Wed, Oct 14, 2020 at 02:40:42PM +0100, Jessica Clarke wrote: > On 14 Oct 2020, at 14:28, Mateusz Guzik wrote: > > > > This should use copy_file_range (also available on Linux). > > I assume this is a bootstrap tool and hence the system OS and version > is relevant. macOS does not have copy_file_range, and FreeBSD only has > it in -CURRENT so that would break building on 11.x and 12.x. So any > use would need to be guarded by preprocessor checks (and there are > still actively-supported Linux distributions out there that are based > on too-old versions of the kernel and/or glibc to include it). > > (FYI macOS's equivalent is copyfile(3)... maybe one day it will adopt > the copy_file_range(2) interface too) copyfile has different semantics, not the least of which is supporting file clones. Once ZFS grows file clone support it would be nice if install supported them as well. I'd love to only pay the inode cost for installed files when I'm just building a disk image. -- Brooks signature.asc Description: PGP signature
Re: svn commit: r366697 - head/usr.bin/xinstall
On 10/14/20, Alexander Richardson wrote: > On Wed, 14 Oct 2020 at 14:29, Mateusz Guzik wrote: >> >> This should use copy_file_range (also available on Linux). >> > > I agree. I even mentioned this in > https://reviews.freebsd.org/D26041#589287. > This change avoids the two unnecessary syscalls, but I agree that > longer-term install should share the copy_file_range code with cp. > The only thing that copy_file_range won't speed up is the check > whether source and target are already identical. > So did a quick check with make install in bin/cp: install -s -o root -g wheel -m 555 cp /zoo/lynx4/bin/cp install -o root -g wheel -m 444 cp.debug /zoo/lynx4/usr/lib/debug/bin/cp.debug install -o root -g wheel -m 444 cp.1.gz /zoo/lynx4/usr/share/man/man1/ None of these result in calling the compare routine. Looking at truss output it seems the biggest win for these files would be to avoid root:wheel translation. -- Mateusz Guzik ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r366697 - head/usr.bin/xinstall
On 14 Oct 2020, at 14:28, Mateusz Guzik wrote:
>
> This should use copy_file_range (also available on Linux).
I assume this is a bootstrap tool and hence the system OS and version
is relevant. macOS does not have copy_file_range, and FreeBSD only has
it in -CURRENT so that would break building on 11.x and 12.x. So any
use would need to be guarded by preprocessor checks (and there are
still actively-supported Linux distributions out there that are based
on too-old versions of the kernel and/or glibc to include it).
(FYI macOS's equivalent is copyfile(3)... maybe one day it will adopt
the copy_file_range(2) interface too)
Jess
> On 10/14/20, Alex Richardson wrote:
>> Author: arichardson
>> Date: Wed Oct 14 12:28:41 2020
>> New Revision: 366697
>> URL: https://svnweb.freebsd.org/changeset/base/366697
>>
>> Log:
>> 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
>>
>> Modified:
>> head/usr.bin/xinstall/xinstall.c
>>
>> Modified: head/usr.bin/xinstall/xinstall.c
>> ==
>> --- head/usr.bin/xinstall/xinstall.c Wed Oct 14 10:12:39 2020
>> (r366696)
>> +++ head/usr.bin/xinstall/xinstall.c Wed Oct 14 12:28:41 2020
>> (r366697)
>> @@ -148,7 +148,7 @@ static void metadata_log(const char *, const char
>> *, s
>> 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,
>> s
>> 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,
>> co
>>
>> 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 sysc
Re: svn commit: r366697 - head/usr.bin/xinstall
On Wed, 14 Oct 2020 at 14:29, Mateusz Guzik wrote:
>
> This should use copy_file_range (also available on Linux).
>
I agree. I even mentioned this in https://reviews.freebsd.org/D26041#589287.
This change avoids the two unnecessary syscalls, but I agree that
longer-term install should share the copy_file_range code with cp.
The only thing that copy_file_range won't speed up is the check
whether source and target are already identical.
Alex
> On 10/14/20, Alex Richardson wrote:
> > Author: arichardson
> > Date: Wed Oct 14 12:28:41 2020
> > New Revision: 366697
> > URL: https://svnweb.freebsd.org/changeset/base/366697
> >
> > Log:
> > 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
> >
> > Modified:
> > head/usr.bin/xinstall/xinstall.c
> >
> > Modified: head/usr.bin/xinstall/xinstall.c
> > ==
> > --- head/usr.bin/xinstall/xinstall.c Wed Oct 14 10:12:39 2020
> > (r366696)
> > +++ head/usr.bin/xinstall/xinstall.c Wed Oct 14 12:28:41 2020
> > (r366697)
> > @@ -148,7 +148,7 @@ static void metadata_log(const char *, const char
> > *, s
> > 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,
> > s
> > 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,
> > co
> >
> > 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.
> > +
Re: svn commit: r366697 - head/usr.bin/xinstall
This should use copy_file_range (also available on Linux).
On 10/14/20, Alex Richardson wrote:
> Author: arichardson
> Date: Wed Oct 14 12:28:41 2020
> New Revision: 366697
> URL: https://svnweb.freebsd.org/changeset/base/366697
>
> Log:
> 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
>
> Modified:
> head/usr.bin/xinstall/xinstall.c
>
> Modified: head/usr.bin/xinstall/xinstall.c
> ==
> --- head/usr.bin/xinstall/xinstall.c Wed Oct 14 10:12:39 2020
> (r366696)
> +++ head/usr.bin/xinstall/xinstall.c Wed Oct 14 12:28:41 2020
> (r366697)
> @@ -148,7 +148,7 @@ static void metadata_log(const char *, const char
> *, s
> 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,
> s
> 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,
> co
>
> 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);
> }
> _
svn commit: r366697 - head/usr.bin/xinstall
Author: arichardson
Date: Wed Oct 14 12:28:41 2020
New Revision: 366697
URL: https://svnweb.freebsd.org/changeset/base/366697
Log:
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
Modified:
head/usr.bin/xinstall/xinstall.c
Modified: head/usr.bin/xinstall/xinstall.c
==
--- head/usr.bin/xinstall/xinstall.cWed Oct 14 10:12:39 2020
(r366696)
+++ head/usr.bin/xinstall/xinstall.cWed Oct 14 12:28:41 2020
(r366697)
@@ -148,7 +148,7 @@ static void metadata_log(const char *, const char *, s
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 voidusage(void);
int
@@ -1087,7 +1087,7 @@ compare(int from_fd, const char *from_name __unused, s
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, co
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);
}
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
