Prev: PROJECT
Next: PCMCIA: resource, fix lock imbalance
From: David Howells on 16 Mar 2010 12:40
Trond Myklebust <Trond.Myklebust(a)netapp.com> wrote:

> > + rcu_read_lock();
> > cred = rcu_dereference(delegation->cred);
> > rcu_assign_pointer(delegation->cred, NULL);
> > + rcu_read_unlock();
> > call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> > if (cred)
> > put_rpccred(cred);
>
> That's bogus. We're in the process of freeing the delegation, so we
> don't need to rely on rcu to read delegation->cred.
>
> Better to just convert that rcu_dereference() into an ordinary pointer
> dereference.

Now that I take a second look at this code, it looks wrong. You shouldn't
really start dissassembling the delegation record until the RCU callback
triggers as it may be in use by someone up to that point. If you know it
isn't in use at this point, why use call_rcu()?

> The spinlock already provides protection. Again we can just convert the
> rcu_dereference() into a pointer dereference.

That seems reasonable.

> We cannot hold the rcu read lock across the entire RPC call in
> nfs_do_return_delegation(). All we want to do above is to check that
> nfsi->delegation != NULL.

Good point.

However, that leads me to note that nfs_detach_delegation_locked() then must
be using rcu_dereference() unnecessarily. Also nfs_do_return_delegation()
must be too.

I think if all you're doing is checking the state of the pointer, you don't
need the interpolated memory barrier, since you've no need to synchronise what
the pointer points to.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 16 Mar 2010 13:50
Trond Myklebust <Trond.Myklebust(a)netapp.com> wrote:

> > The lock is probably held here, in which case something like the
> > following would work well without needing the artificial rcu_read_lock()
> > and rcu_read_unlock():
>
> No. The lock is not held here. At this point, the delegation has been
> detached from the inode that pointed to it, and so we can free up its
> contents.
>
> We still need the call_rcu() to free up the allocated memory in order to
> ensure that some process doing lockless traversal of the
> clp->cl_delegations list doesn't crash.

In that case, surely you can't detach the credentials pointer until the
callback is invoked?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: PROJECT
Next: PCMCIA: resource, fix lock imbalance