On Sun, Apr 11, 2010 at 06:57:23PM -0400, Trond Myklebust wrote:
> 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:
This does work at present, but the sparse-based checks that Arnd is
working on will generate warnings anywhere an RCU-protected pointer is
used as a normal pointer. So I believe that it would be good to get
these taken care of now, rather than having them break again in a very
short time.
Thanx, Paul
> 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
