Page MenuHomeFreeBSD

Enhance mdmfs(8) to work with tmpfs(5)
ClosedPublic

Authored by ian on Sep 10 2017, 1:50 AM.

Details

Summary

Existing scripts and associated config such as rc.initdiskless, rc.d/var, and others, use mdmfs to create memory filesystems. That program accepts a size argument which allows SI suffixes and treats an unsuffixed number as a count of 512 byte sectors. That makes it difficult to convert existing scripts to use tmpfs instead of mdmfs, because tmpfs treats unsuffixed numbers as a count of bytes. The script logic to deal with existing user config that might include suffixed and unsuffixed numbers is... unpleasant.

Also, there is no g'tee that tmpfs will be available. It is sometimes configured out of small-resource embedded systems to save memory and flash storage space.

These changes enhance mdmfs(8) so that it accepts two new values for the 'md-device' arg: 'tmpfs' and 'auto'. With tmpfs, the program always uses tmpfs(5) (and fails if it's not available). With 'auto' the program prefers tmpfs, but falls back to using md(4) if tmpfs isn't available. It also handles the -s <size> argument so that the mdconfig interpetation of unsuffixed numbers applies when tmpfs is used as well, so that existing user config keeps working after a switch to tmpfs.

A new rc setting, mfs_type, is added to etc/defaults/rc.conf to let users force the use of tmpfs or md; the default value is "auto".

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

ian created this revision.Sep 10 2017, 1:50 AM
ian added a comment.Sep 10 2017, 1:55 AM

I am proposing this an as alternate solution to https://reviews.freebsd.org/D11765

I developed this back in 2014, but it was dismissed as unnecessary when I asked for comment on it, so I mothballed it. I've dusted it off and re-tested it, still seems to work fine.

brooks added a subscriber: brooks.Sep 11 2017, 4:22 PM

This seems like a clean solution. The only downside I see is that you'd always need mdmfs(8) around.

ian added a comment.Sep 13 2017, 5:58 PM

This seems like a clean solution. The only downside I see is that you'd always need mdmfs(8) around.

The rc system has assumed the presence of mdmfs since 2004 (r127345) when you added mount_md() to rc.subr. That's part of what inspired me to make it the unifying tool.

stevek added inline comments.Sep 13 2017, 7:34 PM
etc/rc.initdiskless
201 ↗(On Diff #32867)

Would it not be better to do the following in case someone builds a platform that is not using etc/defaults/rc.conf?
/sbin/mdmfs $flags -s $1 ${mfs_type:-auto} $2

etc/rc.subr
1793 ↗(On Diff #32867)

Same thing here:
/sbin/mdmfs $flags -s $1 ${mfs_type:-auto} $2

ian added inline comments.Sep 23 2017, 1:59 AM
etc/rc.initdiskless
201 ↗(On Diff #32867)

If that was a thing to worry about, wouldn't every script in etc/rc.d and rc.subr and others all have the :- logic built in to all the variables they use everywhere? It looks like only a few of the scripts do that now.

Technically, /etc/defaults/rc.conf is documented as being required. rc.conf(5) says that /etc/rc.conf is included by /etc/defaults/rc.conf (a clever way to ensure required defaults are in place), but the current implementation doesn't actually do it that way.

On the other hand, rc.initdiskless doesn't read rc.conf or defaults, so $mfs_type can't even be used here, it'll have to be hard-coded as auto.

stevek added inline comments.Sep 23 2017, 7:06 PM
etc/rc.initdiskless
201 ↗(On Diff #32867)

The rc.d scripts often do have defaults (and should, IMO, if they're written correctly), but it's in entries like the following (from etc/rc.d/sshd):

: ${sshd_rsa1_enable:="no"}
: ${sshd_rsa_enable:="yes"}
: ${sshd_dsa_enable:="no"}
: ${sshd_ecdsa_enable:="yes"}
: ${sshd_ed25519_enable:="yes"}

...although there could be a valid argument for requiring defaults to be in /etc/defaults/rc.conf and not in the rc.d scripts themselves. It would, at least, make things consistent, instead of what we currently have.

ngie accepted this revision.Sep 29 2017, 4:11 PM

Please commit this. We'll work out the nuance with the variables later

This revision is now accepted and ready to land.Sep 29 2017, 4:11 PM
ngie added inline comments.Sep 29 2017, 4:15 PM
etc/rc.initdiskless
202 ↗(On Diff #32867)

Don't assume rc.conf has been sourced in this script. Doing so breaks being able to call this script standalone, which someone might be doing already, FYI

ian marked an inline comment as done.Sep 29 2017, 10:11 PM
ian added inline comments.
etc/rc.initdiskless
202 ↗(On Diff #32867)

I'm going to commit it with ${mfs_type} changed to "auto", but I'm not going to bother to upload a new diff here first.

This revision was automatically updated to reflect the committed changes.
ian marked an inline comment as done.