Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim() [ver #2]

[David Howells <dhowells@redhat.com>]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

> If you dont own a lock, and test a pointer, what guarantee do you have
> this pointer doesnt change right after you tested it ?

There are five possibilities:

 (1) A pointer points to something when you check, and still points to the
     same thing after you've gained the lock.

 (2) A pointer points to something when you check, and points to something
     else after you've gained the lock.

 (3) A pointer points to something when you check, and is NULL after you've
     gained the lock.

 (4) A pointer points to NULL when you check, and points to something after
     you've gained the lock.

 (5) A pointer points to NULL when you check, and points to NULL after you've
     gained the lock.

However, what if you _know_ that the pointer can only ever be made non-NULL
during initialisation, and may even be left unset?  That means possibility (4)
can never happen, and that possibility (5) can be detected by testing before
taking the lock.  Now, what if (5) is a common occurrence?  It might make
sense to make the test.

And what matter if the pointer _does_ change after you test it.  If it was
NULL before, it can only be NULL now - by the semantics defined for that
particular pointer.

> If *something* protects the pointer from being changed, then how can be
> expressed this fact ?
> 
> If nothing protects the pointer, why test it then, as result of test is
> unreliable ?

I think you may be misunderstanding the purpose of rcu_dereference().  It is
to make sure the reading and dereferencing of the pointer are correctly
ordered with respect to the setting up of the pointed to record and the
changing of the pointer.

There must be two memory accesses for the barrier implied to be of use.  In
nfs_inode_return_delegation() there aren't two memory accesses to order,
therefore the barrier is pointless.

> If NFS was using rcu_dereference(), it probably was for a reason, but if
> nobody can recall it, it was a wrong reason ?

I think it is incorrectly used.  Given that the rcu_dereference() in:

	if (rcu_dereference(nfsi->delegation) != NULL) {
		spin_lock(&clp->cl_lock);
		delegation = nfs_detach_delegation_locked(nfsi, NULL);
		spin_unlock(&clp->cl_lock);
		if (delegation != NULL)
			nfs_do_return_delegation(inode, delegation, 0);
	}

resolves to:

	_________p1 = nfsi->delegation;
	smp_read_barrier_depends();
	if (_________p1) {
		spin_lock(&clp->cl_lock); // implicit LOCK-class barrier
		==>nfs_detach_delegation_locked(nfsi, NULL);
		  [dereference nfsi->delegation]
		...
	}

do you actually need the smp_read_barrier_depends()?  You _have_ a barrier in
the form of the spin_lock().  In fact, the spin_lock() is avowedly sufficient
to protect accesses to and dereferences of nfsi->delegation, which means that:

	static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
	{
		struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
		...
	}

has no need of the internal barrier provided by rcu_dereference() either.

David