On Tue, 2017-04-11 at 12:50 -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.
>
Which may not come for a LONG time, if the client has other activity
going on. A signalled process is not enough to shut down the client, in
general.
> 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.
>
> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
> fs/nfs/file.c | 10 +++++-----
> fs/nfs/nfs3proc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> fs/nfs/nfs4proc.c | 9 +++++++++
> 3 files changed, 67 insertions(+), 6 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 03b3c3de28f1..ce06ae0a25cf 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -865,12 +865,63 @@ 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;
> + if (l_ctx && test_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags)) {
> + get_nfs_open_context(l_ctx->open_context);
> + nfs_get_lock_context(l_ctx->open_context);
> + }
> +}
> +
> +bool nfs3_nlm_unlock_prepare(struct rpc_task *task, void *data)
> +{
> + struct nfs_lock_context *l_ctx = data;
> + if (l_ctx && test_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags))
> + return nfs_async_iocounter_wait(task, l_ctx);
> + return false;
> +
> +}
> +
> +void nfs3_nlm_release_call(void *data)
> +{
> + struct nfs_lock_context *l_ctx = data;
> + struct nfs_open_context *ctx;
> + if (l_ctx && test_bit(NFS_CONTEXT_UNLOCK, &l_ctx->open_context->flags)) {
> + ctx = l_ctx->open_context;
> + nfs_put_lock_context(l_ctx);
> + put_nfs_open_context(ctx);
> + }
> +}
> +
> +const struct nlmclnt_operations nlmclnt_fl_close_lock_ops = {
> + .nlmclnt_alloc_call = nfs3_nlm_alloc_call,
> + .nlmclnt_unlock_prepare = nfs3_nlm_unlock_prepare,
> + .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;
> + struct nfs_open_context *ctx = nfs_file_open_context(filp);
> + int status;
>
> - return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, NULL);
> + if (fl->fl_flags & FL_CLOSE) {
> + l_ctx = nfs_get_lock_context(ctx);
> + if (IS_ERR(l_ctx))
> + l_ctx = NULL;
> + else
> + set_bit(NFS_CONTEXT_UNLOCK, &ctx->flags);
> + }
> +
> + status = nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl, l_ctx);
> +
> + if (l_ctx)
> + nfs_put_lock_context(l_ctx);
> +
> + return status;
> }
>
> static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
> @@ -921,6 +972,7 @@ const struct nfs_rpc_ops nfs_v3_clientops = {
> .dir_inode_ops = &nfs3_dir_inode_operations,
> .file_inode_ops = &nfs3_file_inode_operations,
> .file_ops = &nfs_file_operations,
> + .nlmclnt_ops = &nlmclnt_fl_close_lock_ops,
> .getroot = nfs3_proc_get_root,
> .submount = nfs_submount,
> .try_mount = nfs_try_mount,
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 91f88bfbbe79..171ed89d243f 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5906,6 +5906,7 @@ struct nfs4_unlockdata {
> 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;
> @@ -5930,6 +5931,7 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
> 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,6 +5942,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);
> + nfs_put_lock_context(calldata->l_ctx);
> put_nfs_open_context(calldata->ctx);
> kfree(calldata);
> }
> @@ -5981,6 +5984,10 @@ static void nfs4_locku_prepare(struct rpc_task *task, void *data)
> {
> struct nfs4_unlockdata *calldata = data;
>
> + if (test_bit(NFS_CONTEXT_UNLOCK, &calldata->l_ctx->open_context->flags) &&
> + 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);
> @@ -6032,6 +6039,8 @@ static struct rpc_task *nfs4_do_unlck(struct file_lock *fl,
> * canceled lock is passed in, and it won't be an unlock.
> */
> fl->fl_type = F_UNLCK;
> + if (fl->fl_flags & FL_CLOSE)
> + set_bit(NFS_CONTEXT_UNLOCK, &ctx->flags);
>
> data = nfs4_alloc_unlockdata(fl, ctx, lsp, seqid);
> if (data == NULL) {
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
