Re: [BK-PATCH 1/3] Introduce fs/inode.c::ilookup().

[Daniel Phillips <phillips@arcor.de>]

On Monday 09 September 2002 10:59, Anton Altaparmakov wrote:
> At 18:04 08/09/02, Daniel Phillips wrote:
> >On Saturday 07 September 2002 16:27, Anton Altaparmakov wrote:
> > > The second and third patch have the small disadvantage to the previous
> > > code in that in the case that ilookup() fails in iget_locked() and
> > > get_new_inode_fast() is called the inode hash is calculated twice.
> > > But that is the slow path so I don't think it is a problem.
> >
> >It doesn't make sense to introduce even this small inefficiency when
> >all you need to do is wrap an __ilookup inline that takes the hash
> >list, and is called both from ilookup and iget.  The inline costs
> >nothing, the hidden inefficiency costs cycles, however few.
> 
> True. It is not in fast path though. The fast path is ilookup() succeeding. 
> If the inode is not in cache, then the fs will have to read it from disk at 
> which point the additional hash calculation goes down in the noise on the 
> cpu cycles front.

The inode block may be (probably is) in cache, so the hash doesn't get as
lost as you'd think.  Granted, it's an efficient hash, but actually, we'd
like to make it a little less efficient to get better distribution, and the 
efficiency argument re that improvement has dribbled on for *years*, even
though the improved hash would not be as much as twice as expensive.

That slow path is still very important.  Many loads always hit the slow
path (find).

> But if you think it is so important, then yeah, I can 
> make an inline wrapper. No problem.

Thanks, then I'll be able to sleep tonight ;-)

While you're in there, feast your eyes on hash() and meditate on how much
it sucks.  Bill Irwin did some lovely work a while back on sparse
multiplicative hashes, which ought to be dressed up and incorporated.

-- 
Daniel