[PATCH 0/4] [RFC] New path lookup function

[PATCH 0/4] [RFC] New path lookup function

[Josef 'Jeff' Sipek]

Stackable file systems, among others, frequently need to lookup paths or
path components starting from an arbitrary point in the namespace
(identified by a dentry and a vfsmount).  Currently, such file systems use
lookup_one_len, which is frowned upon [1] as it does not pass the lookup
intent along; not passing a lookup intent, for example, can trigger BUG_ON's
when stacking on top of NFSv4.

The first patch introduces a new lookup function to allow lookup starting
from an arbitrary point in the namespace.  This approach has been suggested
by Christoph Hellwig [2].

The second patch changes sunrpc to use path_component_lookup.

The third patch changes nfsctl.c to use path_component_lookup.

The fourth, and last patch, unexports path_walk because it is no longer
unnecessary to call it directly, and using the new path_component_lookup is
cleaner.

For example, the following snippet of code, looks up "some/path/component"
in a directory pointed to by parent_{dentry,vfsmnt}:

err = path_component_lookup(parent_dentry, parent_vfsmnt,
			    "some/path/component", 0, &nd);
if (!err) {
	/* exits */

	...

	/* once done, release the references */
	path_release(&nd);
} else if (err == -ENOENT) {
	/* doesn't exist */
} else {
	/* other error */
}

VFS functions such as lookup_create can be used on the nameidata structure
to pass the create intent to the file system.

Currently, there is no easy way to pass the LOOKUP_OPEN intent.  The proper
way would be to call open_namei.

We'd like to get comments about what's necessary to make stackable file
systems do lookups right: this includes potential changes to open_namei.

Josef 'Jeff' Sipek.

[1] http://lkml.org/lkml/2007/3/9/95
[2] http://lkml.org/lkml/2007/5/4/51

[PATCH 1/4] fs: Introduce path_component_lookup

[Josef 'Jeff' Sipek]

Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
 fs/namei.c            |   26 ++++++++++++++++++++++++++
 include/linux/namei.h |    2 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 3449e0a..b547af0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1175,6 +1175,31 @@ int fastcall path_lookup(const char *name, unsigned int flags,
 	return do_path_lookup(AT_FDCWD, name, flags, nd);
 }
 
+int path_component_lookup(struct dentry *dentry, struct vfsmount *mnt,
+			  const char *name, unsigned int flags, 
+			  struct nameidata *nd)
+{
+	int retval;
+
+	/* same as do_path_lookup */
+	nd->last_type = LAST_ROOT;
+	nd->flags = flags;
+	nd->depth = 0;
+
+	nd->mnt = mntget(mnt);
+	nd->dentry = dget(dentry);
+
+	retval = path_walk(name, nd);
+	if (likely(retval == 0)) {
+		if (unlikely(!audit_dummy_context() && nd && nd->dentry && 
+				nd->dentry->d_inode))
+			audit_inode(name, nd->dentry->d_inode);
+	}
+
+	return retval;
+
+}
+
 static int __path_lookup_intent_open(int dfd, const char *name,
 		unsigned int lookup_flags, struct nameidata *nd,
 		int open_flags, int create_mode)
@@ -2799,6 +2824,7 @@ EXPORT_SYMBOL(__page_symlink);
 EXPORT_SYMBOL(page_symlink);
 EXPORT_SYMBOL(page_symlink_inode_operations);
 EXPORT_SYMBOL(path_lookup);
+EXPORT_SYMBOL(path_component_lookup);
 EXPORT_SYMBOL(path_release);
 EXPORT_SYMBOL(path_walk);
 EXPORT_SYMBOL(permission);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 0ab27ba..2247397 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -70,6 +70,8 @@ extern int FASTCALL(__user_walk_fd(int dfd, const char __user *, unsigned, struc
 #define user_path_walk_link(name,nd) \
 	__user_walk_fd(AT_FDCWD, name, 0, nd)
 extern int FASTCALL(path_lookup(const char *, unsigned, struct nameidata *));
+extern int path_component_lookup(struct dentry *, struct vfsmount *,
+				 const char *, unsigned int, struct nameidata *);
 extern int FASTCALL(path_walk(const char *, struct nameidata *));
 extern int FASTCALL(link_path_walk(const char *, struct nameidata *));
 extern void path_release(struct nameidata *);
-- 
1.5.0.3.1043.g4342

Re: [PATCH 1/4] fs: Introduce path_component_lookup

[Christoph Hellwig]

I wrote up the suggestion before my first morning tea yesterday
and must admit that the name path_component_lookup is pretty stupid.
We don't just look up a component but any relative path starting
from the vfsmount/dentry pair.  How about vfs_path_lookup instead
because it mirrors various other vfs_ function that are dentry based?

Also as a new exported symbol it should get a kerneldoc comment describing
it.

> +	if (likely(retval == 0)) {
> +		if (unlikely(!audit_dummy_context() && nd && nd->dentry && 
> +				nd->dentry->d_inode))
> +			audit_inode(name, nd->dentry->d_inode);
> +	}

This should get the same simplification I suggested for do_path_lookup.

[PATCH 2/4] sunrpc: Use path_component_lookup

[Josef 'Jeff' Sipek]

use path_component_lookup instead of open-coding the necessary
functionality.

Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
 net/sunrpc/rpc_pipe.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 9b9ea50..b67cb54 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -451,21 +451,19 @@ void rpc_put_mount(void)
 static int
 rpc_lookup_parent(char *path, struct nameidata *nd)
 {
+	struct vfsmount *mnt;
+
 	if (path[0] == '\0')
 		return -ENOENT;
-	nd->mnt = rpc_get_mount();
-	if (IS_ERR(nd->mnt)) {
+
+	mnt = rpc_get_mount();
+	if (IS_ERR(mnt)) {
 		printk(KERN_WARNING "%s: %s failed to mount "
 			       "pseudofilesystem \n", __FILE__, __FUNCTION__);
-		return PTR_ERR(nd->mnt);
+		return PTR_ERR(mnt);
 	}
-	mntget(nd->mnt);
-	nd->dentry = dget(rpc_mount->mnt_root);
-	nd->last_type = LAST_ROOT;
-	nd->flags = LOOKUP_PARENT;
-	nd->depth = 0;
 
-	if (path_walk(path, nd)) {
+	if (path_component_lookup(rpc_mount->mnt_root, mnt, path, LOOKUP_PARENT, nd)) {
 		printk(KERN_WARNING "%s: %s failed to find path %s\n",
 				__FILE__, __FUNCTION__, path);
 		rpc_put_mount();
-- 
1.5.0.3.1043.g4342

Re: [PATCH 2/4] sunrpc: Use path_component_lookup

[Trond Myklebust]

On Sat, 2007-05-05 at 19:09 -0400, Josef 'Jeff' Sipek wrote:
> use path_component_lookup instead of open-coding the necessary
> functionality.
> 
> Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
> ---
>  net/sunrpc/rpc_pipe.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 9b9ea50..b67cb54 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -451,21 +451,19 @@ void rpc_put_mount(void)
>  static int
>  rpc_lookup_parent(char *path, struct nameidata *nd)
>  {
> +	struct vfsmount *mnt;
> +
>  	if (path[0] == '\0')
>  		return -ENOENT;
> -	nd->mnt = rpc_get_mount();
> -	if (IS_ERR(nd->mnt)) {
> +
> +	mnt = rpc_get_mount();
> +	if (IS_ERR(mnt)) {
>  		printk(KERN_WARNING "%s: %s failed to mount "
>  			       "pseudofilesystem \n", __FILE__, __FUNCTION__);
> -		return PTR_ERR(nd->mnt);
> +		return PTR_ERR(mnt);
>  	}
> -	mntget(nd->mnt);
> -	nd->dentry = dget(rpc_mount->mnt_root);
> -	nd->last_type = LAST_ROOT;
> -	nd->flags = LOOKUP_PARENT;
> -	nd->depth = 0;
>  
> -	if (path_walk(path, nd)) {
> +	if (path_component_lookup(rpc_mount->mnt_root, mnt, path, LOOKUP_PARENT, nd)) {
>  		printk(KERN_WARNING "%s: %s failed to find path %s\n",
>  				__FILE__, __FUNCTION__, path);
>  		rpc_put_mount();

Just one minor nit: could you make that

	if (path_component_lookup(mnt->mnt_root, mnt, path, LOOKUP_PARENT, nd)) {

instead. It really is a bug even for the existing code to be referencing
rpc_mount directly.

Cheers
  Trond

[PATCH 1/1] sunrpc: Use path_component_lookup

[Josef 'Jeff' Sipek]

use path_component_lookup instead of open-coding the necessary
functionality.

Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
 net/sunrpc/rpc_pipe.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 9b9ea50..c93a454 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -451,21 +451,19 @@ void rpc_put_mount(void)
 static int
 rpc_lookup_parent(char *path, struct nameidata *nd)
 {
+	struct vfsmount *mnt;
+
 	if (path[0] == '\0')
 		return -ENOENT;
-	nd->mnt = rpc_get_mount();
-	if (IS_ERR(nd->mnt)) {
+
+	mnt = rpc_get_mount();
+	if (IS_ERR(mnt)) {
 		printk(KERN_WARNING "%s: %s failed to mount "
 			       "pseudofilesystem \n", __FILE__, __FUNCTION__);
-		return PTR_ERR(nd->mnt);
+		return PTR_ERR(mnt);
 	}
-	mntget(nd->mnt);
-	nd->dentry = dget(rpc_mount->mnt_root);
-	nd->last_type = LAST_ROOT;
-	nd->flags = LOOKUP_PARENT;
-	nd->depth = 0;
 
-	if (path_walk(path, nd)) {
+	if (path_component_lookup(mnt->mnt_root, mnt, path, LOOKUP_PARENT, nd)) {
 		printk(KERN_WARNING "%s: %s failed to find path %s\n",
 				__FILE__, __FUNCTION__, path);
 		rpc_put_mount();
-- 
1.5.2.rc1.20.g86b9

Re: [PATCH 1/1] sunrpc: Use path_component_lookup

[Trond Myklebust]

On Sat, 2007-05-05 at 20:52 -0400, Josef 'Jeff' Sipek wrote:
> use path_component_lookup instead of open-coding the necessary
> functionality.
> 
> Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
> ---
>  net/sunrpc/rpc_pipe.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 9b9ea50..c93a454 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -451,21 +451,19 @@ void rpc_put_mount(void)
>  static int
>  rpc_lookup_parent(char *path, struct nameidata *nd)
>  {
> +	struct vfsmount *mnt;
> +
>  	if (path[0] == '\0')
>  		return -ENOENT;
> -	nd->mnt = rpc_get_mount();
> -	if (IS_ERR(nd->mnt)) {
> +
> +	mnt = rpc_get_mount();
> +	if (IS_ERR(mnt)) {
>  		printk(KERN_WARNING "%s: %s failed to mount "
>  			       "pseudofilesystem \n", __FILE__, __FUNCTION__);
> -		return PTR_ERR(nd->mnt);
> +		return PTR_ERR(mnt);
>  	}
> -	mntget(nd->mnt);
> -	nd->dentry = dget(rpc_mount->mnt_root);
> -	nd->last_type = LAST_ROOT;
> -	nd->flags = LOOKUP_PARENT;
> -	nd->depth = 0;
>  
> -	if (path_walk(path, nd)) {
> +	if (path_component_lookup(mnt->mnt_root, mnt, path, LOOKUP_PARENT, nd)) {
>  		printk(KERN_WARNING "%s: %s failed to find path %s\n",
>  				__FILE__, __FUNCTION__, path);
>  		rpc_put_mount();

Thanks! That looks good.

Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Cheers
  Trond

[PATCH 3/4] nfsctl: Use path_component_lookup

[Josef 'Jeff' Sipek]

use path_component_lookup instead of open-coding the necessary
functionality.

Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
 fs/nfsctl.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/nfsctl.c b/fs/nfsctl.c
index c043136..2035dc7 100644
--- a/fs/nfsctl.c
+++ b/fs/nfsctl.c
@@ -23,19 +23,14 @@
 static struct file *do_open(char *name, int flags)
 {
 	struct nameidata nd;
+	struct vfsmount *mnt;
 	int error;
 
-	nd.mnt = do_kern_mount("nfsd", 0, "nfsd", NULL);
+	mnt = do_kern_mount("nfsd", 0, "nfsd", NULL);
+	if (IS_ERR(mnt))
+		return (struct file *)mnt;
 
-	if (IS_ERR(nd.mnt))
-		return (struct file *)nd.mnt;
-
-	nd.dentry = dget(nd.mnt->mnt_root);
-	nd.last_type = LAST_ROOT;
-	nd.flags = 0;
-	nd.depth = 0;
-
-	error = path_walk(name, &nd);
+	error = path_component_lookup(mnt->mnt_root, mnt, name, 0, &nd);
 	if (error)
 		return ERR_PTR(error);
 
-- 
1.5.0.3.1043.g4342

[PATCH 4/4] fs: Remove path_walk export

[Josef 'Jeff' Sipek]

Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
 fs/namei.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index b547af0..0262594 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2826,7 +2826,6 @@ EXPORT_SYMBOL(page_symlink_inode_operations);
 EXPORT_SYMBOL(path_lookup);
 EXPORT_SYMBOL(path_component_lookup);
 EXPORT_SYMBOL(path_release);
-EXPORT_SYMBOL(path_walk);
 EXPORT_SYMBOL(permission);
 EXPORT_SYMBOL(vfs_permission);
 EXPORT_SYMBOL(file_permission);
-- 
1.5.0.3.1043.g4342

Re: [PATCH 4/4] fs: Remove path_walk export

[Christoph Hellwig]

On Sat, May 05, 2007 at 07:09:34PM -0400, Josef 'Jeff' Sipek wrote:
> Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
> ---
>  fs/namei.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

it should be possible to make it static now aswell.  (the uml code
using it is #if0'ed and will never be resurrected because it's quite bogus).

Also link_path_walk can become static aswell.  It will need a forward
declaration though as it's used recursively.