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-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
