vfsmount gutting
Subsystems (3)
Al Viro
maintainerRFC PATCH 1/1 vfsmount gutting
OK, that's something I wanted to do for a long time - struct vfsmount
contains too much stuff that is strictily VFS-internal and not needed by anything outside of a very small subset of VFS, at that. As the matter of fact, only 3 fields, 1 of them redundant, are used by code outside of that area - it's mnt_flags ("how it's mounted"), mnt_root ("what dentry tree is mounted here", assign-once thing) and mnt_sb (always equal to mnt_root->d_sb, also assign-once, might or might not be not needed).
What I've done in vfs.git#vfsmount-guts is massive trimming of
struct vfsmount. It's a long massage session for fs/namespace.c and friends. Here's what happens there:
* struct vfsmount is embedded into a VFS-private structure. The
best name I'd been able to come up with was struct mount and yes, it would be more natural the other way round. Since all instances of struct vfsmount come from alloc_vfsmnt(), it's safe - they can't be static, they can't be on stack, they can't be embedded into other structures or arrays.
* said structure is defined in fs/mounts.h. All fields of struct
vfsmount except for these 3 had been moved to struct mount. Very few files include that one.
* fs/namespace.c and fs/pnode.c internals are dealing with struct
mount * instead of struct vfsmount *; the public API is unchanged and still gets struct vfsmount *, of course. We use container_of() to get to the corresponding struct mount * (see real_mount() in fs/mounts.h).
* ->mnt_parent and ->mnt_master point to struct mount *; it's
more convenient that way.
* struct mnt_namespace became opaque - its definition is in the
same fs/mounts.h. ->root points to struct mount * now (that, BTW, is what had lead to grep over the tree that found breakage in apparmor).
* proc/*/mounts-related code got moved to separate file -fs/proc_namespace.c; essentially it's a part of procfs, but its ties to fs/namespace.c are much stronger than those to the rest of fs/proc/*. Interface boundary shifted a bit, with IMO cleaner separation as the result.
Filesystem code is unchanged; it still gets pointers to struct
vfsmount *, it's just that this structure is less cluttered now and IMO it makes a lot more sense that way.
What is missing:
1) Review. As far as I can see on local testing, the thing works, but extra eyes would obviously be very welcome.
2) Documentation of the vfsmount ex-guts. This stuff is dealt with by very compact area now, so it really can be sanely documented. In progress...
3) layout optimizations wrt holes, cacheline boundaries, etc. It's not too far from what we used to have before, but it'll need to be dealt with.
4) *possibly* a removal of ->mnt_sb. I'm not sure about that one; it is redundant (for all vfsmounts ->mnt_sb == ->mnt_root->d_sb and they are assigned at the same point, before anything can see that vfsmount) and it's not as if it had been used on a lot of hot paths, but... Note, BTW, that in a lot of cases we have path->dentry->d_sb spelled as path->mnt->mnt_sb; these don't really need ->mnt_sb, of course. Another class of users is something along the lines of configfs_mount->mnt_sb->s_root. What it wants to be is configfs_mount->mnt_root - these internal vfsmounts have roots set to that fs in question, so that's a straightforward optimization. Hell knows... It's a separate series in any case.
I'm going to put that series into -next; if anybody sees problems
with it, please yell.
I'm *NOT* mailbombing fsdevel with that - it's a long series and
if somebody wants a private mailbomb, that can always be arranged. Or you could grab git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
#vfsmount-guts and use git format-patch locally...
Al, off to review Miklos' series now...
Quoted 5 lines · Al Viro
I'm *NOT* mailbombing fsdevel with that - it's a long series and if somebody wants a private mailbomb, that can always be arranged. Or you could grab
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git
#vfsmount-guts and use git format-patch locally...
gitweb is also not horrible for things like this. You can point your browser at
http://git.kernel.org/?p=linux/kernel/git/viro/vfs.git;a=shortlog;h=refs/heads/vfsmount-guts
and click on the top-most commitdiff. Then you can see follow the rest by just clicking on "parent", and you get the nicely colorized diff with changelog entries etc. Or you can pick-and-choose based on shortlog name.
So linear series like these can be looked at even without direct git access. Sure, you'll get that "generating.." thing if the cache is cold, and it all depends on how fast the net is, but it's not necessarily any worse than being mail-bombed.
Linus
Quoted 7 lines · Al Viro
OK, that's something I wanted to do for a long time - struct vfsmount
contains too much stuff that is strictily VFS-internal and not needed by anything outside of a very small subset of VFS, at that. As the matter of fact, only 3 fields, 1 of them redundant, are used by code outside of that area - it'smnt_flags("how it's mounted"),mnt_root("what dentry tree is mounted here", assign-once thing) andmnt_sb(always equal tomnt_root->d_sb, also assign-once, might or might not be not needed).
If only these 3 fields are used, why not make the definition of struct vfsmount entirely hidden, and add accessor functions for those three?
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Al Viro
maintainerQuoted 11 lines · Matthew Wilcox
Quoted 7 lines · Al Viro
OK, that's something I wanted to do for a long time - struct vfsmount
contains too much stuff that is strictily VFS-internal and not needed by anything outside of a very small subset of VFS, at that. As the matter of fact, only 3 fields, 1 of them redundant, are used by code outside of that area - it'smnt_flags("how it's mounted"),mnt_root("what dentry tree is mounted here", assign-once thing) andmnt_sb(always equal tomnt_root->d_sb, also assign-once, might or might not be not needed).If only these 3 fields are used, why not make the definition of struct vfsmount entirely hidden, and add accessor functions for those three?
Code churn from hell, for starters. And cost of non-inlined calls on some fairly sensitive paths, but that's secondary.
It might turn out to be feasible, but the price is likely to include several method prototype changes affecting filesystems all over the place. E.g. I really suspect that ->show_options() taking vfsmount had been a mistake. Mine, at that. It ought to take root dentry of (sub)tree instead. No, passing struct super_block * will _not_ work; the things that share superblock between several mounts really want to know e.g. which of the snapshots it is (nilfs2), etc. The same probably goes for the rest of ->show...() methods.
Hell knows; if we end up in a place where what you said would be doable - great, but I'd prefer to pull what can be pulled without messing with fs code first. Besides, this separation really makes sense - it's "what part of fs this vfsmount refers to" vs. "how vfsmounts are connected into mount trees".
James Morris
maintainerQuoted 3 lines
What is missing:
1) Review. As far as I can see on local testing, the thing works, but extra eyes would obviously be very welcome.
I've been through the code and not seen any issues, and I'm also running it on my dev box with SELinux on, with no apparent problems.
If anyone has the SELinux testsuite set up, it would be good to run it with these patches applied.
- James
Signature · 2 lines
James Morris <[email protected]>
Tetsuo Handa
maintainerTried vfs.git#vfsmount-guts as of f939b0ff "vfs: trim includes a bit". Regarding TOMOYO part, I got one build error and got no runtime problems.
[PATCH] TOMOYO: Fix build error.
Include linux/magic.h for PROC_SUPER_MAGIC and SOCKFS_MAGIC.
security/tomoyo/realpath.c +1 -0
@@ -5,6 +5,7 @@*/ #include "common.h" +#include <linux/magic.h> /** * tomoyo_encode2 - Encode binary string to ascii string.