Re: [PATCH] Add MS_BIND_FLAGS mount flag

[Christoph Hellwig <hch@infradead.org>]

On Tue, Feb 12, 2008 at 09:45:15PM -0800, Paul Menage wrote:
> From: Paul Menage <menage@google.com>
>
> Add a new mount() flag, MS_BIND_FLAGS.
>
> MS_BIND_FLAGS indicates that a bind mount should take its per-mount flags
> from the arguments passed to mount() rather than from the source
> mountpoint.
>
> This flag allows you to create a bind mount with the desired per-mount
> flags in a single operation, rather than having to do a bind mount
> followed by a remount, which is fiddly and can block for non-trivial
> periods of time (on sb->s_umount?).
>
> For recursive bind mounts, only the root of the tree being bound
> inherits the per-mount flags from the mount() arguments; sub-mounts
> inherit their per-mount flags from the source tree as usual.

I think this concept is reasonable, but I don't think MS_BIND_FLAGS
is a descriptive name for this flag.  MS_EXPLICIT_FLAGS might be better
but still isn't optimal.

> +static struct vfsmount *
> +__copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> +	    int flag, int mnt_flags)

> +struct vfsmount *copy_tree(struct vfsmount *mnt, struct dentry *dentry,
> +			   int flag)
> +{
> +	return __copy_tree(mnt, dentry, flag, mnt->mnt_flags);
> +}

Please just change copy_tree to have an additional parameter.  There's
just four callers of it in the tree, and three of them want the new
parameter.

> +	/* Use the source mount flags unless the user passed MS_BIND_FLAGS */

I think this comment could be made a little more explicit.

	/*
	 * If MS_EXPLICIT_FLAGS is passed in we will take the paramters
	 * the user has specified.  The default behaviour for bind
	 * mounts is however to take the flags from existing mount
	 * instances for the same superblock.
	 */
	 
> #define MS_RELATIME	(1<<21)	/* Update atime relative to mtime/ctime. */
> #define MS_KERNMOUNT	(1<<22) /* this is a kern_mount call */
> #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
> +#define MS_BIND_FLAGS	(1<<24) /* For MS_BIND, take mnt_flags from
> +				 * args, not from source mountpoint */
> #define MS_ACTIVE	(1<<30)
> #define MS_NOUSER	(1<<31)

this looks like whitespace damage to me.