| Message ID | 1372883132-23410-1-git-send-email-Waiman.Long@hp.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jul 03, 2013 at 04:25:32PM -0400, Waiman Long wrote: > There is no change in logic and everything should just work. > - spin_lock(&file->f_path.dentry->d_lock); > + d_lock(file->f_path.dentry); > if (!d_unhashed(file->f_path.dentry)) > clnt = RPC_I(inode)->private; > if (clnt != NULL && atomic_inc_not_zero(&clnt->cl_count)) { > - spin_unlock(&file->f_path.dentry->d_lock); > + d_unlock(file->f_path.dentry); Could somebody explain WTF is being protected here? It's not ->private - that gets set (and, more importantly, cleared) without ->d_lock in sight. Trond, that seems to be your code from about three years ago (introduced in "SUNRPC: Fix a race in rpc_info_open"). What's going on there? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-07-04 at 05:20 +0100, Al Viro wrote: > On Wed, Jul 03, 2013 at 04:25:32PM -0400, Waiman Long wrote: > > There is no change in logic and everything should just work. > > > - spin_lock(&file->f_path.dentry->d_lock); > > + d_lock(file->f_path.dentry); > > if (!d_unhashed(file->f_path.dentry)) > > clnt = RPC_I(inode)->private; > > if (clnt != NULL && atomic_inc_not_zero(&clnt->cl_count)) { > > - spin_unlock(&file->f_path.dentry->d_lock); > > + d_unlock(file->f_path.dentry); > > Could somebody explain WTF is being protected here? It's not ->private - > that gets set (and, more importantly, cleared) without ->d_lock in sight. > Trond, that seems to be your code from about three years ago (introduced > in "SUNRPC: Fix a race in rpc_info_open"). What's going on there? AFAICR we're using the fact that the dentry will remain hashed until we're in the process of freeing up the rpc_client. By testing that the dentry is hashed under the dentry->d_lock, we are assured that the non-NULL 'clnt' is still pointing to a valid rpc_client, and that it is safe to access clnt->cl_count. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index e7ce4b3..58bc128 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -427,14 +427,14 @@ rpc_info_open(struct inode *inode, struct file *file) if (!ret) { struct seq_file *m = file->private_data; - spin_lock(&file->f_path.dentry->d_lock); + d_lock(file->f_path.dentry); if (!d_unhashed(file->f_path.dentry)) clnt = RPC_I(inode)->private; if (clnt != NULL && atomic_inc_not_zero(&clnt->cl_count)) { - spin_unlock(&file->f_path.dentry->d_lock); + d_unlock(file->f_path.dentry); m->private = clnt; } else { - spin_unlock(&file->f_path.dentry->d_lock); + d_unlock(file->f_path.dentry); single_release(inode, file); ret = -EINVAL; }
Because of the changes made in dcache.h header file, files that use the d_lock field of the dentry structure need to be changed accordingly. All the d_lock's spin_lock() and spin_unlock() calls are replaced by the corresponding d_lock() and d_unlock() calls. There is no change in logic and everything should just work. Signed-off-by: Waiman Long <Waiman.Long@hp.com> --- net/sunrpc/rpc_pipe.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)