Re: [PATCH] VFS: Cut down inode->i_op->xyz accesses in path walking

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sat, Jul 23, 2011 at 03:35:34PM +0200, Christoph Hellwig wrote:
> On Fri, Jul 22, 2011 at 08:55:48PM -0700, Linus Torvalds wrote:
> > If something sets dentry->d_inode without going through __d_instantiate, 
> > that misses it. I'm looking at d_obtain_alias(), and wondering, for 
> > example.
> 
> As pointed out in my last mail d_obtain_alias will absolute need setting
> up these flags as well.

... and dentry_unlink_inode() - clearing them (note that we already are
clearing DCACHE_CANT_MOUNT there, so additional price will be trivial).

> I don't think we need to re-add exec_permission for it.  By checking
> DCACHE_OP_PERMISSION instead of inode->i_op->permission you'll always
> got directly to generic_permission in inode_permission, and in there
> acl_permission_check -> check_acl should simply do the right thing with
> your your patch to take the ACL cache checking into common code.

Checking DCACHE_OP_PERMISSION on _what_?  inode_permission() gets inode,
not dentry...  Now, we could mirror those into struct inode itself (and
that might make more sense, actually), but I'd rather not go through
the equivalent of d_set_d_op() patchset for i_op, with no hope for per-sb
defaults reducing the size of mess this time.  Calculating those flags
at __d_instantiate/d_obtain_alias still looks like the least painful
way to do it...

Come to think of that, reintroducing exec_permission() and making it
take dentry has a problem - we would be asking for trouble with the
"clean the flags" side of things.  If we race with e.g. rmdir(),
dentry *can* become negative under us on those calls.  Inode will
remain allocated and, with MAY_NOT_BLOCK, the actual ->permission()
will hopefully not rely on anything that might have been freed in
->evict_inode() (we do need to document and verify that, BTW), but
dentry flags could be cleared...