On Mon, 28 Dec 2015, Jeff Layton wrote:
> On Mon, 28 Dec 2015 11:27:59 -0500
> Benjamin Coddington <bcodding@xxxxxxxxxx> 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 <bcodding@xxxxxxxxxx>
>
>
> 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?
Patch 9 sets up the only time we'll want to perform an unlock without a
filp, and that is when that unlock has been deferred due to outstanding IO.
When the IO completes, nfs_iocounter_dec() is called while holding a
reference to a nfs_lock_context containing a reference to the
nfs_open_context, and that final unlock is done while holding that
reference.
Ben
> > ---
> > 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 <jeff.layton@xxxxxxxxxxxxxxx>
>
--
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
