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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 25 Oct 2010, Nick Piggin wrote:

> On Sun, Oct 24, 2010 at 05:44:24PM +0200, Thomas Gleixner wrote:
> > 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.
> 
> Right, a look at the docmentation and another layer of indirection
> for a reader.
> 
> And it's not exactly "properly" documented. It doesn't say if it may
> turn into a sleeping lock or is allowed to be used from irq or bh
> context.

Oh well, that's nickpicking :)
 
> > 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.
> 
> You do not want to add a bloated mutex to each inode hash bucket and
> think you can just dust off your hands and walk away.  You would
> probably make a smaller auxiliary hash of locks, sanely sized, and
> protect it with that.
> 
> So it would be wrong to just bloat hlist_bl by a factor of several times
> (how big is a mutex in -rt?) without doing anything else.

Let me worry about it.

> Although a sane locking macro and structure like I had, would perfectly
> allow you to switch locks in a single place just the same.

And a locking macro/structure is better in self documenting than a
helper function which was proposed by Christoph?

Thanks,

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


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux