Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 21 Sep 2018 17:54:56 +0200
From:   Christian Brauner <christian@brauner.io>
To:     David Howells <dhowells@redhat.com>
Cc:     Miklos Szeredi <mszeredi@redhat.com>,
        Al Viro <viro@ZenIV.linux.org.uk>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags
Message-ID: <20180921155455.flkf5vdrm3vgn6do@brauner.io>
References: <20180920151214.15484-6-mszeredi@redhat.com>
 <20180920151214.15484-1-mszeredi@redhat.com>
 <17157.1537542475@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <17157.1537542475@warthog.procyon.org.uk>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Sep 21, 2018 at 04:07:55PM +0100, David Howells wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
> 
> > What happens if we introduce new flags for fsmount(2) and are already out
> > of flags for mount(2)?  I see a big mess that way.
> > 
> > So let's instead start a clean new set, to be used in the new API.

If I may chime in. I think this is a really good idea.

> 
> If we must.  But let's not call them just M_* please.  Let's call them
> MOUNT_ATTR_* or something.
> 
> > The MS_RELATIME flag was accepted but ignored.  Simply leave this out of
> > the new set, since "relatime" is the default.
> 
> Can we make RELATIME, STRICTATIME and NOATIME an enum rather than individual
> flags?
> 
> 	#define MOUNT_ATTR_RDONLY	0x01
> 	#define MOUNT_ATTR_NOSUID	0x02
> 	#define MOUNT_ATTR_NODEV	0x04
> 	#define MOUNT_ATTR_NOEXEC	0x08
> 	#define MOUNT_ATTR_RELATIME	0x00
> 	#define MOUNT_ATTR_NOATIME	0x10
> 	#define MOUNT_ATTR_STRICTATIME	0x20
> 	#define MOUNT_ATTR_ATIME_MASK	0x30
> 	#define MOUNT_ATTR_NODIRATIME	0x40
> 
> We can also use these for a mount_setattr() syscall:

So a - maybe stupid - question I had when recently working on a patch
was how to add new mount properties in the new api.
What I would expect is that once the new mount API has landed the old
mount() syscall should not see any new features added since I take it
that we want userspace to very slowly over time have sufficient
incentive to only use the new mount API.

So from reading the patch I got the impression that superblock mount
options passed via fsconfig() are passed as strings like "ro" and are
translated into approriate objects (e.g. flags etc.) by the kernel. That
seems like a future proof mechanism to extend mount options for
userspace without having to worry about exceeding any integer types in
the future. Maybe this would make sense for non-superblock options as
well? I can see that this is less performant that checking flags and
string parsing is a thing that is not particularly nice but it would be
one option to solve the running out of flags problem.

> 
> 	mount_setattr(int dfd, const char *path, unsigned int atflags,
> 		      unsigned int attr_values,
> 		      unsigned int attr_mask);

If we go with the flag arguments wouldn't it make sense to use a larger
integer type?

> 
> where atflags can potentially include AT_RECURSIVE.

Very much in favor of having this operate recursively!

Christian