link_path_walk cleanup
RFC 1/1 link_path_walk cleanup
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
fs/namei.c +131 -133
@@ -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;
include/linux/fs.h +1 -0
@@ -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 */
Signature · 5 lines
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
Quoted 3 lines · Maneesh Soni
..
+static inline intwalk_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!
-
Quoted 7 lines · Andrew Morton
Quoted 3 lines · Maneesh Soni
..
+static inline intwalk_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...
Quoted 5 lines · Alexander Viro
Quoted 4 lines · Andrew Morton
Quoted 2 lines · Maneesh Soni
..
+static inline intwalk_one(struct nameidata *nd)
Quoted 7 lines
Quoted 2 lines
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