Re: [RFC PATCH 20/25] d_walk(): shrink rcu_read_lock() scope

[Frederic Weisbecker <frederic@kernel.org>]

(Adding other RCU folks in Cc)


Le Tue, May 05, 2026 at 09:05:01PM +0100, Al Viro a écrit :
> On Tue, May 05, 2026 at 10:01:39AM -0700, Linus Torvalds wrote:
> 
> > If we had a function that was "transfer spinlock without dropping RCU
> > read lock", and we'd use that as an _optimization_ (maybe we could
> > avoid all the preemption count games and testing for whether there are
> > active RCU events etc), that would make sense to me.
> >
> > At that point it would *all* become "we rely on the spinlock to hold
> > RCU read locking", and it wouldn't mix locking models. So I think such
> > an operation might make sense on its own.
> 
> Definitely; we have the same "bridge two spin_lock scopes together into
> a contiguous RCU read-side critical area" elsewhere as well.  Hell,
> 
> 	rcu_read_lock();
> 	do {
> 		spin_unlock(&dentry->d_lock);
> 		spin_lock(&inode->i_lock);
> 		spin_lock(&dentry->d_lock);
> 		if (likely(inode == dentry->d_inode))
> 			break;
> 		spin_unlock(&inode->i_lock);
> 		inode = dentry->d_inode;
> 	} while (inode);
> 	rcu_read_unlock();
> 
> in lock_for_kill() might be better off as
> 
> 	do {
> 		transfer_spinlock_with_rcu(&dentry->d_lock, &inode->i_lock);
> 		spin_lock(&dentry->d_lock);
> 		if (likely(inode == dentry->d_inode))
> 			break;
> 		spin_unlock(&inode->i_lock);
> 		inode = dentry->d_inode;
> 	} while (inode);
> 
> Explicit rcu_read_lock() uses don't explain what's really going on; in this
> form it's "we can't take ->i_lock under ->d_lock, but we need an unbroken
> RCU read-side critical area for memory safety".  The same goes for d_walk()
> use ("can't take parent's ->d_lock under child's one"), as well as in
> shrink_dcache_for_umount() ("can't take ->d_lock under ->s_roots_lock")
> and I wouldn't be surprised to see other instances of the same pattern.
> 
> I would really prefer to have all explicit rcu_read_lock() in fs/dcache.c
> clearly documented ;-/

And the calls to rcu_read_lock() / rcu_read_unlock() are _usually_ cheap enough
that their explicit calls should go unnoticed over an implicit spinlock RCU
critical section. Ok rcu_read_unlock() has its slowpath but it would otherwise
trigger randomly when implicit read side are used (context switch, interrupts,
etc...)

Also rcu_read_lock() has the advantage to tell clearly that we are doing an RCU read
side (and its uses indeed claim to be documented) here while relying on spinlocks to do
that is fine but implicit and more error-prone when there are tricky things
like spinlock transfers: better don't ask for trouble.

So I would say that we should probably avoid introducing such an API if possible.

> FWIW, back in 2015 spin_lock() did *not* provide an RCU read-side critical
> area, according to https://lwn.net/Articles/652156/; would be nice to have
> the history mentioned in D/RCU/whatisRCU.rst - at least to the level of
> "once upon a time one *did* need an explicit rcu_read_lock() in addition
> to spin_lock() - spin_lock() or preempt_disable() alone was not enough;
> that has changed in <...>".
> 
> I'm not familiar enough with that code for that kind of archaeology ;-/
> Could somebody help with that?  Paul?

That's quite outdated. Now preempt disabled, IRQs disabled, and spinlocks
are all implicit RCU read sides. On non RT spinlocks disable preemption and
on RT they call rcu_read_lock() internally.

Thanks.

-- 
Frederic Weisbecker
SUSE Labs