Re: dcache locking question

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

On Sat, Mar 16, 2019 at 03:31:28PM -0700, Paul E. McKenney wrote:
> > Paul, could you comment on that one?  The function in question is
> > this:
> > 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);
> >         /*
> >          * We can't blindly lock dentry until we are sure
> >          * that we won't violate the locking order.
> >          * Any changes of dentry->d_parent must have
> >          * been done with parent->d_lock held, so
> >          * spin_lock() above is enough of a barrier
> >          * for checking if it's still our child.
> >          */
> >         if (unlikely(parent != dentry->d_parent)) {
> 
> We are talking about the above access to dentry->d_parent, correct?
> 
> Given that dentry->d_parent isn't updated except when holding
> dentry->d_lock, and given that locks do what they are supposed to,
> both in terms of memory ordering and mutual exclusion, the value of
> dentry->d_parent must be constant throughout the current critical section.
> It therefore doesn't matter what strange code the compiler generates,
> it will get the same value regardless.
> 
> So, no, there is absolutely no point in doing this:
> 
> 	if (unlikely(parent != READ_ONCE(dentry->d_parent))) {
> 
> Or did I miss the point of this discussion?

dentry->d_parent is *NOT* guaranteed to be stable here - we are not
holding dentry->d_lock.  So it can change; what it can't do is to
change to or from the pointer we have in 'parent'.

IOW, the value of dentry->d_parent isn't stable; the result of
comparison, OTOH, is.

I also think that READ_ONCE would be useless here - reordering
can't happen due to compiler barrier in spin_lock() and memory
barriers should be provided by ACQUIRE/RELEASE on parent->d_lock.

We are observing a result of some store.  If we fetch the value
equal to 'parent', that store had been under parent->d_lock *AND*
any subsequent store changing the value of dentry->d_parent away
from 'parent' would also have been under parent->d_lock.  So
either it hasn't happened yet, or we would've observed its
results, since we are holding parent->lock ourselves.

If we fetch the value not equal to 'parent', any subsequent store
making dentry->d_parent equal to parent would've been done under
parent->d_lock, so again, it either has not happened yet, or we
would've observed its results.

FWIW, the whole point here is that we can't grab the lock sufficient
to stabilize dentry->d_parent here - not until we'd verified that
dentry still has the same parent.  We could play with trylock, but
AFAICS the trick above is sufficient (and gets fewer retries).

What I'm not certain about is whether we need READ_ONCE() on the
other fetch in there.  Suppose that gets replaced with plain
assignment; what problems could that cause?  It obviously can't
get reordered past the spin_lock() - the address of spinlock
is derived from the value we fetch.  And in case of retry...
wouldn't spin_unlock(parent->d_lock) we do there supply a compiler
barrier?

I realize that READ_ONCE() gives smp_read_barrier_depends(), but
do we need it here?