linux-kernel - Re: [RFC PATCH] vfs: add rcu-based find

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: <20240611112846.qesh7qhhuk3qp4dy@quack3>
Date: Tue, 11 Jun 2024 13:28:46 +0200
From: Jan Kara <jack@...e.cz>
To: Dave Chinner <david@...morbit.com>
Cc: Mateusz Guzik <mjguzik@...il.com>, brauner@...nel.org,
	viro@...iv.linux.org.uk, jack@...e.cz, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC PATCH] vfs: add rcu-based find_inode variants for iget ops

On Tue 11-06-24 20:17:16, Dave Chinner wrote:
> Your patch, however, just converts *some* of the lookup API
> operations to use RCU. It adds complexity for things like inserts
> which are going to need inode hash locking if the RCU lookup fails,
> anyway.
> 
> Hence your patch optimises the case where the inode is in cache but
> the dentry isn't, but we'll still get massive contention on lookup
> when the RCU lookup on the inode cache and inserts are always going
> to be required.
> 
> IOWs, even RCU lookups are not going to prevent inode hash lock
> contention for parallel cold cache lookups. Hence, with RCU,
> applications are going to see unpredictable contention behaviour
> dependent on the memory footprint of the caches at the time of the
> lookup. Users will have no way of predicting when the behaviour will
> change, let alone have any way of mitigating it. Unpredictable
> variable behaviour is the thing we want to avoid the most with core
> OS caches.

I don't believe this is what Mateusz's patches do (but maybe I've terribly
misread them). iget_locked() does:

	spin_lock(&inode_hash_lock);
	inode = find_inode_fast(...);
	spin_unlock(&inode_hash_lock);
	if (inode)
		we are happy and return
	inode = alloc_inode(sb);
	spin_lock(&inode_hash_lock);
	old = find_inode_fast(...)
	the rest of insert code
	spin_unlock(&inode_hash_lock);

And Mateusz got rid of the first lock-unlock pair by teaching
find_inode_fast() to *also* operate under RCU. The second lookup &
insertion stays under inode_hash_lock as it is now. So his optimization is
orthogonal to your hash bit lock improvements AFAICT. Sure his optimization
just ~halves the lock hold time for uncached cases (for cached it
completely eliminates the lock acquisition but I agree these are not that
interesting) so it is not a fundamental scalability improvement but still
it is a nice win for a contended lock AFAICT.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR