install: Always use a temporary file.

[des]

Summary

Previously, we would only use a temporary file if explicitly asked to
with the -S option, and even then, only if the target file already
existed. This meant that an outside observer looking for the target
file might see a partial file, and might see the file disappear and
then reappear.

With this patch, we always use a temporary file, ensuring atomicity.
The downside is slightly increased disk usage. The upside is never
having to worry about, for instance, cron jobs randomly failing if
they happen to run simultaneously with make installworld.

The -S option is retained, partly for compatibility, and partly
to control the use of fsync(2), which has a non-negligible cost
(approximately 10% increase in wall time for make installworld).

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

[markj]

The downside is slightly increased disk usage.

Won't this now also be a fair bit slower since install now fsync()s its output files before exiting? I don't particularly object to the change overall, but does it make a significant difference to, say, installworld runtime?

[brooks]

Atomicity is well worth the quite minimal potential cost here.

usr.bin/xinstall/install.1

233–235

[imp]

Generally this looks like a nop vs always forcing S on.
One or two places might me a smidge less efficient. I'm having trouble seeing that it could be measured, much lass it having a material impact on installworld.

usr.bin/xinstall/install.1

237–239

Hmmm, looking at the code, it doesn't do anything now. I didn't get that sense from reading this description, though.

.It Fl S
Install always copies safely, so this option is only parsed for backwards compatibility.

might be better?

usr.bin/xinstall/xinstall.c

901

This is a little less efficient... But I strongly suspect with today's slowest disks, even, one couldn't measure a difference.

[des]

usr.bin/xinstall/install.1

233–235

I copied the wording from the entry for -c. I can change both or neither but not just one.

237–239

Exactly, it doesn't do anything, but we don't want to break scripts and Makefiles that use it, so we're keeping it for backwards compatibility, just like the -c option.

[des]

In D44742#1019804, @markj wrote:

Won't this now also be a fair bit slower since install now fsync()s its output files before exiting? I don't particularly object to the change overall, but does it make a significant difference to, say, installworld runtime?

OK, I built world without the patch and then ran sudo make installworld -j8 twice in a row, timing the second run.

sudo make installworld -j8  77.59s user 86.52s system 251% cpu 1:05.35 total

Then I reapplied the patch and did the same thing again:

sudo make installworld -j8  77.56s user 87.40s system 227% cpu 1:12.65 total

That's a 10% slowdown in wall time. Not insignificant.

What if we make the fsync() calls, and _only_ the fsync() calls, conditional on -S? That way we get most of the benefit (atomicity from the point of view of other processes running concurrently with installworld, but not crash resistance) at a lower cost:

sudo make installworld -j8  78.50s user 89.57s system 250% cpu 1:07.05 total

[des]

make fsync conditional again

[0mp]

usr.bin/xinstall/install.1

228–231

Maybe s/Previously/Historically/?

Also, the current behavior should be listed first I think. The historical would then be another paragraph in the description of the flag.

[des]

improve man page

[0mp]

The manual page change LGTM.

[markj]

usr.bin/xinstall/xinstall.c

1234

An older bug: I think we're failing to fsync in this path.

[des]

usr.bin/xinstall/xinstall.c

1234

Thank you, see D44756.

[imp]

I'm cool with this.