Re: [RFC][PATCH] make sure that lock_for_kill() callers drop the locks in safe order

Linus Torvalds
2026-04-10T19:30:34Z

On Fri, 10 Apr 2026 at 11:48, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, I wonder if we would be better off with the following:
>
> void shrink_dentry_list(struct list_head *list)
> {
> while (!list_empty(list)) {
> struct dentry *dentry;
>
> dentry = list_entry(list->prev, struct dentry, d_lru);
> spin_lock(&dentry->d_lock);
> d_shrink_del(dentry);
> if (unlikely(dentry->d_flags & DCACHE_DENTRY_KILLED)) {
> // killed while on shrink list, freeing left to us
> spin_unlock(&dentry->d_lock);
> dentry_free(dentry);
> continue;
> }
> rcu_read_lock();
> if (!lock_for_kill(dentry)) {
> spin_unlock(&dentry->d_lock);
> rcu_read_unlock();
> continue;
> }

Ugh. I despise that rcu_read_lock() in there, because it's typically
completely useless.

The reason it exists is because lock_for_kill() can drop d_lock(), but
that's in the unlikely case that we cn't just immediately get the
inode lock.

So honestly, I think that rcu_read_lock() should be inside
lock_for_kill(), rather than in the caller as a "just in case things
go down".

IOW, that code basically now not only does that typically unnecessary
rcu locking (and then unlocking), it does so *because* it knows about
the subtle internal behavior of lock_for_kill().

In contrast, if we put it inside lock_for_kill(), all we need is a
fairly straightforward comment along the lines of

"we're dropping the spinlock in order to take the inode lock, but
the caller may be depending on RCU behavior so we need to take the rcu
read lock first".

that would turn strange illogical code that also generates worse code
into straightforwardly explained code that also performs better.

Ok, so "performs better" is kind of exaggerated, in that obviously the
extra rcu_read_lock/unlock sequences aren't exactly expensive, but
still - I feel it's a win-win to just do this differently.

Or am I missing somethign else?

Linus