Received-SPF: pass (google.com: domain of linux-kernel+bounces-209737-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 11 Jun 2024 13:28:46 +0200
From: Jan Kara <jack@suse.cz>
To: Dave Chinner <david@fromorbit.com>
Cc: Mateusz Guzik <mjguzik@gmail.com>, brauner@kernel.org,
	viro@zeniv.linux.org.uk, jack@suse.cz, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH] vfs: add rcu-based find_inode variants for iget ops
Message-ID: <20240611112846.qesh7qhhuk3qp4dy@quack3>
References: <20240606140515.216424-1-mjguzik@gmail.com>
 <ZmJqyrgPXXjY2Iem@dread.disaster.area>
 <bujynmx7n32tzl2xro7vz6zddt5p7lf5ultnaljaz2p2ler64c@acr7jih3wad7>
 <ZmgkLHa6LoV8yzab@dread.disaster.area>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZmgkLHa6LoV8yzab@dread.disaster.area>

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@suse.com>
SUSE Labs, CR