Page MenuHomeFreeBSD

Replace mount(2) calls to in-tree filesystems will nmount(2).
Needs ReviewPublic

Authored by brooks on Jun 21 2019, 3:15 AM.

Details

Reviewers
kib
markj
jhb
Summary

Most in-tree filesystems support both mount(2) and nmount(2). The
mount(2) API is difficult to translate for non-native ABIs (e.g.,
freebsd32). It is also nearly unused in the base system (amd(8) is the
only consumer).

This change adds translations from the mount(2) API to the nmount(2)
API for known filesystems. Where a translation is unavailable, it
falls back to calling the mount(2) system call.

This is intended to be a step towards allow new ABIs to not support
mount(2) while retaining most functionality.

This patch was developed by (Oleg Derevenetz) with review and minor
changes by me.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25181
Build 23868: arc lint + arc unit

Event Timeline

brooks created this revision.Jun 21 2019, 3:15 AM

I'd eventually like to move mount(2) under COMPATXX, but that's probably a long road. I've talked to one storage vendor who hasn't migrated their internal filesystem to nmount yet which suggests there are more.

kib added a comment.Jun 22 2019, 6:08 PM

There is a lot of style(9) violations, and I stopped listing at some point. Each thing I noted is recurrent.

From the description, it is not clear to me why do we need to add this to libc. Is this to allow to run amd(8) ? If yes, it is going out, I hope. Otherwise, the only use of mount(2) is to provide compat for old userspace (old enough to predate versioned libc), which cannot take advantage of the new libc in any case. I think that some vendor that still uses mount(2) internally have to fix the use on his own.

What I dislike from this commit is that the translations from mount(2) to nmount(2) occur in two places now.

lib/libc/sys/mount.c
57

The '{' should be on the previous line.

67

The use of spaces is wrong.

77

The number of blank lines in this function is same as non-blank lines. I think none of the blank lines, except after locals declarations, are needed.

78

Why 4 ?

102

Why 2 ? IOV, add comment that we are adding name and value.

195

We do not put tab after type in local decl.

200

{} are excessive.

230

{}

241

Again tabs/spaces before '='.

310

() are excessive.

421

static ?

444

I think using nitems() is better than NULL-terminate static array.

463

Why do you use _nmount there but __sys_mount later ?

If we can actually get rid of amd that would be sufficient for my local uses, but I doubt it's the only consumer.

I've been unable to find a good way to audit ports for mount() use so I don't know how many consumers there are. I suppose I could request and exp-run with the prototype and symbols removed to see. To the extent that there are consumers, this helps with freebsd32 (or in a CHERI world freebsd64) because there are fewer structs that require translation.

I'm not especially concerned about duplicating this logic. It's not something we'll need to maintain since we're not going to be adding more mount() filesystems to the tree.

brooks updated this revision to Diff 59165.Jun 28 2019, 6:48 PM
  • Explain magic numbers in comments.
  • Make supported_fs static.
  • Use nitems() rather then a NULL terminated array.

I've addressed the main technical concerns (other then which syscall implementation to call). If we agree to move forward with this patch I can address style(9), but don't want to spend the time unless it's going to land in FreeBSD.

lib/libc/sys/mount.c
463

In the case of _nmount, my (possibly misguided) understanding is that we generally call sys calls by their _ version inside libc. This was originally nmount. The __sys_mount call is to call the syscall directly without any interposition.

kib added a comment.Jun 29 2019, 6:40 PM

I've addressed the main technical concerns (other then which syscall implementation to call). If we agree to move forward with this patch I can address style(9), but don't want to spend the time unless it's going to land in FreeBSD.

It is strange to have such non-trivial emulation both in libc and in kernel. I would not object strongly against adding this code, but my personal opinion is that we better get rid of amd instead.

lib/libc/sys/mount.c
463

This code is the libc interposer for a syscall, and intent is to make the syscall. I would expect this place to use direct syscall stub.

brooks updated this revision to Diff 59320.Jul 2 2019, 6:06 PM
  • Call __sys_nmount from the mount wrapper.