On Thu, Nov 09, 2023 at 09:39:09AM -0800, Linus Torvalds wrote:
> On Wed, 8 Nov 2023 at 22:23, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > static struct dentry *__lock_parent(struct dentry *dentry)
> > {
> > struct dentry *parent;
> > - rcu_read_lock();
> > - spin_unlock(&dentry->d_lock);
> > again:
> > parent = READ_ONCE(dentry->d_parent);
> > spin_lock(&parent->d_lock);
>
> Can we rename this while at it?
>
> That name *used* to make sense, in that the function was entered with
> the dentry lock held, and then it returned with the dentry lock *and*
> the parent lock held.
>
> But now you've changed the rules so that the dentry lock is *not* held
> at entry, so now the semantics of that function is essentially "lock
> dentry and parent". Which I think means that the name should change to
> reflect that.
>
> Finally: it does look like most callers actually did hold the dentry
> lock, and that you just moved the
>
> spin_unlock(&dentry->d_lock);
>
> from inside that function to the caller. I don't hate that, but now
> that I look at it, I get the feeling that what we *should* have done
> is
>
> static struct dentry *__lock_parent(struct dentry *dentry)
> {
> struct dentry *parent = dentry->d_parent;
> if (try_spin_lock(&parent->d_lock))
> return parent;
> /* Uhhuh - need to get the parent lock first */
> .. old code goes here ..
>
> but that won't work with the new world order.
Can't - currently lock_for_kill() uses it in a loop. Can't have trylocks
in there, or realtime setups will get unhappy. More to the point, the whole
function is gone by the end of the series. Along with lock_parent().
The only reason why we needed that thing is that we lock the parent too
early; that's where the last commit in the series is a big win. There
we remove from the parent's list of children in the very end, when we'd
already made the victim negative (and unlocked it); there ->d_parent
is stable and we can simply lock that, then lock dentry.
We still need a loop in lock_for_kill() to get the inode locked along
with dentry, but that's less convoluted (the ordering between two
->d_lock can change; ->i_lock is always safe to take before ->d_lock).
> So I get the feeling that maybe instead of renaming it for the new
> semantics, maybe the old semantics of "called with the dentry lock
> held" were simply better"
lock_parent() goes aways when d_prune_alias() is switched to shrink list;
after that __lock_parent() is used only in that loop in lock_for_kill()
and only until (22/22) when lock_for_kill() stops touching the parent.
After that it's simply gone.
