Page MenuHomeFreeBSD

simple trimming before swapon
ClosedPublic

Authored by dougm on Jun 11 2019, 6:07 AM.

Details

Summary

Inspired by trim.c, add a function to be called before swapon that will trim everything.

Test Plan

It compiles. I don't know how to test it.

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.Jun 11 2019, 6:07 AM
markj added a comment.Jun 11 2019, 4:41 PM

For testing, if you have a spare SSD available, it should be sufficient to just invoke "swapon <device>" from the command line. If not, I can help test it.

swapon.c
732 ↗(On Diff #58510)

Don't you need to open RDWR or WRONLY?

There should not be any need to use O_DIRECT here.

742 ↗(On Diff #58510)

I would find it clearer to have

off_t sz;
...
if (S_ISREG(sb.st_mode) {
    sz = sb.st_size;
} else if (S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode)) {
    if (ioctl(fd, DIOCGMEDIASIZE, &sz) != 0)
        err(1, "ioctl(DIOCGMEDIASIZE");
} else {
    errx(1, "%s has an invalid file type", name);
}

ioarg[0] = 0;
ioarg[1] = sz;
if (ioctl(fd, DIOCGDELETE, ioarg) != 0)
    warn(1, "ioctl(DIOCGDELETE)");

In particular, it does not make sense to swap on a directory.

I am not sure how we should handle errors. I think it is reasonable to log them and continue, i.e., using warn(3) as above.

754 ↗(On Diff #58510)

My feeling is that this should be explicitly configured by the administrator. It's not unusual for devices to have slow or buggy TRIM support.

dougm marked 3 inline comments as done.Jun 12 2019, 7:35 AM
dougm updated this revision to Diff 58554.

Apply reviewer suggestions. Muddle through trying to process a "swaponce" option in fstab.

markj added inline comments.Jun 13 2019, 4:17 PM
swapon.c
389 ↗(On Diff #58554)

Hmm, see the description of -T in geli(8). The combination of "notrim" and "trimonce" should probably result in an error.

395 ↗(On Diff #58554)

Style: continuing lines should be indented by four space.

741 ↗(On Diff #58554)

Why is O_DIRECT needed?

803 ↗(On Diff #58554)

I guess it should be

[-F fstab] -aLq | [-E] file ...

In the first type of usage we're getting the list of swap devices and options from the fstab, and in the second we're specifying the swap device on the command line. -E should only be available in the latter usage.

dougm marked 4 inline comments as done.Jun 13 2019, 8:23 PM
dougm updated this revision to Diff 58595.

Address reviewer comments.

markj added a comment.Jun 17 2019, 3:42 PM

The "trimonce" option should be documented in the fstab(5) man page, share/man/man5/fstab.5, and the -E option should be documented in the swapon(8) man page, sbin/swapon/swapon.8.

The syntax used for man pages is documented in the mdoc(7) page. It's helpful also to look at existing man pages for examples of mandoc syntax.

swapon.c
192 ↗(On Diff #58595)

The options are per-swap device and the fstab may describe multiple swap devices. This enables the "trimonce" option for all of them following the first one with "trimonce" configured.

742 ↗(On Diff #58595)

Extra newline.

749 ↗(On Diff #58595)

This should be a fatal error.

771 ↗(On Diff #58595)

Extra newline.

dougm updated this revision to Diff 58751.Jun 17 2019, 10:14 PM

Endeavor to apply reviewer suggestions.

markj added inline comments.Jun 19 2019, 5:51 PM
sbin/swapon/swapon.8
31 ↗(On Diff #58751)

.Dd should be bumped for non-trivial changes like adding a new option.

92 ↗(On Diff #58751)

Same comment as below regarding .Dv.

sbin/swapon/swapon.c
181 ↗(On Diff #58751)

Hmm, does this code just blindly look for option names without checking against partial matches? That will need to be fixed at some point if we later add a plain "trim" option for online trim.

191 ↗(On Diff #58751)

Doesn't this still leave Eflag set for all remaining swap devices in the fstab?

400 ↗(On Diff #58751)

We should handle this consistently with "late" and "noauto", i.e., have a separate else if statement, or combine all of the strcmp() calls into a single predicate.

share/man/man5/fstab.5
219 ↗(On Diff #58751)

I would suggest qualifying this: "For swap devices, the keyword..."

221 ↗(On Diff #58751)

Identifiers like BIO_DELETE should be marked with .Dv ("defined variable"):

... triggers the delivery of a
.Dv BIO_DELETE
command ...
dougm marked 8 inline comments as done.Jun 19 2019, 8:14 PM
dougm updated this revision to Diff 58804.

Addressed reviewer comments about these changes.

dougm added inline comments.Jun 21 2019, 3:37 AM
sbin/swapon/swapon.c
191 ↗(On Diff #58751)

No, it will be reset for every iteration of the getfsent loop - every swap device in the fstab, as I understand it. However, it could leave Eflag set for all the devices on the command line that are processed after the fstab is processed, so I'll use the 1 bit of Eflags for fstab trimming and the 2 bit for the rest.

markj added a comment.Jun 21 2019, 5:29 PM

With the nits I mentioned fixed, the patch seems to work. Thanks for doing this.

BTW, to build and install a new swapon, run:

$ cd /usr/src/sbin/swapon
$ make cleandir all
$ sudo make install

Or from /usr/src:

$ make -C sbin/swapon cleandir all
$ sudo make -C sbin/swapon install
sbin/swapon/swapon.c
191 ↗(On Diff #58751)

Ok, I was looking at an older version of the diff.

757 ↗(On Diff #58804)

You'll need to #include <sys/disk.h>

764 ↗(On Diff #58804)

warn() doesn't take an exit status parameter.

dougm updated this revision to Diff 58870.Jun 21 2019, 6:34 PM

Add a header. Drop an extra argument to warn().

markj accepted this revision.Jun 21 2019, 7:51 PM
This revision is now accepted and ready to land.Jun 21 2019, 7:51 PM