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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101019165044.GA13531@infradead.org>
Date:	Tue, 19 Oct 2010 12:50:44 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Nick Piggin <npiggin@...nel.dk>
Cc:	Christoph Hellwig <hch@...radead.org>,
	Andi Kleen <andi@...stfloor.org>,
	Dave Chinner <david@...morbit.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	tglx@...utronix.de
Subject: Re: [PATCH 11/18] fs: Introduce per-bucket inode hash locks

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/