Hi
This is a patch for link_path_walk cleanup based on the IRC disscussion with
Al Viro and I hope it to be pretty close to his thinking. The code is more
readable now. The cleanup is done so as to implement Hanna Linder's
fast_path_walk work more easily. Basically what I am doing is based
on the following main points:
path_walk
- calls the helper lookup_parent()
- handles the last component if needed by calling walk_one (inline)
lookup_parent (it is the lookup_parent case of old link_path_walk)
- for each component of the path (except the last one)
- calculates the hash value
- calls walk_one (inline)
walk_one
- checks for . & ..
- does actual lookup
- check mount points
- does the symlink resolution
I have kept the logic for walk_one similar to my first attempt that is to
handle one path component, doesnot matter even if it is last component. I have
tested the patch with ltp-20020307.
Please comment.
Thanks
Maneesh
link_path_walk-2.5.10.patch
diff -urN linux-2.5.10/fs/namei.c linux-2.5.10-lpw/fs/namei.c
--- linux-2.5.10/fs/namei.c Wed Apr 24 12:45:16 2002
+++ linux-2.5.10-lpw/fs/namei.c Fri Apr 26 16:41:29 2002
@@ -442,6 +442,91 @@
;
}
+static inline int walk_one(struct nameidata *nd)
+{
+ int err = 0;
+ struct dentry * dentry;
+ struct inode * inode;
+
+ /*
+ * "." and ".." are special - ".." especially so because it has
+ * to be able to know about the current root directory and
+ * parent relationships.
+ */
+ if (nd->last.name[0] == '.') switch (nd->last.len) {
+ default:
+ break;
+ case 2:
+ if (nd->last.name[1] != '.')
+ break;
+ follow_dotdot(nd);
+ inode = nd->dentry->d_inode;
+ /* fallthrough */
+ case 1:
+ return 0;
+ }
+ /*
+ * See if the low-level filesystem might want
+ * to use its own hash..
+ */
+ if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
+ err = nd->dentry->d_op->d_hash(nd->dentry, &nd->last);
+ if (err < 0)
+ goto out_path_release;
+ }
+
+ /* This does the actual lookups.. */
+ dentry = cached_lookup(nd->dentry, &nd->last,
+ (nd->flags & LOOKUP_LAST) ? 0 : LOOKUP_CONTINUE);
+ if (!dentry) {
+ dentry = real_lookup(nd->dentry, &nd->last,
+ (nd->flags & LOOKUP_LAST) ? 0 : LOOKUP_CONTINUE);
+ err = PTR_ERR(dentry);
+ if (IS_ERR(dentry))
+ goto out_path_release;
+ }
+ /* Check mountpoints.. */
+ while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
+ ;
+
+ inode = dentry->d_inode;
+ if ((nd->flags & LOOKUP_LAST) && !(nd->flags & LOOKUP_FOLLOW)) {
+ dput(nd->dentry);
+ nd->dentry = dentry;
+ goto do_not_follow;
+ }
+
+ if (inode && inode->i_op && inode->i_op->follow_link) {
+ err = do_follow_link(dentry, nd);
+ dput(dentry);
+ if (err)
+ return err;
+ inode = nd->dentry->d_inode;
+ } else {
+ dput(nd->dentry);
+ nd->dentry = dentry;
+ }
+
+do_not_follow:
+ err = -ENOENT;
+ if (!inode)
+ goto out_path_release;
+
+ if ((nd->flags & LOOKUP_LAST) && !(nd->flags & LOOKUP_DIRECTORY))
+ return 0;
+
+ err = -ENOTDIR;
+ if (!inode->i_op || !inode->i_op->lookup)
+ goto out_path_release;
+
+ return 0;
+
+out_path_release:
+ path_release(nd);
+
+ return err;
+}
+
/*
* Name resolution.
*
@@ -450,12 +535,11 @@
*
* We expect 'base' to be positive and a directory.
*/
-int link_path_walk(const char * name, struct nameidata *nd)
+int lookup_parent(const char * name, struct nameidata *nd)
{
struct dentry *dentry;
struct inode *inode;
- int err;
- unsigned int lookup_flags = nd->flags;
+ int err = 0;
while (*name=='/')
name++;
@@ -464,7 +548,7 @@
inode = nd->dentry->d_inode;
if (current->link_count)
- lookup_flags = LOOKUP_FOLLOW;
+ nd->flags = LOOKUP_FOLLOW;
/* At this point we know we have a real path component. */
for(;;) {
@@ -488,7 +572,8 @@
} while (c && (c != '/'));
this.len = name - (const char *) this.name;
this.hash = end_name_hash(hash);
-
+
+ nd->last = this;
/* remove trailing slashes? */
if (!c)
goto last_component;
@@ -496,150 +581,53 @@
if (!*name)
goto last_with_slashes;
- /*
- * "." and ".." are special - ".." especially so because it has
- * to be able to know about the current root directory and
- * parent relationships.
- */
- if (this.name[0] == '.') switch (this.len) {
- default:
- break;
- case 2:
- if (this.name[1] != '.')
- break;
- follow_dotdot(nd);
- inode = nd->dentry->d_inode;
- /* fallthrough */
- case 1:
- continue;
- }
- /*
- * See if the low-level filesystem might want
- * to use its own hash..
- */
- if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
- err = nd->dentry->d_op->d_hash(nd->dentry, &this);
- if (err < 0)
- break;
- }
- /* This does the actual lookups.. */
- dentry = cached_lookup(nd->dentry, &this, LOOKUP_CONTINUE);
- if (!dentry) {
- dentry = real_lookup(nd->dentry, &this, LOOKUP_CONTINUE);
- err = PTR_ERR(dentry);
- if (IS_ERR(dentry))
- break;
- }
- /* Check mountpoints.. */
- while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
- ;
-
- err = -ENOENT;
- inode = dentry->d_inode;
- if (!inode)
- goto out_dput;
- err = -ENOTDIR;
- if (!inode->i_op)
- goto out_dput;
+ if ((err = walk_one(nd)) < 0)
+ return err;
- if (inode->i_op->follow_link) {
- err = do_follow_link(dentry, nd);
- dput(dentry);
- if (err)
- goto return_err;
- err = -ENOENT;
- inode = nd->dentry->d_inode;
- if (!inode)
- break;
- err = -ENOTDIR;
- if (!inode->i_op)
- break;
- } else {
- dput(nd->dentry);
- nd->dentry = dentry;
- }
- err = -ENOTDIR;
- if (!inode->i_op->lookup)
- break;
- continue;
+ inode = nd->dentry->d_inode;
+
+ continue;
/* here ends the main loop */
last_with_slashes:
- lookup_flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+ nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
last_component:
- if (lookup_flags & LOOKUP_PARENT)
- goto lookup_parent;
- if (this.name[0] == '.') switch (this.len) {
- default:
- break;
- case 2:
- if (this.name[1] != '.')
- break;
- follow_dotdot(nd);
- inode = nd->dentry->d_inode;
- /* fallthrough */
- case 1:
+ nd->flags |= LOOKUP_LAST;
+
+ if (nd->flags & LOOKUP_PARENT) {
+
+ /* stop at parent of the last component */
+ nd->last_type = LAST_NORM;
+ if (this.name[0] != '.')
goto return_base;
+ if (this.len == 1)
+ nd->last_type = LAST_DOT;
+ else if (this.len == 2 && this.name[1] == '.')
+ nd->last_type = LAST_DOTDOT;
}
- if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
- err = nd->dentry->d_op->d_hash(nd->dentry, &this);
- if (err < 0)
- break;
- }
- dentry = cached_lookup(nd->dentry, &this, 0);
- if (!dentry) {
- dentry = real_lookup(nd->dentry, &this, 0);
- err = PTR_ERR(dentry);
- if (IS_ERR(dentry))
- break;
- }
- while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
- ;
- inode = dentry->d_inode;
- if ((lookup_flags & LOOKUP_FOLLOW)
- && inode && inode->i_op && inode->i_op->follow_link) {
- err = do_follow_link(dentry, nd);
- dput(dentry);
- if (err)
- goto return_err;
- inode = nd->dentry->d_inode;
- } else {
- dput(nd->dentry);
- nd->dentry = dentry;
- }
- err = -ENOENT;
- if (!inode)
- break;
- if (lookup_flags & LOOKUP_DIRECTORY) {
- err = -ENOTDIR;
- if (!inode->i_op || !inode->i_op->lookup)
- break;
- }
- goto return_base;
-lookup_parent:
- nd->last = this;
- nd->last_type = LAST_NORM;
- if (this.name[0] != '.')
- goto return_base;
- if (this.len == 1)
- nd->last_type = LAST_DOT;
- else if (this.len == 2 && this.name[1] == '.')
- nd->last_type = LAST_DOTDOT;
return_base:
return 0;
-out_dput:
- dput(dentry);
- break;
}
path_release(nd);
-return_err:
return err;
}
int path_walk(const char * name, struct nameidata *nd)
{
+ int err;
+
current->total_link_count = 0;
- return link_path_walk(name, nd);
+ err = lookup_parent(name, nd);
+
+ /* nothing else to do if walking '/' */
+ if (name[0] == '/' && name[1] == '\0')
+ return err;
+
+ /* handle last component if needed */
+ if (!err && !(nd->flags & LOOKUP_PARENT))
+ err = walk_one(nd);
+
+ return err;
}
/* SMP-safe */
@@ -1922,7 +1910,17 @@
/* weird __emul_prefix() stuff did it */
goto out;
}
- res = link_path_walk(link, nd);
+
+ res = lookup_parent(link, nd);
+
+ /* nothing else to do if walking '/' */
+ if (link[0] == '/' && link[1] == '\0')
+ goto out;
+
+ /* handle last component if needed */
+ if (!res && !(nd->flags & LOOKUP_PARENT))
+ res = walk_one(nd);
+
out:
if (current->link_count || res || nd->last_type!=LAST_NORM)
return res;
diff -urN linux-2.5.10/include/linux/fs.h linux-2.5.10-lpw/include/linux/fs.h
--- linux-2.5.10/include/linux/fs.h Wed Apr 24 12:45:14 2002
+++ linux-2.5.10-lpw/include/linux/fs.h Fri Apr 26 16:37:20 2002
@@ -1392,6 +1392,7 @@
#define LOOKUP_CONTINUE (4)
#define LOOKUP_PARENT (16)
#define LOOKUP_NOALT (32)
+#define LOOKUP_LAST (64)
/*
* Type of the last component on LOOKUP_PARENT
*/
--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/locking/rcupdate.html
Maneesh Soni wrote:
>
>..
> +static inline int walk_one(struct nameidata *nd)
This function is hundreds and hundreds of bytes of code. It has
three call sites. Making it an inline is very inefficient!
-
On Fri, 26 Apr 2002, Andrew Morton wrote:
> Maneesh Soni wrote:
> >
> >..
> > +static inline int walk_one(struct nameidata *nd)
>
> This function is hundreds and hundreds of bytes of code. It has
> three call sites. Making it an inline is very inefficient!
Unfortunately, this is a very special case. This sucker is involved
in mutual recursion and extra frame on stack will be nasty for, say it,
sparc. Normally I would agree that something of that kind should not be
inlined, but...
On 26 April 2002 15:26, Alexander Viro wrote:
> On Fri, 26 Apr 2002, Andrew Morton wrote:
> > Maneesh Soni wrote:
> > >..
> > > +static inline int walk_one(struct nameidata *nd)
> >
> > This function is hundreds and hundreds of bytes of code. It has
> > three call sites. Making it an inline is very inefficient!
>
> Unfortunately, this is a very special case. This sucker is involved
> in mutual recursion and extra frame on stack will be nasty for, say it,
> sparc. Normally I would agree that something of that kind should not be
> inlined, but...
Maybe add a comment? Or this question will emerge on lkml from time to time.
--
vda
Hi Maneesh,
The handling of '/' in path_walk() and vfs_follow_link() is broken - if
the pathname consists entirely of '/' characters, then lookup_parent
returns the base immediately without setting nd->last. If there's more
than one '/', then the check for looking up '/' won't be triggered, and
walk_one() will be called with an undefined nd->last.
So e.g. running
ls '//'
produces
ls: //: File name too long
(but I suspect that it could Oops, depending on what was on the stack
when the nameidata was allocated.)
Ideally the tests in vfs_follow_link() and path_walk() ought to be
testing for (nd->last_type == LAST_ROOT), rather than checking
explicitly for '/' followed by NUL. But that doesn't work, as last_type
is only set if you include LOOKUP_PARENT in the flags. Setting last_type
on every path element resolution would probably be unecessary overhead
given the relative infrequency of looking up "/".
So the alternative that I suggest is to change lookup_parent() as
follows:
while (*name=='/')
name++;
if (!*name) {
nd->last = (struct qstr) { name : ".", len : 1, hash : 0 };
goto return_base;
}
and remove the tests for "/" in vfs_follow_link() and path_walk().
Setting nd->last to refer to "." will cause walk_one() to return
immediately, so the zero hash value is OK.
Paul
On Fri, Apr 26, 2002 at 02:28:26PM -0700, Paul Menage wrote:
>
> Hi Maneesh,
>
> The handling of '/' in path_walk() and vfs_follow_link() is broken - if
> the pathname consists entirely of '/' characters, then lookup_parent
> returns the base immediately without setting nd->last. If there's more
> than one '/', then the check for looking up '/' won't be triggered, and
> walk_one() will be called with an undefined nd->last.
> [..]
Hi Paul,
Thanks for pointing it. The corrected patch is appended.
Regards,
Maneesh
--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/locking/rcupdate.html
diff -urN linux-2.5.10/fs/namei.c linux-2.5.10-lpw/fs/namei.c
--- linux-2.5.10/fs/namei.c Wed Apr 24 12:45:16 2002
+++ linux-2.5.10-lpw/fs/namei.c Mon Apr 29 08:42:05 2002
@@ -442,6 +442,95 @@
;
}
+
+/* Big routine.. not supposed to be inlined but stack usage has to be limited
+ * no other choice
+ */
+static inline int walk_one(struct nameidata *nd)
+{
+ int err = 0;
+ struct dentry * dentry;
+ struct inode * inode;
+
+ /*
+ * "." and ".." are special - ".." especially so because it has
+ * to be able to know about the current root directory and
+ * parent relationships.
+ */
+ if (nd->last.name[0] == '.') switch (nd->last.len) {
+ default:
+ break;
+ case 2:
+ if (nd->last.name[1] != '.')
+ break;
+ follow_dotdot(nd);
+ inode = nd->dentry->d_inode;
+ /* fallthrough */
+ case 1:
+ return 0;
+ }
+ /*
+ * See if the low-level filesystem might want
+ * to use its own hash..
+ */
+ if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
+ err = nd->dentry->d_op->d_hash(nd->dentry, &nd->last);
+ if (err < 0)
+ goto out_path_release;
+ }
+
+ /* This does the actual lookups.. */
+ dentry = cached_lookup(nd->dentry, &nd->last,
+ (nd->flags & LOOKUP_LAST) ? 0 : LOOKUP_CONTINUE);
+ if (!dentry) {
+ dentry = real_lookup(nd->dentry, &nd->last,
+ (nd->flags & LOOKUP_LAST) ? 0 : LOOKUP_CONTINUE);
+ err = PTR_ERR(dentry);
+ if (IS_ERR(dentry))
+ goto out_path_release;
+ }
+ /* Check mountpoints.. */
+ while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
+ ;
+
+ inode = dentry->d_inode;
+ if ((nd->flags & LOOKUP_LAST) && !(nd->flags & LOOKUP_FOLLOW)) {
+ dput(nd->dentry);
+ nd->dentry = dentry;
+ goto do_not_follow;
+ }
+
+ if (inode && inode->i_op && inode->i_op->follow_link) {
+ err = do_follow_link(dentry, nd);
+ dput(dentry);
+ if (err)
+ return err;
+ inode = nd->dentry->d_inode;
+ } else {
+ dput(nd->dentry);
+ nd->dentry = dentry;
+ }
+
+do_not_follow:
+ err = -ENOENT;
+ if (!inode)
+ goto out_path_release;
+
+ if ((nd->flags & LOOKUP_LAST) && !(nd->flags & LOOKUP_DIRECTORY))
+ return 0;
+
+ err = -ENOTDIR;
+ if (!inode->i_op || !inode->i_op->lookup)
+ goto out_path_release;
+
+ return 0;
+
+out_path_release:
+ path_release(nd);
+
+ return err;
+}
+
/*
* Name resolution.
*
@@ -450,21 +539,22 @@
*
* We expect 'base' to be positive and a directory.
*/
-int link_path_walk(const char * name, struct nameidata *nd)
+int lookup_parent(const char * name, struct nameidata *nd)
{
struct dentry *dentry;
struct inode *inode;
- int err;
- unsigned int lookup_flags = nd->flags;
+ int err = 0;
while (*name=='/')
name++;
- if (!*name)
+ if (!*name) {
+ nd->last = (struct qstr) { name : ".", len : 1, hash : 0 };
goto return_base;
+ }
inode = nd->dentry->d_inode;
if (current->link_count)
- lookup_flags = LOOKUP_FOLLOW;
+ nd->flags = LOOKUP_FOLLOW;
/* At this point we know we have a real path component. */
for(;;) {
@@ -488,7 +578,8 @@
} while (c && (c != '/'));
this.len = name - (const char *) this.name;
this.hash = end_name_hash(hash);
-
+
+ nd->last = this;
/* remove trailing slashes? */
if (!c)
goto last_component;
@@ -496,150 +587,49 @@
if (!*name)
goto last_with_slashes;
- /*
- * "." and ".." are special - ".." especially so because it has
- * to be able to know about the current root directory and
- * parent relationships.
- */
- if (this.name[0] == '.') switch (this.len) {
- default:
- break;
- case 2:
- if (this.name[1] != '.')
- break;
- follow_dotdot(nd);
- inode = nd->dentry->d_inode;
- /* fallthrough */
- case 1:
- continue;
- }
- /*
- * See if the low-level filesystem might want
- * to use its own hash..
- */
- if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
- err = nd->dentry->d_op->d_hash(nd->dentry, &this);
- if (err < 0)
- break;
- }
- /* This does the actual lookups.. */
- dentry = cached_lookup(nd->dentry, &this, LOOKUP_CONTINUE);
- if (!dentry) {
- dentry = real_lookup(nd->dentry, &this, LOOKUP_CONTINUE);
- err = PTR_ERR(dentry);
- if (IS_ERR(dentry))
- break;
- }
- /* Check mountpoints.. */
- while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
- ;
-
- err = -ENOENT;
- inode = dentry->d_inode;
- if (!inode)
- goto out_dput;
- err = -ENOTDIR;
- if (!inode->i_op)
- goto out_dput;
+ if ((err = walk_one(nd)) < 0)
+ return err;
- if (inode->i_op->follow_link) {
- err = do_follow_link(dentry, nd);
- dput(dentry);
- if (err)
- goto return_err;
- err = -ENOENT;
- inode = nd->dentry->d_inode;
- if (!inode)
- break;
- err = -ENOTDIR;
- if (!inode->i_op)
- break;
- } else {
- dput(nd->dentry);
- nd->dentry = dentry;
- }
- err = -ENOTDIR;
- if (!inode->i_op->lookup)
- break;
- continue;
+ inode = nd->dentry->d_inode;
+
+ continue;
/* here ends the main loop */
last_with_slashes:
- lookup_flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+ nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
last_component:
- if (lookup_flags & LOOKUP_PARENT)
- goto lookup_parent;
- if (this.name[0] == '.') switch (this.len) {
- default:
- break;
- case 2:
- if (this.name[1] != '.')
- break;
- follow_dotdot(nd);
- inode = nd->dentry->d_inode;
- /* fallthrough */
- case 1:
+ nd->flags |= LOOKUP_LAST;
+
+ if (nd->flags & LOOKUP_PARENT) {
+
+ /* stop at parent of the last component */
+ nd->last_type = LAST_NORM;
+ if (this.name[0] != '.')
goto return_base;
+ if (this.len == 1)
+ nd->last_type = LAST_DOT;
+ else if (this.len == 2 && this.name[1] == '.')
+ nd->last_type = LAST_DOTDOT;
}
- if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
- err = nd->dentry->d_op->d_hash(nd->dentry, &this);
- if (err < 0)
- break;
- }
- dentry = cached_lookup(nd->dentry, &this, 0);
- if (!dentry) {
- dentry = real_lookup(nd->dentry, &this, 0);
- err = PTR_ERR(dentry);
- if (IS_ERR(dentry))
- break;
- }
- while (d_mountpoint(dentry) && __follow_down(&nd->mnt, &dentry))
- ;
- inode = dentry->d_inode;
- if ((lookup_flags & LOOKUP_FOLLOW)
- && inode && inode->i_op && inode->i_op->follow_link) {
- err = do_follow_link(dentry, nd);
- dput(dentry);
- if (err)
- goto return_err;
- inode = nd->dentry->d_inode;
- } else {
- dput(nd->dentry);
- nd->dentry = dentry;
- }
- err = -ENOENT;
- if (!inode)
- break;
- if (lookup_flags & LOOKUP_DIRECTORY) {
- err = -ENOTDIR;
- if (!inode->i_op || !inode->i_op->lookup)
- break;
- }
- goto return_base;
-lookup_parent:
- nd->last = this;
- nd->last_type = LAST_NORM;
- if (this.name[0] != '.')
- goto return_base;
- if (this.len == 1)
- nd->last_type = LAST_DOT;
- else if (this.len == 2 && this.name[1] == '.')
- nd->last_type = LAST_DOTDOT;
return_base:
return 0;
-out_dput:
- dput(dentry);
- break;
}
path_release(nd);
-return_err:
return err;
}
int path_walk(const char * name, struct nameidata *nd)
{
+ int err;
+
current->total_link_count = 0;
- return link_path_walk(name, nd);
+ err = lookup_parent(name, nd);
+
+ /* handle last component if needed */
+ if (!err && !(nd->flags & LOOKUP_PARENT))
+ err = walk_one(nd);
+
+ return err;
}
/* SMP-safe */
@@ -1922,7 +1912,13 @@
/* weird __emul_prefix() stuff did it */
goto out;
}
- res = link_path_walk(link, nd);
+
+ res = lookup_parent(link, nd);
+
+ /* handle last component if needed */
+ if (!res && !(nd->flags & LOOKUP_PARENT))
+ res = walk_one(nd);
+
out:
if (current->link_count || res || nd->last_type!=LAST_NORM)
return res;
diff -urN linux-2.5.10/include/linux/fs.h linux-2.5.10-lpw/include/linux/fs.h
--- linux-2.5.10/include/linux/fs.h Fri Apr 26 17:12:19 2002
+++ linux-2.5.10-lpw/include/linux/fs.h Fri Apr 26 16:48:48 2002
@@ -1392,6 +1392,7 @@
#define LOOKUP_CONTINUE (4)
#define LOOKUP_PARENT (16)
#define LOOKUP_NOALT (32)
+#define LOOKUP_LAST (64)
/*
* Type of the last component on LOOKUP_PARENT
*/