improve swapon comments
ClosedPublic
Actions

Authored by dougm on Jul 23 2019, 3:15 AM.

Details

Reviewers

Commits

Summary

The comments that seek to explain the order of events in swapon_trim are something of a muddle. Endeavor to rewrite them to make them comprehensible.

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

sbin/swapon/swapon.c
770 ↗	(On Diff #60039)	You use two spaces after a period in the comment below.
777 ↗	(On Diff #60039)	I don't quite follow: what's the second connection here?

sbin/swapon/swapon.c
777 ↗	(On Diff #60039)	The one that swapon opens, and keeps internally, and closes when swapoff gets called eventually. If 'connection' isn't the right word, I'll take a suggestion a better one, but there's some count we're preventing from falling to zero, and if it's not "connections", what is it?

sbin/swapon/swapon.c
777 ↗	(On Diff #60039)	In GEOM terminology these references/connections create "consumers" of the GELI "provider" corresponding to the device. It might be clearer to write something like, "Do not close the device until the swap pager has attempted to create another consumer. For GELI devices created with the 'detach -l' option, removing the last consumer causes the device to be detached - that is, to disappear. This ordering ensures that the device will not be detached until swapoff is called."

Consume reviewer suggestions.

This revision is now accepted and ready to land.Jul 23 2019, 3:57 PM

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	`Open a descriptor to create a GEOM consumer of the device.`
769 ↗	(On Diff #60050)	`Start using the device for swapping, which implicitly creates a second GEOM consumer.`

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	I can't find documentation that assures me that swapon is only ever applied to GEOM devices, or consumers of devices, or whatever. So before I apply the suggested changes, I need some assurance that the rewritten comments will be true for all cases, not just geli cases.

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	Indeed, the swap pager can be configured to swap directly to a file rather than a GEOM. See sys_swapon(), which calls swapongeom() or swaponvp().

In one comment, connection -> descriptor.

This revision now requires review to proceed.Jul 25 2019, 7:32 PM

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	So, this comment is a bit misleading for the reason stated above: "name" might not refer to a GEOM device, but instead a regular file.

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	man (8) swapon says: The swapon, swapoff and swapctl utilities are used to control swap devices in the system. man (2) swapon says: The swapon() system call makes the block device special available to the system for allocation for paging and swapping. So "device" and "swapon" are pretty closely linked, and I don't see another word to use. Perhaps the problem is not in the comments, but rather that swapon_trim has been written so that it does not fail when "name" identifies a file instead of a device. Or maybe there is no problem.

markj added inline comments.

sbin/swapon/swapon.c
749 ↗	(On Diff #60050)	I'm referring here to the use of "consumer." But that's really just pedantic. In practice the swap device is almost always a GEOM device.