Return-Path: Message-ID: <1491567742.2745.10.camel@poochiereds.net> Subject: Re: [PATCH 6/6] NFS: Always wait for I/O completion before unlock From: Jeff Layton To: Benjamin Coddington , Trond Myklebust , Anna Schumaker , bfields@fieldses.org, Miklos Szeredi , Alexander Viro Cc: linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org Date: Fri, 07 Apr 2017 08:22:22 -0400 In-Reply-To: <0351835c423970c9171e446e918ac3d3d2fe854e.1491477181.git.bcodding@redhat.com> References: <0351835c423970c9171e446e918ac3d3d2fe854e.1491477181.git.bcodding@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: On Thu, 2017-04-06 at 07:23 -0400, Benjamin Coddington wrote: > NFS attempts to wait for read and write completion before unlocking in > order to ensure that the data returned was protected by the lock. When > this waiting is interrupted by a signal, the unlock may be skipped, and > messages similar to the following are seen in the kernel ring buffer: > > [20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3: > [20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183 > [20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185 > > For NFSv3, the missing unlock will cause the server to refuse conflicting > locks indefinitely. For NFSv4, the leftover lock will be removed by the > server after the lease timeout. > > This patch fixes this issue by skipping the usual wait in > nfs_iocounter_wait if the FL_CLOSE flag is set when signaled. Instead, the > wait happens in the unlock RPC task on the NFS UOC rpc_waitqueue. > > For NFSv3, use lockd's new nlmclnt_operations along with > nfs_async_iocounter_wait to defer NLM's unlock task until the lock > context's iocounter reaches zero. > > For NFSv4, call nfs_async_iocounter_wait() directly from unlock's > current rpc_call_prepare. > Dare I ask about v2? :) Hmm actually, it looks like v2 could use the same operations as v3. I don't see anything protocol-specific there. > Signed-off-by: Benjamin Coddington > --- > fs/nfs/file.c | 10 +++++----- > fs/nfs/nfs3proc.c | 38 +++++++++++++++++++++++++++++++++++++- > fs/nfs/nfs4proc.c | 10 +++++++--- > 3 files changed, 49 insertions(+), 9 deletions(-) > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > index a490f45df4db..df695f52bb9d 100644 > --- a/fs/nfs/file.c > +++ b/fs/nfs/file.c > @@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local) > if (!IS_ERR(l_ctx)) { > status = nfs_iocounter_wait(l_ctx); > nfs_put_lock_context(l_ctx); > - if (status < 0) > + /* NOTE: special case > + * If we're signalled while cleaning up locks on process exit, we > + * still need to complete the unlock. > + */ > + if (status < 0 && !(fl->fl_flags & FL_CLOSE)) > return status; > } > > - /* NOTE: special case > - * If we're signalled while cleaning up locks on process exit, we > - * still need to complete the unlock. > - */ > /* > * Use local locking if mounted with "-onolock" or with appropriate > * "-olocal_lock=" > diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c > index 086623ab07b1..0e21306bfed9 100644 > --- a/fs/nfs/nfs3proc.c > +++ b/fs/nfs/nfs3proc.c > @@ -865,12 +865,48 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess > msg->rpc_proc = &nfs3_procedures[NFS3PROC_COMMIT]; > } > > +void nfs3_nlm_alloc_call(void *data) > +{ > + struct nfs_lock_context *l_ctx = data; > + nfs_get_lock_context(l_ctx->open_context); > +} > + > +void nfs3_nlm_release_call(void *data) > +{ > + struct nfs_lock_context *l_ctx = data; > + nfs_put_lock_context(l_ctx); > +} > + > +const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = { > + .nlmclnt_alloc_call = nfs3_nlm_alloc_call, > + .nlmclnt_unlock_prepare = nfs_async_iocounter_wait, > + .nlmclnt_release_call = nfs3_nlm_release_call, > +}; > + > static int > nfs3_proc_lock(struct file *filp, int cmd, struct file_lock *fl) > { > struct inode *inode = file_inode(filp); > + struct nfs_lock_context *l_ctx = NULL; > + const struct nlmclnt_operations *nlmclnt_ops = NULL; > + int status; > > - return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL, NULL); > + if (fl->fl_flags & FL_CLOSE) { > + l_ctx = nfs_get_lock_context(nfs_file_open_context(filp)); > + if (IS_ERR(l_ctx)) { > + l_ctx = NULL; > + } else { > + set_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags); > + nlmclnt_ops = &nlmclnt_fl_close_lock_ops; FWIW, I'm not a huge fan of using the pointer to indicate whether to run the operations or not. IMO, it'd be cleaner to: 1) store the pointer to the operations struct in the nlm_host, pass it a pointer to it in the nlmclnt_initdata. 2) make the decision to do the operations or not based on the setting of NFS_CONTEXT_UNLOCK in the callbacks themselves. If it's not set, just return quickly without doing anything. > + } > + } > + > + status = nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, > + nlmclnt_ops, l_ctx); > + if (l_ctx) > + nfs_put_lock_context(l_ctx); > + > + return status; > } > > static int nfs3_have_delegation(struct inode *inode, fmode_t flags) > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 91f88bfbbe79..e46cc2be60e2 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -5905,7 +5905,7 @@ struct nfs4_unlockdata { > struct nfs_locku_args arg; > struct nfs_locku_res res; > struct nfs4_lock_state *lsp; > - struct nfs_open_context *ctx; > + struct nfs_lock_context *l_ctx; > struct file_lock fl; > struct nfs_server *server; > unsigned long timestamp; > @@ -5929,7 +5929,7 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl, > p->lsp = lsp; > atomic_inc(&lsp->ls_count); > /* Ensure we don't close file until we're done freeing locks! */ > - p->ctx = get_nfs_open_context(ctx); > + p->l_ctx = nfs_get_lock_context(ctx); > memcpy(&p->fl, fl, sizeof(p->fl)); > p->server = NFS_SERVER(inode); > return p; > @@ -5940,7 +5940,7 @@ static void nfs4_locku_release_calldata(void *data) > struct nfs4_unlockdata *calldata = data; > nfs_free_seqid(calldata->arg.seqid); > nfs4_put_lock_state(calldata->lsp); > - put_nfs_open_context(calldata->ctx); > + nfs_put_lock_context(calldata->l_ctx); > kfree(calldata); > } > > @@ -5981,6 +5981,10 @@ static void nfs4_locku_prepare(struct rpc_task *task, void *data) > { > struct nfs4_unlockdata *calldata = data; > > + if (calldata->fl.fl_flags & FL_CLOSE && > + nfs_async_iocounter_wait(task, calldata->l_ctx)) > + return; > + > if (nfs_wait_on_sequence(calldata->arg.seqid, task) != 0) > goto out_wait; > nfs4_stateid_copy(&calldata->arg.stateid, &calldata->lsp->ls_stateid); -- Jeff Layton