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

[Thomas Gleixner]

On Tue, 19 Oct 2010, Christoph Hellwig wrote:

> 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().

Errm, when hlist_lock() has proper documentation than it should not be
rocket science to figure out what it does.

And if you use bit 0 of hlist then you better have helper functions to
access it anyway. We do that with other data types which (ab)use the
lower two bits of pointers.

> 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

And if you design that structure clever, then simple dereferencing of
it (w/o casting magic) should make the compiler barf. So you are
forced to use the helper functions.

> 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.

Yes, please use proper helper functions. Having to change code is a
horror for RT, when we can get away with a single change in a header
file.

Aside of RT there is another advantage of being able to change the
lock implementation at a single place: you can change it to a real
spinlock and have lockdep coverage of that code. I fundamentally hate
bit_spin_locks for sneaking around lockdep.

Thanks,

	tglx
--
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/