Page MenuHomeFreeBSD

improve swapon comments
ClosedPublic

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

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dougm created this revision.Jul 23 2019, 3:15 AM
markj added inline comments.Jul 23 2019, 2:05 PM
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?

dougm added inline comments.Jul 23 2019, 3:18 PM
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?

markj added inline comments.Jul 23 2019, 3:32 PM
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."

dougm updated this revision to Diff 60050.Jul 23 2019, 3:55 PM

Consume reviewer suggestions.

markj accepted this revision.Jul 23 2019, 3:57 PM
This revision is now accepted and ready to land.Jul 23 2019, 3:57 PM
rpokala added inline comments.Jul 24 2019, 6:03 AM
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.

dougm added inline comments.Jul 24 2019, 8:01 AM
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.

markj added inline comments.Jul 24 2019, 2:38 PM
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().

dougm updated this revision to Diff 60146.Jul 25 2019, 7:32 PM

In one comment, connection -> descriptor.

This revision now requires review to proceed.Jul 25 2019, 7:32 PM
markj added inline comments.Jul 26 2019, 1:28 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.

dougm added inline comments.Jul 26 2019, 3:02 PM
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 accepted this revision.Jul 26 2019, 3:08 PM
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.

This revision is now accepted and ready to land.Jul 26 2019, 3:08 PM
This revision was automatically updated to reflect the committed changes.