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

[Paul E. McKenney]

On Wed, Mar 31, 2010 at 11:53:28PM +0100, David Howells wrote:
> Eric Dumazet <eric.dumazet@xxxxxxxxx> 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);
> 	}

And nfs_detach_delegation_locked() rechecks nfsi->delegation() under
the lock, so this is a legitimate use.

The pointer is not held constant, but any changes will be accounted
for and handled correctly.  So I would argue that the pointer value is
in fact protected by the recheck-under-lock algorithm used here.

							Thanx, Paul

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/