From: ebiederm@xmission.com (Eric W. Biederman)
To: David Howells <dhowells@redhat.com>
Cc: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <149926824154.20611.6104595541055328700.stgit@warthog.procyon.org.uk>
Date: Wed, 05 Jul 2017 11:34:28 -0500
In-Reply-To: <149926824154.20611.6104595541055328700.stgit@warthog.procyon.org.uk>
        (David Howells's message of "Wed, 05 Jul 2017 16:24:01 +0100")
Message-ID: <87tw2qetbf.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [RFC][PATCH 00/14] VFS: Make all filesystems implement ->show_options()
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5070
Lines: 110

David Howells <dhowells@redhat.com> writes:

> Here's a set of patches that:
>
>  (1) Removes calls to save/replace_mount_options() where the filesystem
>      then implements ->show_options() anyway, ignoring ->s_options.
>
>  (2) Makes all filesystems implement the ->show_options() superblock
>      operation rather than using generic_show_options().  If necessary,
>      extra information is stored in the superblock information.
>
>  (3) Deletes save_mount_options(), replace_mount_options(),
>      generic_show_options() and super_block::s_options.
>
> This makes it easier to implement a context-based mount where the options
> are passed individually over a file descriptor.  It also allows duplicate
> options, options that override each other and ignored options to be
> resolved rather than storing irrelevant data.

This makes me a little nervous but is probably fine.  But we do need
to be careful with remount.  Today the rule is all options that need
to be preserved need to be passed to remount.  Passing options in one by
one looks like it may make it easy to get that confused while you are
developing your patches.

> Further, a lot of the time, all the information we want to display is
> stored in the super block information anyway, so the option string is
> redundant.
>
> Some things I noted whilst doing this:
>
>  (1) A number of filesystems take uid/gid options.  Should these be
>      reported relative to the observer's user namespace rather than init's
>      user namespace?  After all, you can't then use those uid/gid options
>      if the numbers are interpreted incorrectly if you try and forge a
>      mount command from them.

Options should be relative to the mount call.  Which is almost always
&init_user_ns and in the general case would be sb->s_user_ns.

Which I believe becomes the creator of the mount context in the model
you are working towards.

>  (2) Should I provide a helper for displaying uid/gid options?
>
>  (3) How much do we need to worry about racing with remount?  Some
>      filesystems happily give out the contents of the super_block without
>      regard to the fact that remount might be changing it simultaneously -
>      ext4, for example.
>
>  (4) What string options actually need 'munging'?
>
> These patches can be found here:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=for-viro
>
> on top of three other minor patches.
>
> David
> ---
> David Howells (14):
>       VFS: Don't use save/replace_mount_options if not using generic_show_options
>       hugetlbfs: Implement show_options
>       omfs: Implement show_options
>       pstore: Implement show_options
>       ramfs: Implement show_options
>       bpf: Implement show_options
>       spufs: Implement show_options
>       befs: Implement show_options
>       affs: Implement show_options
>       afs: Implement show_options
>       isofs: Implement show_options
>       9p: Implement show_options
>       orangefs: Implement show_options
>       VFS: Kill off s_options and helpers
>
>
>  Documentation/filesystems/vfs.txt         |    6 --
>  arch/powerpc/platforms/cell/spufs/inode.c |   21 +++++++--
>  fs/9p/v9fs.c                              |   59 ++++++++++++++++++++++++
>  fs/9p/v9fs.h                              |    3 +
>  fs/9p/vfs_super.c                         |    6 +-
>  fs/affs/super.c                           |   42 +++++++++++++++--
>  fs/afs/super.c                            |   45 ++++++++++++++++++-
>  fs/befs/linuxvfs.c                        |   24 +++++++++-
>  fs/btrfs/super.c                          |    1 
>  fs/debugfs/inode.c                        |    2 -
>  fs/efivarfs/super.c                       |    1 
>  fs/hugetlbfs/inode.c                      |   70 +++++++++++++++++++++++------
>  fs/isofs/inode.c                          |   51 ++++++++++++++++++++-
>  fs/isofs/isofs.h                          |    3 +
>  fs/namespace.c                            |   59 ------------------------
>  fs/omfs/inode.c                           |   33 ++++++++++++--
>  fs/orangefs/super.c                       |   15 ++++++
>  fs/pstore/inode.c                         |   14 +++++-
>  fs/pstore/internal.h                      |    3 +
>  fs/pstore/platform.c                      |    2 -
>  fs/ramfs/inode.c                          |   32 +++++++++----
>  fs/reiserfs/super.c                       |    4 --
>  fs/super.c                                |    1 
>  fs/tracefs/inode.c                        |    2 -
>  include/linux/fs.h                        |    9 ----
>  include/linux/hugetlb.h                   |    3 +
>  include/net/9p/client.h                   |   13 +++++
>  include/net/9p/transport.h                |    1 
>  kernel/bpf/inode.c                        |   16 +++++--
>  net/9p/client.c                           |   25 ++++++++++
>  net/9p/trans_fd.c                         |   31 ++++++++++++-
>  net/9p/trans_rdma.c                       |   31 ++++++++++++-
>  32 files changed, 481 insertions(+), 147 deletions(-)