Return-Path: Date: Mon, 28 Dec 2015 14:44:49 -0500 From: Jeff Layton To: Benjamin Coddington Cc: Trond Myklebust , Anna Schumaker , "J. Bruce Fields" , Christoph Hellwig , linux-nfs@vger.kernel.org Subject: Re: [PATCH v3 03/10] NFS: Pass nfs_open_context instead of file to the lock procs Message-ID: <20151228144449.6f4b1b77@tlielax.poochiereds.net> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII List-ID: On Mon, 28 Dec 2015 11:27:59 -0500 Benjamin Coddington wrote: > We only use the file struct pointer to obtain the nfs_open_context (or the > inode for v2/v3), so just pass that instead. This allows us to use the > lock procedure without a reference to a struct file which may not be > available while releasing locks after a file close. > > Signed-off-by: Benjamin Coddington Hmm... So what pins the nfs_open_context in place after the filp is gone? AFAICT, that gets zapped when the struct file goes away. Is there any chance that the ctx pointers you're passing around here might outlive the structure to which they point? > --- > fs/nfs/file.c | 9 ++++++--- > fs/nfs/nfs3proc.c | 4 ++-- > fs/nfs/nfs4proc.c | 4 +--- > fs/nfs/proc.c | 4 ++-- > include/linux/nfs_xdr.h | 2 +- > 5 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > index 1e4804b..5dec0fb 100644 > --- a/fs/nfs/file.c > +++ b/fs/nfs/file.c > @@ -711,6 +711,7 @@ static int > do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > { > struct inode *inode = filp->f_mapping->host; > + struct nfs_open_context *ctx = nfs_file_open_context(filp); > int status = 0; > unsigned int saved_type = fl->fl_type; > > @@ -728,7 +729,7 @@ do_getlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > if (is_local) > goto out_noconflict; > > - status = NFS_PROTO(inode)->lock(filp, cmd, fl); > + status = NFS_PROTO(inode)->lock(ctx, cmd, fl); > out: > return status; > out_noconflict: > @@ -745,6 +746,7 @@ static int > do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > { > struct inode *inode = filp->f_mapping->host; > + struct nfs_open_context *ctx = nfs_file_open_context(filp); > struct nfs_lock_context *l_ctx; > int status; > > @@ -771,7 +773,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > * "-olocal_lock=" > */ > if (!is_local) > - status = NFS_PROTO(inode)->lock(filp, cmd, fl); > + status = NFS_PROTO(inode)->lock(ctx, cmd, fl); > else > status = do_vfs_lock(filp, fl); > return status; > @@ -786,6 +788,7 @@ static int > do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > { > struct inode *inode = filp->f_mapping->host; > + struct nfs_open_context *ctx = nfs_file_open_context(filp); > int status; > > /* > @@ -801,7 +804,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > * "-olocal_lock=" > */ > if (!is_local) > - status = NFS_PROTO(inode)->lock(filp, cmd, fl); > + status = NFS_PROTO(inode)->lock(ctx, cmd, fl); > else > status = do_vfs_lock(filp, fl); > if (status < 0) > diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c > index cb28cce..a10e147 100644 > --- a/fs/nfs/nfs3proc.c > +++ b/fs/nfs/nfs3proc.c > @@ -866,9 +866,9 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess > } > > static int > -nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl) > +nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl) > { > - struct inode *inode = file_inode(filp); > + struct inode *inode = d_inode(ctx->dentry); > > return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl); > } > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 05ea1e1..784ba4e 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -6097,15 +6097,13 @@ static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock * > } > > static int > -nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request) > +nfs4_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *request) > { > - struct nfs_open_context *ctx; > struct nfs4_state *state; > unsigned long timeout = NFS4_LOCK_MINTIMEOUT; > int status; > > /* verify open state */ > - ctx = nfs_file_open_context(filp); > state = ctx->state; > > if (IS_GETLK(cmd)) { > diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c > index b417bbc..185deb9 100644 > --- a/fs/nfs/proc.c > +++ b/fs/nfs/proc.c > @@ -634,9 +634,9 @@ nfs_proc_commit_setup(struct nfs_commit_data *data, struct rpc_message *msg) > } > > static int > -nfs_proc_lock(struct file *filp, int cmd, struct file_lock *fl) > +nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl) > { > - struct inode *inode = file_inode(filp); > + struct inode *inode = d_inode(ctx->dentry); > > return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl); > } > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index 11bbae4..58c4682 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -1555,7 +1555,7 @@ struct nfs_rpc_ops { > void (*commit_setup) (struct nfs_commit_data *, struct rpc_message *); > void (*commit_rpc_prepare)(struct rpc_task *, struct nfs_commit_data *); > int (*commit_done) (struct rpc_task *, struct nfs_commit_data *); > - int (*lock)(struct file *, int, struct file_lock *); > + int (*lock)(struct nfs_open_context *, int, struct file_lock *); > int (*lock_check_bounds)(const struct file_lock *); > void (*clear_acl_cache)(struct inode *); > void (*close_context)(struct nfs_open_context *ctx, int); -- Jeff Layton