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

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

On Mon, Jul 25, 2011 at 07:04:11PM -0700, Linus Torvalds wrote:
> 
> 
> One of the biggest remaining unnecessary costs in path walking is the
> pointer chasing in inode operations.  We already avoided the
> dentry->d_op derferences with the DCACHE_OP_xyz flags, this just starts
> doing the same thing for the i_op->xyz cases and new IOP_xyz flags in 
> the new inode->i_opflag field.
> 
> To make sure the i_opflag field is consistent with i_op, we introduce a 
> simple wrapper function to do assignments to i_op. That makes the patch 
> big, but it was almost entirely mechanical and very straightforward.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> 
> Ok, so this patch is pretty big, but the bulk if it is an entirely 
> mechanical replacement of
> 
> 	inode->i_op = xyz;
> 
> with
> 
> 	set_inode_ops(inode, xyz);
> 
> using a very stupid sed-script. It was followed by some manual editing 
> (a couple of fixups for my sed-script being stupid, and a couple of 
> whitespace cleanups for pre-existing whitespace problems that 'git diff' 
> just highlights).
> 
> This removes the 'inode->i_op->xyz' chain from the critical pathname walk, 
> although the *single* hottest instruction in link_path_walk on my Westmere 
> system remains the test of 'inode->i_opflag' for IOP_FOLLOW_LINK:
> 
>    14.70%: testb  $0x2,(%rax)
> 
> because it turns out that that instruction is what brings in the inode 
> into the L1 cache. But hey, it's certainly better than getting that i_op 
> and then following the pointer there and checking it for NULL.
> 
> And it's better to the tune of roughly 0.1s for my empty "make -j" test 
> (which is actually dominated by the user-space costs in 'make', but has a 
> largish pathname component to it too). That's out of about 4.3s, so it is 
> about 2%. Not huge, but it seems quite measurable.
> 
> Of course, those kinds of costs will depend very much on cache sizes and 
> microarchitectural details, and the "empty kernel make -j" may not be the 
> most relevant benchmark around, but it's more or a real load than some.
> 
> Hmm? Too painful?

We could actually cheat a bit.  *All* inodes that ever reach inode_permission()
have at some point been pointed to by ->d_inode of some dentry.  It's
inconvenient as hell (and inviting abuse) to pass such dentry instead
of inode; moreover, there would be nasty questions of ->d_inode stability
on RCU path.

However, we can greatly reduce that amount of changes if we do the
following:
	* one bit in your new field set when inode is put in ->d_inode.
Checked at inode_permission() time, BUG_ON() if not set.
	* other bit(s) set by checking ->i_op contents at the same time,
if bit hadn't been already set (->i_op can't change afterwards); checked
by inode_permission() and whatever else might want to (e.g. checks for
non-NULL ->lookup(); for those we also know that inode went through
some ->d_inode at some point).
	* we need to play with setting these flags only in two places -
__d_instantiate() and d_obtain_alias().

IOW, add

/* called with inode->i_lock held */
static inline void set_inode_flags(struct inode *inode)
{
	if (!(inode->i_opflag & Iop_valid)) {
		struct inode_operations *op = inode->i_op;
		int flags = Iop_valid;
		if (op->permission)
			flags |= Iop_permission;
		if (op->lookup)
			flags |= Iop_lookup;
		inode->i_opflag = flags;
	}
}

and turn __d_instantiate() into
static void __d_instantiate(struct dentry *dentry, struct inode *inode)
{
        spin_lock(&dentry->d_lock);
        if (inode) {
                if (unlikely(IS_AUTOMOUNT(inode)))
                        dentry->d_flags |= DCACHE_NEED_AUTOMOUNT;
		set_inode_flags(inode);
                list_add(&dentry->d_alias, &inode->i_dentry);
        }
        dentry->d_inode = inode;
        dentry_rcuwalk_barrier(dentry);
        spin_unlock(&dentry->d_lock);
        fsnotify_d_instantiate(dentry, inode);
}

with set_inode_flags(inode); also added right after
        tmp->d_inode = inode;
in d_obtain_alias()

Voila - inode_permission() would do
	int flags;
	...
	flags = inode->i_opflag;
	BUG_ON(!(flags & Iop_valid));
	if (unlikely(flags & Iop_permission))
		res = inode->i_op->permission(...);
	else
		res = generic_permission(...)
and we are all set.  Individual fs code is not affected at all...  Sure, it's
a bit of cheating, but it avoids a lot of churn *and* a nasty class of bugs
in the future - somebody assigning ->i_op directly.  Dunno...