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

[Christoph Hellwig]

On Tue, Oct 19, 2010 at 06:00:57PM +1100, Nick Piggin wrote:
> But it is still "magic". Because you don't even know whether it
> is a spin or sleeping lock, let alone whether it is irq or bh safe.
> You get far more information seeing a bit_spin_lock(0, &hlist) call
> than hlist_lock().
> 
> Even if you do rename them to hlist_bit_spin_lock, etc. Then you need
> to add variants for each type of locking a caller wants to do on it.
> Ask Linus what he thinks about that.

And why do we need all these versions?  We never had irqsave or bhsave
versions of bitlocks.  The only thing we might eventually want are
trylock and is_locked operations once we grow user of it.

To get back a bit to the point:

 - we have a new bl_hlist sturcture which combines a hash list and a
   lock embedded into the head
 - the reason why we do it is to be able to use a bitlock
 
Now your initial version exposed the ugly defaults of that to the user
which is required to case the hash head to and unsigned long and use
bit_spin_lock on bit 0 of it.  There's zero abstraction and a lot
internal gutting required there.

The version I suggest and that dave implemented instead adds wrappers
to call bit_spin_lock/unlock with thos conventions as helper that
operate on the abstract type.  This frees the caller from revinventing
just those same wrappers, as done in your demo tree for the inode and
dentry hashes.

Furthermore it allows the RT people to simply throw a mutex into the
head and everything keeps working without touching a sinlge line of
code outside of hlist_bl.h.

To me that's very clearly preferable, and I'd really like to see
a clearly reasoned argument against it.
--
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/