Re: RCU condition checks

[Trond Myklebust]

On Wed, 2010-04-07 at 10:10 -0700, Paul E. McKenney wrote: 
> On Wed, Apr 07, 2010 at 05:35:30PM +0100, David Howells wrote:
> > Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > > Why is there a need for 'c'?
> > > 
> > > An example use is where rcu_access_pointer() is legal because we are
> > > either initializing or cleaning up, so that no other CPU has access
> > > to the pointer.  In these cases, you might do something like:
> > > 
> > > 	q = rcu_access_pointer(p->a, p->refcnt == 0);
> > 
> > I think the main problem I have with this is that the fact that p->refcnt
> > should be 0 here is unrelated to the fact that we're wanting to look at the
> > value of p->a.  I'd say that this should be two separate statements, for
> > example:
> > 
> > 	ASSERT(p->refcnt == 0);
> > 	q = rcu_access_pointer(p->a);
> > 
> > I could see using a lockdep-managed ASSERT here would work, though.
> > 
> > The other problem I have with this is that I'm assuming rcu_access_pointer()
> > is simply for looking at the value of the pointer without dereferencing it -
> > in which case, is there any need for the lock-describing condition?
> 
> I agree that in many cases there won't be a reasonable condition.
> In which case, using "1" and an explanatory comment makes sense.
> In other cases, the fact that the value is zero can mean that no one
> else can possibly have a reference.
> 
> All that aside, I fully expect that uses of rcu_access_pointer() will
> require more than the usual code-review effort, as these sorts of
> unprotected accesses are notoriously error-prone.
> 
> > I agree, though, that:
> > 
> > 	q = rcu_dereference_check(p->a,
> > 				  rcu_read_lock_held() || (
> > 				   lockdep_is_held(p->lock) &&
> > 				   lockdep_is_held(q->lock)));
> > 
> > is a reasonable way of keeping the dereference and the lock checks together,
> > though that could equally well be written, say:
> > 
> > 	LOCKDEP_ASSERT(rcu_read_lock_held() || (
> > 		        lockdep_is_held(p->lock) &&
> > 			lockdep_is_held(q->lock)));
> > 	q = rcu_dereference_protected(p->a);
> > 
> > but combining those makes it easier to ensure people to write lock checking.
> 
> Glad you like it!
> 
> 							Thanx, Paul

What say we just list the conditions in the comments. I'm happy with
something like the following:

Trond
--------------------------------------------------------------------------------------------- 
NFSv4: Kill the bogus RCU dereferencing warnings in fs/nfs/delegation.c

From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>

Kill all the bogus warnings about RCU dereferencing, and document which
locks are protecting the pointer derefs.

Reported-by: David Howells <dhowells@xxxxxxxxxx>
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---

 fs/nfs/delegation.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)


diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 1567124..5a1a379 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -34,12 +34,17 @@ static void nfs_free_delegation_callback(struct rcu_head *head)
 	nfs_do_free_delegation(delegation);
 }
 
+/*
+ * At this point, we know that the nfsi->rwsem protects us against read
+ * access by the state recovery thread, so it is safe to assume nobody
+ * else is accessing delegation->cred.
+ */
 static void nfs_free_delegation(struct nfs_delegation *delegation)
 {
 	struct rpc_cred *cred;
 
-	cred = rcu_dereference(delegation->cred);
-	rcu_assign_pointer(delegation->cred, NULL);
+	cred = delegation->cred;
+	delegation->cred = NULL;
 	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
 	if (cred)
 		put_rpccred(cred);
@@ -166,12 +171,18 @@ static struct inode *nfs_delegation_grab_inode(struct nfs_delegation *delegation
 	return inode;
 }
 
+/*
+ * This function must be called with the nfs_client->cl_lock held to
+ * ensure that the value of nfsi->delegation is protected against
+ * modification by other threads.
+ */
 static struct nfs_delegation *nfs_detach_delegation_locked(struct nfs_inode *nfsi, const nfs4_stateid *stateid)
 {
-	struct nfs_delegation *delegation = rcu_dereference(nfsi->delegation);
+	struct nfs_delegation *delegation = nfsi->delegation;
 
 	if (delegation == NULL)
 		goto nomatch;
+	/* Lock out RCU-protected lookups. */
 	spin_lock(&delegation->lock);
 	if (stateid != NULL && memcmp(delegation->stateid.data, stateid->data,
 				sizeof(delegation->stateid.data)) != 0)
@@ -212,8 +223,9 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	delegation->flags = 1<<NFS_DELEGATION_REFERENCED;
 	spin_lock_init(&delegation->lock);
 
+	/* Protect nfsi->delegation against modification */
 	spin_lock(&clp->cl_lock);
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (nfsi->delegation != NULL) {
 		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
 					sizeof(delegation->stateid)) == 0 &&
 				delegation->type == nfsi->delegation->type) {
@@ -330,7 +342,7 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (nfsi->delegation != NULL) {
 		spin_lock(&clp->cl_lock);
 		delegation = nfs_detach_delegation_locked(nfsi, NULL);
 		spin_unlock(&clp->cl_lock);
@@ -346,7 +358,7 @@ int nfs_inode_return_delegation(struct inode *inode)
 	struct nfs_delegation *delegation;
 	int err = 0;
 
-	if (rcu_dereference(nfsi->delegation) != NULL) {
+	if (nfsi->delegation != NULL) {
 		spin_lock(&clp->cl_lock);
 		delegation = nfs_detach_delegation_locked(nfsi, NULL);
 		spin_unlock(&clp->cl_lock);

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