Re: [PATCH 06/21] VFS: Introduce a superblock configuration context [ver #3]

From: David Howells
Date: Tue May 16 2017 - 12:33:28 EST


Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote:

> One way to split this large patch up into more managable chunks would be:
>
> 1) common infrastructure
> 2) new mount related changes
> 3) reconfig (remount) related changes
>
> Would that work?

The problem is that remount seems to generally use the same parsing code as
the new-mount entry point.

Before considering how to split it, can we consider whether to roll patches 20
and 21 into the preceding patches?

> (a) new mount with new super block created
> (b) new mount with existing super block reused
> (c) remount

(b) is internal-only at the moment, used by NFS submounts as triggered by
automounts. There isn't currently any way to supply mount options to this.

> 2) modify options ("foo" turns option on, "nofoo" turns it off)

Not all options are binary and some options may be mandatory.

> The surprising thing here is that we do (a) and (b) via the same route
> and (a) and (c) via a different ones. This doesn't feel right.

You need to look at it like this:

Case Options Ref Call Modify
super sget super
======= ======= ======= ======= =======
a Y - Y -
b - Y Y -
c Y [1] - Y

[1] We don't have a separate reference sb, only the one we're going to modify,
but we can preload the sb_config from that.

(a) and (b) have the same action.

> i) options that determine the sb instance (such as the blockdev or
> the server IP address)
> ii) subpath: this can determine the sb as well as the subtree to use
> iii) options that can be changed while sb in use
> iv) ???

Ah - but some of these options have to be set *inside* sget() or before the
superblock becomes live, even the ones that can be changed in-flight.

> Would it make sense to make the "new mount" case be
>
> A) find or create sb based on (i) and (ii) options
> B) reconfigure the resulting sb based on (iii) options

You would *have* to do the reconfiguration before making the superblock live
to prevent config/use races, and some options in (iii) may be required during
sget(), or even before you get as far as calling sget() (say you need to
access a server).

> This would make legacy new mount be: (A) + if new then (B). And
> legacy remount just (B).

It's not obvious that this is sufficiently equivalent from your brief
description.

> Also I think silently ignoring options is not always the right answer.

Example?

Do you mean like the NFS 'sloppy' option? I've noted that that might be best
handled in userspace.

> > + int (*remount_fs_sc) (struct super_block *, struct sb_config *);
>
> How about reconfig_fs() or just reconfig()?

Sure.

> > + (*) struct dentry *(*mount)(struct sb_config *sc);
>
> I'd be much happier with "get_root()" or something.

Changed in patch 21 to ->get_tree() as suggested by Al. Having looked over
the code, I'm tempted to change it back to ->mount() as being more obvious.

> > + err = parse_monolithic_mount_data(sc, data);
> > + if (err < 0)
> > + goto err_sc;
>
> If filesystem defines ->monolithic_mount_data() who is responsible for
> calling the security hook?

Which security hook? security_sb_remount()?

Note this code has changed in patch 20. I should update security_sb_remount()
to take an sb_config and call it in all paths.

> Largely duplicated do_new_mount_sc(). What's the point?

Legacy vs new. Fixed in patch 20.

> Lots of these are not superblock options, and should be moved over to
> the forbidden ones. Look at do_mount() for a hint.

I still have to support legacy mount option parsing. Do I actually see these
in legacy mount(2)? Or are they weeded out by mount(8)?

David