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.
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"
Linus
