small pathname lookup cleanups

small pathname lookup cleanups

[Christoph Hellwig]

Hi Al,

this series contains a few minor lookup cleanups.

[PATCH 1/4] fs: remove the unused fs_lookup_param function

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/filesystems/mount_api.rst | 18 +-------
 fs/fs_parser.c                          | 56 -------------------------
 include/linux/fs_parser.h               |  5 ---
 3 files changed, 2 insertions(+), 77 deletions(-)

diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
index 29c169c68961f3..dbff847986da47 100644
--- a/Documentation/filesystems/mount_api.rst
+++ b/Documentation/filesystems/mount_api.rst
@@ -254,8 +254,8 @@ manage the filesystem context.  They are as follows:
      will have been weeded out and fc->sb_flags updated in the context.
      Security options will also have been weeded out and fc->security updated.
 
-     The parameter can be parsed with fs_parse() and fs_lookup_param().  Note
-     that the source(s) are presented as parameters named "source".
+     The parameter can be parsed with fs_parse().  Note that the source(s) are
+     presented as parameters named "source".
 
      If successful, 0 should be returned or a negative error code otherwise.
 
@@ -809,17 +809,3 @@ process the parameters it is given.
      If the parameter isn't matched, -ENOPARAM will be returned; if the
      parameter is matched, but the value is erroneous, -EINVAL will be
      returned; otherwise the parameter's option number will be returned.
-
-   * ::
-
-       int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *value,
-			   bool want_bdev,
-			   struct path *_path);
-
-     This takes a parameter that carries a string or filename type and attempts
-     to do a path lookup on it.  If the parameter expects a blockdev, a check
-     is made that the inode actually represents one.
-
-     Returns 0 if successful and ``*_path`` will be set; returns a negative
-     error code if not.
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index ab53e42a874aaa..ee40f838b2be91 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -133,62 +133,6 @@ int __fs_parse(struct p_log *log,
 }
 EXPORT_SYMBOL(__fs_parse);
 
-/**
- * fs_lookup_param - Look up a path referred to by a parameter
- * @fc: The filesystem context to log errors through.
- * @param: The parameter.
- * @want_bdev: T if want a blockdev
- * @_path: The result of the lookup
- */
-int fs_lookup_param(struct fs_context *fc,
-		    struct fs_parameter *param,
-		    bool want_bdev,
-		    struct path *_path)
-{
-	struct filename *f;
-	unsigned int flags = 0;
-	bool put_f;
-	int ret;
-
-	switch (param->type) {
-	case fs_value_is_string:
-		f = getname_kernel(param->string);
-		if (IS_ERR(f))
-			return PTR_ERR(f);
-		put_f = true;
-		break;
-	case fs_value_is_filename:
-		f = param->name;
-		put_f = false;
-		break;
-	default:
-		return invalf(fc, "%s: not usable as path", param->key);
-	}
-
-	f->refcnt++; /* filename_lookup() drops our ref. */
-	ret = filename_lookup(param->dirfd, f, flags, _path, NULL);
-	if (ret < 0) {
-		errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name);
-		goto out;
-	}
-
-	if (want_bdev &&
-	    !S_ISBLK(d_backing_inode(_path->dentry)->i_mode)) {
-		path_put(_path);
-		_path->dentry = NULL;
-		_path->mnt = NULL;
-		errorf(fc, "%s: Non-blockdev passed as '%s'",
-		       param->key, f->name);
-		ret = -ENOTBLK;
-	}
-
-out:
-	if (put_f)
-		putname(f);
-	return ret;
-}
-EXPORT_SYMBOL(fs_lookup_param);
-
 int fs_param_bad_value(struct p_log *log, struct fs_parameter *param)
 {
 	return inval_plog(log, "Bad value for '%s'", param->key);
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index aab0ffc6bac67a..a62ed20fda6d98 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -73,11 +73,6 @@ static inline int fs_parse(struct fs_context *fc,
 	return __fs_parse(&fc->log, desc, param, result);
 }
 
-extern int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *param,
-			   bool want_bdev,
-			   struct path *_path);
-
 extern int lookup_constant(const struct constant_table tbl[], const char *name, int not_found);
 
 #ifdef CONFIG_VALIDATE_FS_PARSER
-- 
2.28.0

[PATCH 2/4] fs: mark filename_lookup static

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/internal.h | 2 --
 fs/namei.c    | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 10517ece45167f..695e12bc285061 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -71,8 +71,6 @@ extern int finish_clean_context(struct fs_context *fc);
 /*
  * namei.c
  */
-extern int filename_lookup(int dfd, struct filename *name, unsigned flags,
-			   struct path *path, struct path *root);
 extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
 			   const char *, unsigned int, struct path *);
 long do_rmdir(int dfd, struct filename *name);
diff --git a/fs/namei.c b/fs/namei.c
index e99e2a9da0f7de..7963f97a130442 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2351,7 +2351,7 @@ static int path_lookupat(struct nameidata *nd, unsigned flags, struct path *path
 	return err;
 }
 
-int filename_lookup(int dfd, struct filename *name, unsigned flags,
+static int filename_lookup(int dfd, struct filename *name, unsigned flags,
 		    struct path *path, struct path *root)
 {
 	int retval;
-- 
2.28.0

[PATCH 3/4] fs: pass a nameidata into filename_lookup

[Christoph Hellwig]

This allows keeping the LOOKUP_ROOT case for vfs_path_lookup entirely
out of the normal fast path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/namei.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 7963f97a130442..90e1cb008ae449 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2352,22 +2352,18 @@ static int path_lookupat(struct nameidata *nd, unsigned flags, struct path *path
 }
 
 static int filename_lookup(int dfd, struct filename *name, unsigned flags,
-		    struct path *path, struct path *root)
+		    struct path *path, struct nameidata *nd)
 {
 	int retval;
-	struct nameidata nd;
+
 	if (IS_ERR(name))
 		return PTR_ERR(name);
-	if (unlikely(root)) {
-		nd.root = *root;
-		flags |= LOOKUP_ROOT;
-	}
-	set_nameidata(&nd, dfd, name);
-	retval = path_lookupat(&nd, flags | LOOKUP_RCU, path);
+	set_nameidata(nd, dfd, name);
+	retval = path_lookupat(nd, flags | LOOKUP_RCU, path);
 	if (unlikely(retval == -ECHILD))
-		retval = path_lookupat(&nd, flags, path);
+		retval = path_lookupat(nd, flags, path);
 	if (unlikely(retval == -ESTALE))
-		retval = path_lookupat(&nd, flags | LOOKUP_REVAL, path);
+		retval = path_lookupat(nd, flags | LOOKUP_REVAL, path);
 
 	if (likely(!retval))
 		audit_inode(name, path->dentry,
@@ -2450,8 +2446,10 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
 
 int kern_path(const char *name, unsigned int flags, struct path *path)
 {
-	return filename_lookup(AT_FDCWD, getname_kernel(name),
-			       flags, path, NULL);
+	struct nameidata nd;
+
+	return filename_lookup(AT_FDCWD, getname_kernel(name), flags, path,
+			       &nd);
 }
 EXPORT_SYMBOL(kern_path);
 
@@ -2467,10 +2465,12 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 		    const char *name, unsigned int flags,
 		    struct path *path)
 {
-	struct path root = {.mnt = mnt, .dentry = dentry};
-	/* the first argument of filename_lookup() is ignored with root */
+	struct nameidata nd;
+
+	nd.root.mnt = mnt;
+	nd.root.dentry = dentry;
 	return filename_lookup(AT_FDCWD, getname_kernel(name),
-			       flags , path, &root);
+			       flags | LOOKUP_ROOT, path, &nd);
 }
 EXPORT_SYMBOL(vfs_path_lookup);
 
@@ -2643,8 +2643,10 @@ int path_pts(struct path *path)
 int user_path_at_empty(int dfd, const char __user *name, unsigned flags,
 		 struct path *path, int *empty)
 {
+	struct nameidata nd;
+
 	return filename_lookup(dfd, getname_flags(name, flags, empty),
-			       flags, path, NULL);
+			       flags, path, &nd);
 }
 EXPORT_SYMBOL(user_path_at_empty);
 
-- 
2.28.0

[PATCH 4/4] fs: simplify vfs_path_lookup

[Christoph Hellwig]

vfs_path_lookup is only used to lookup mount points.  So drop the dentry
parameter that is always set to mnt->mnt_root, remove the unused export
and rename the function to mount_path_lookup.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/internal.h  |  4 ++--
 fs/namei.c     | 16 +++-------------
 fs/namespace.c |  8 +++-----
 3 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index 695e12bc285061..bbdae2648f6b7d 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -71,8 +71,8 @@ extern int finish_clean_context(struct fs_context *fc);
 /*
  * namei.c
  */
-extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
-			   const char *, unsigned int, struct path *);
+int mount_path_lookup(struct vfsmount *mnt, const char *name,
+		      unsigned int flags, struct path *path);
 long do_rmdir(int dfd, struct filename *name);
 long do_unlinkat(int dfd, struct filename *name);
 int may_linkat(struct path *link);
diff --git a/fs/namei.c b/fs/namei.c
index 90e1cb008ae449..30f7caf5eda79b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2453,26 +2453,16 @@ int kern_path(const char *name, unsigned int flags, struct path *path)
 }
 EXPORT_SYMBOL(kern_path);
 
-/**
- * vfs_path_lookup - lookup a file path relative to a dentry-vfsmount pair
- * @dentry:  pointer to dentry of the base directory
- * @mnt: pointer to vfs mount of the base directory
- * @name: pointer to file name
- * @flags: lookup flags
- * @path: pointer to struct path to fill
- */
-int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
-		    const char *name, unsigned int flags,
-		    struct path *path)
+int mount_path_lookup(struct vfsmount *mnt, const char *name,
+		unsigned int flags, struct path *path)
 {
 	struct nameidata nd;
 
 	nd.root.mnt = mnt;
-	nd.root.dentry = dentry;
+	nd.root.dentry = mnt->mnt_root;
 	return filename_lookup(AT_FDCWD, getname_kernel(name),
 			       flags | LOOKUP_ROOT, path, &nd);
 }
-EXPORT_SYMBOL(vfs_path_lookup);
 
 static int lookup_one_len_common(const char *name, struct dentry *base,
 				 int len, struct qstr *this)
diff --git a/fs/namespace.c b/fs/namespace.c
index bae0e95b3713a3..0e904b27f7baeb 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3368,9 +3368,8 @@ struct dentry *mount_subtree(struct vfsmount *m, const char *name)
 	ns->mounts++;
 	list_add(&mnt->mnt_list, &ns->list);
 
-	err = vfs_path_lookup(m->mnt_root, m,
-			name, LOOKUP_FOLLOW|LOOKUP_AUTOMOUNT, &path);
-
+	err = mount_path_lookup(m, name, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
+				&path);
 	put_mnt_ns(ns);
 
 	if (err)
@@ -4060,8 +4059,7 @@ static int mntns_install(struct nsset *nsset, struct ns_common *ns)
 	nsproxy->mnt_ns = mnt_ns;
 
 	/* Find the root */
-	err = vfs_path_lookup(mnt_ns->root->mnt.mnt_root, &mnt_ns->root->mnt,
-				"/", LOOKUP_DOWN, &root);
+	err = mount_path_lookup(&mnt_ns->root->mnt, "/", LOOKUP_DOWN, &root);
 	if (err) {
 		/* revert to old namespace */
 		nsproxy->mnt_ns = old_mnt_ns;
-- 
2.28.0

Re: [PATCH 4/4] fs: simplify vfs_path_lookup

[Al Viro]

On Sat, Sep 26, 2020 at 11:20:51AM +0200, Christoph Hellwig wrote:
> vfs_path_lookup is only used to lookup mount points.  So drop the dentry
> parameter that is always set to mnt->mnt_root, remove the unused export
> and rename the function to mount_path_lookup.

NAK.  It's a general-purpose API and simplifications are not worth bothering
with.

Re: [PATCH 3/4] fs: pass a nameidata into filename_lookup

[Al Viro]

On Sat, Sep 26, 2020 at 11:20:50AM +0200, Christoph Hellwig wrote:
> This allows keeping the LOOKUP_ROOT case for vfs_path_lookup entirely
> out of the normal fast path.

... saving you all of a if (unlikely(root)) {....} on it.  Not worth
it, and
> +	struct nameidata nd;
> +
> +	return filename_lookup(AT_FDCWD, getname_kernel(name), flags, path,
> +			       &nd);

is something I would rather avoid on the general principles - better have
set_nameidata()/restore_nameidata() done always to a local variable.  Makes
for somewhat easier analysis.

IOW, NAK

Re: [PATCH 1/4] fs: remove the unused fs_lookup_param function

[Al Viro]

On Sat, Sep 26, 2020 at 11:20:48AM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/filesystems/mount_api.rst | 18 +-------
>  fs/fs_parser.c                          | 56 -------------------------
>  include/linux/fs_parser.h               |  5 ---
>  3 files changed, 2 insertions(+), 77 deletions(-)
> 
> diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
> index 29c169c68961f3..dbff847986da47 100644
> --- a/Documentation/filesystems/mount_api.rst
> +++ b/Documentation/filesystems/mount_api.rst
> @@ -254,8 +254,8 @@ manage the filesystem context.  They are as follows:
>       will have been weeded out and fc->sb_flags updated in the context.
>       Security options will also have been weeded out and fc->security updated.
>  
> -     The parameter can be parsed with fs_parse() and fs_lookup_param().  Note
> -     that the source(s) are presented as parameters named "source".
> +     The parameter can be parsed with fs_parse().  Note that the source(s) are
> +     presented as parameters named "source".

Umm...  Not sure - I'm not too fond of fs_lookup_param(), but AFAIK there are
efforts to convert more filesystems to new mount API and I don't know if any
of those are currently using it - this work has moved to individual fs trees
now, so it's hard to keep track of.

Generally I would say "if it's out of tree, it's not our problem", but the new
API is fairly recent and conversions of in-tree filesystems aree still in
progress.  And they are likely to stay in topical branches in the regular
git trees of those in-tree filesystems while they are being developed, so
right now I would rather be careful with removals in that area, short of
serious problem with the primitive itself.  Being able to make lookup_filename()
static is obviously nice, but considering the changes done later in the series...
Let's hold it back for now.