Re: [PATCH 17/22] don't try to cut corners in shrink_lock_dentry()

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, Nov 09, 2023 at 09:39:09AM -0800, Linus Torvalds wrote:
> On Wed, 8 Nov 2023 at 22:23, Al Viro <viro@zeniv.linux.org.uk> 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.