Re: [PATCH 11/18] fs: Introduce per-bucket inode hash locks

[Dave Chinner]

On Sun, Oct 17, 2010 at 04:12:13AM +1100, Nick Piggin wrote:
> On Sat, Oct 16, 2010 at 12:16:42PM -0400, Christoph Hellwig wrote:
> > On Sat, Oct 16, 2010 at 06:57:03PM +1100, Nick Piggin wrote:
> > > > duplicating these helpers in the dcache code aswell.  IMHO they
> > > > should simple operate directly on the hlist_bl_head, as that's
> > > > what it was designed for.  I also don't really see any point in
> > > > wrapping the hlist_bl_head as inode_hash_bucket.  If the bucket naming
> > > > is important we could rename the hlist_bl stuff to bl_hash, and the
> > > > hlist_bl_head could become bl_hash_bucket.
> > > 
> > > It was done because someone, like -rt, might want more than one bit of
> > > memory to implement a lock. They would have to make a few other
> > > changes, granted, but this helps reduce a lot of churn.
> > > 
> > > I didn't see the point of a layer of dumb wrappers for hlist_bl_head
> > > locking. Just reproducing bit spin and wait locks in wrappers when we
> > > already have good functions for them.
> > 
> > With the changes Dave implemented based on my suggestions we now have
> > an abstract locked hash list data type.  It has the normal hash list
> > operations plus lock/unlock operations.
> 
> That's ugly. It just hides the locking. If a bit of casting bothers
> you then put it in a function where it is used like I did.

I much prefer the abstraction from an end-user point of view. Asking
every developer to understand the intricacies of locking the
hlist_bl structures is asking them to get it wrong. Providing
locking wrappers that are exactly what users need so they don't have
to care about it is, IMO, the right thing to do.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/