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@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
