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