Messages in this thread | | | Date | Tue, 21 May 2013 15:22:44 -0700 | Subject | Stupid VFS name lookup interface.. | From | Linus Torvalds <> |
| |
Ok, Al, please tell me why I'm wrong, but I was looking at the hot code in fs/dcache.c again (__d_lookup_rcu() remains the hottest function under pathname lookup heavy operations) and that "inode" argument was mis-commented (it used to be an in-out argument long long ago) and it just kept bugging me.
And it looks totally pointless anyway. Nobody sane actually wants it.
Yeah, several filesystems use "pinode->i_sb" to look up their superblock, but they can use "dentry->d_sb" for that instead. You did most of that long ago, I think.
The *one* insane exception is ncpfs, which actually wants to look at the parent (ie directory) inode data in order to decide if it should use a case sensitive hash or not. However, even in that case, I'd argue that we could just optimistically do a ACCESS_ONCE(dentry->d_inode) and do the compare using the information we got from that.
Because we don't care if the dentry->d_inode is unstable: if we got some stale inode, we would hit the dentry_rcuwalk_barrier() case for that parent when we later check the sequence numbers. So then we'd throw away the comparison result anyway. We check both the dentry and the parent sequence count in lookup_fast(), verifying that they've been stable over the sequence.
So as far as I can tell, the only thing we should worry about might be a NULL pointer due to a concurrent rmdir(), but the identity of the inode itself we really don't care too much about. Take one or the other, and don't crash on NULL.
There's a similat case going on in proc_sys_compare(). Same logic applies.
Hmm?
Getting rid of those annoying separate dentry inode pointers makes __d_lookup_rcu() compile into noticeably better code on x86-64, because there's no stack frame needed any more. And all the filesystems end up cleaner too, and the crazy cases go where they belong.
Untested patch attached. It compiles cleanly, looks sane, and most of it is just making the function prototypes look much nicer. I think it works.
Linus [unhandled content-type:application/octet-stream] | |