Re: dcache locking question

["Paul E. McKenney" <paulmck@linux.ibm.com>]

On Sun, Mar 17, 2019 at 12:18:40AM +0000, Al Viro wrote:
> 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'.

Color me blind to "dentry" vs. "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).

I can come up with sequences of values that could mimic the value of
'parent', but that assumes load tearing.  I -have- seen stores of constant
values be torn, but not stores of runtime-variable values and not loads.
Still, such tearing is permitted, and including the READ_ONCE() is making
it easier for things like thread sanitizers.  In addition, the READ_ONCE()
makes it clear that the value being loaded is unstable, which can be
useful documentation.

And yes, I am a bit paranoid about this sort of thing.  Something about
talking to a lot of compiler writers and standards committee members...

But at the end of the day, you are the maintainer of this code.

> 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?

Plus all callers to a spin_trylock() before invoking this function,
which does prevent the compiler from repeating the read (no reason to
in this case) or fusing with some other read.  Of course, I still like
the documentation and sanitizer-tooling benefits.

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

We need smp_read_barrier_depends() if we dereference the loaded pointer
using some operation that did not imply memory ordering, which does
happen, for example, in ecryptfs_symlink().  But before the caller has
a chance to do that, there is that spin_lock_nested(), which provides
the needed ordering.

							Thanx, Paul