From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH] VFS: Cut down inode->i_op->xyz accesses in path walking Date: Sat, 23 Jul 2011 15:46:43 +0100 Message-ID: <20110723144643.GE24703@ZenIV.linux.org.uk> References: <20110723133534.GA8644@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Linus Torvalds , linux-fsdevel To: Christoph Hellwig Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:52485 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752875Ab1GWOqr (ORCPT ); Sat, 23 Jul 2011 10:46:47 -0400 Content-Disposition: inline In-Reply-To: <20110723133534.GA8644@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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...