* [PATCH 01/10] NFS: keep nfs4_state for nfs4_lock_state cleanup
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 02/10] NFS4: remove a redundant lock range checks Benjamin Coddington
` (8 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
If we fail to release a lock due to an error or signal on file close, we
might later free the lock if another lock replaces it. Hold a reference to
the nfs4_state to ensure it is not released before freeing the
nfs4_lock_state.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/nfs/nfs4state.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index d854693..624c1e0 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -827,6 +827,7 @@ static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, f
return NULL;
nfs4_init_seqid_counter(&lsp->ls_seqid);
atomic_set(&lsp->ls_count, 1);
+ atomic_inc(&state->count);
lsp->ls_state = state;
lsp->ls_owner = fl_owner;
lsp->ls_seqid.owner_id = ida_simple_get(&server->lockowner_id, 0, 0, GFP_NOFS);
@@ -903,6 +904,7 @@ void nfs4_put_lock_state(struct nfs4_lock_state *lsp)
clp->cl_mvops->free_lock_state(server, lsp);
} else
nfs4_free_lock_state(server, lsp);
+ nfs4_put_open_state(state);
}
static void nfs4_fl_copy_lock(struct file_lock *dst, struct file_lock *src)
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 02/10] NFS4: remove a redundant lock range checks
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
2015-10-14 18:23 ` [PATCH 01/10] NFS: keep nfs4_state for nfs4_lock_state cleanup Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 03/10] NFS: Move the flock open mode check into nfs_flock() Benjamin Coddington
` (7 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
flock64_to_posix_lock() is already doing this check
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/nfs/nfs4proc.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5133bb1..75223d2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6120,9 +6120,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
ctx = nfs_file_open_context(filp);
state = ctx->state;
- if (request->fl_start < 0 || request->fl_end < 0)
- return -EINVAL;
-
if (IS_GETLK(cmd)) {
if (state != NULL)
return nfs4_proc_getlk(state, F_GETLK, request);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 03/10] NFS: Move the flock open mode check into nfs_flock()
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
2015-10-14 18:23 ` [PATCH 01/10] NFS: keep nfs4_state for nfs4_lock_state cleanup Benjamin Coddington
2015-10-14 18:23 ` [PATCH 02/10] NFS4: remove a redundant lock range checks Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 04/10] NFS: Pass nfs_open_context instead of file to the lock procs Benjamin Coddington
` (6 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
We only need to check lock exclusive/shared types against open mode when
flock() is used on NFS, so move it into the flock-specific path instead of
checking it for all locks.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/nfs/file.c | 15 +++++++++++++++
fs/nfs/nfs4proc.c | 13 -------------
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index c0f9b1e..57fefd2 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -904,6 +904,21 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
/* We're simulating flock() locks using posix locks on the server */
if (fl->fl_type == F_UNLCK)
return do_unlk(filp, cmd, fl, is_local);
+
+ /*
+ * Don't rely on the VFS having checked the file open mode,
+ * since it won't do this for flock() locks.
+ */
+ switch (fl->fl_type) {
+ case F_RDLCK:
+ if (!(filp->f_mode & FMODE_READ))
+ return -EBADF;
+ break;
+ case F_WRLCK:
+ if (!(filp->f_mode & FMODE_WRITE))
+ return -EBADF;
+ }
+
return do_setlk(filp, cmd, fl, is_local);
}
EXPORT_SYMBOL_GPL(nfs_flock);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 75223d2..28b771c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6137,19 +6137,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
if (state == NULL)
return -ENOLCK;
- /*
- * Don't rely on the VFS having checked the file open mode,
- * since it won't do this for flock() locks.
- */
- switch (request->fl_type) {
- case F_RDLCK:
- if (!(filp->f_mode & FMODE_READ))
- return -EBADF;
- break;
- case F_WRLCK:
- if (!(filp->f_mode & FMODE_WRITE))
- return -EBADF;
- }
do {
status = nfs4_proc_setlk(state, cmd, request);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 04/10] NFS: Pass nfs_open_context instead of file to the lock procs
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (2 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 03/10] NFS: Move the flock open mode check into nfs_flock() Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 05/10] NFSv4: Pass nfs_open_context instead of nfs4_state to nfs4_proc_unlck() Benjamin Coddington
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
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@redhat.com>
---
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 57fefd2..319847c 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:
@@ -756,6 +757,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;
@@ -782,7 +784,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;
@@ -797,6 +799,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;
/*
@@ -812,7 +815,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 28b771c..18a1ee7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6109,15 +6109,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 52faf7e..627ddc4 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1536,7 +1536,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);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 05/10] NFSv4: Pass nfs_open_context instead of nfs4_state to nfs4_proc_unlck()
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (3 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 04/10] NFS: Pass nfs_open_context instead of file to the lock procs Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 06/10] lockd: Plumb nfs_open_context into nlm client unlock Benjamin Coddington
` (4 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
nfs4_proc_unlk() acquires the nfs_open_context from the file_lock's
fl_file->private, but we will not always have a referenced fl_file in this
path. Instead, pass along the nfs_open_context from the call to
nfs4_proc_lock(). This allows us to use nfs4_proc_unlck() without a
valid reference in fl_file.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/nfs/nfs4proc.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 18a1ee7..c08f8ac 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5668,8 +5668,9 @@ static struct rpc_task *nfs4_do_unlck(struct file_lock *fl,
return rpc_run_task(&task_setup_data);
}
-static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *request)
+static int nfs4_proc_unlck(struct nfs_open_context *ctx, int cmd, struct file_lock *request)
{
+ struct nfs4_state *state = ctx->state;
struct inode *inode = state->inode;
struct nfs4_state_owner *sp = state->owner;
struct nfs_inode *nfsi = NFS_I(inode);
@@ -5705,7 +5706,7 @@ static int nfs4_proc_unlck(struct nfs4_state *state, int cmd, struct file_lock *
status = -ENOMEM;
if (IS_ERR(seqid))
goto out;
- task = nfs4_do_unlck(request, nfs_file_open_context(request->fl_file), lsp, seqid);
+ task = nfs4_do_unlck(request, ctx, lsp, seqid);
status = PTR_ERR(task);
if (IS_ERR(task))
goto out;
@@ -6129,7 +6130,7 @@ nfs4_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *request)
if (request->fl_type == F_UNLCK) {
if (state != NULL)
- return nfs4_proc_unlck(state, cmd, request);
+ return nfs4_proc_unlck(ctx, cmd, request);
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 06/10] lockd: Plumb nfs_open_context into nlm client unlock
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (4 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 05/10] NFSv4: Pass nfs_open_context instead of nfs4_state to nfs4_proc_unlck() Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 07/10] lockd: Send the inode to nlmclnt_setlockargs() Benjamin Coddington
` (3 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
We cannot depend on a file_lock->fl_file still being around while performing
unlocks for NFSv2 and NFSv3. Pass along the nfs_open_context to
provide the necessary inode and credential to complete the unlock.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/lockd/clntproc.c | 14 ++++++++------
fs/nfs/nfs3proc.c | 4 +---
fs/nfs/proc.c | 4 +---
include/linux/lockd/bind.h | 3 ++-
4 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index acd3947..8fe50a9 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -25,7 +25,7 @@
static int nlmclnt_test(struct nlm_rqst *, struct file_lock *);
static int nlmclnt_lock(struct nlm_rqst *, struct file_lock *);
-static int nlmclnt_unlock(struct nlm_rqst *, struct file_lock *);
+static int nlmclnt_unlock(struct nfs_open_context *nfs, struct nlm_rqst *, struct file_lock *);
static int nlm_stat_to_errno(__be32 stat);
static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host);
static int nlmclnt_cancel(struct nlm_host *, int , struct file_lock *);
@@ -147,13 +147,15 @@ static void nlmclnt_release_lockargs(struct nlm_rqst *req)
/**
* nlmclnt_proc - Perform a single client-side lock request
- * @host: address of a valid nlm_host context representing the NLM server
+ * @ctx: address of the nfs_open_context for lock to operate upon
* @cmd: fcntl-style file lock operation to perform
* @fl: address of arguments for the lock operation
*
*/
-int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
+int nlmclnt_proc(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
{
+ struct inode *inode = d_inode(ctx->dentry);
+ struct nlm_host *host = NFS_SERVER(inode)->nlm_host;
struct nlm_rqst *call;
int status;
@@ -175,7 +177,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl)
call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
status = nlmclnt_lock(call, fl);
} else
- status = nlmclnt_unlock(call, fl);
+ status = nlmclnt_unlock(ctx, call, fl);
} else if (IS_GETLK(cmd))
status = nlmclnt_test(call, fl);
else
@@ -657,7 +659,7 @@ nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
* UNLOCK: remove an existing lock
*/
static int
-nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
+nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_lock *fl)
{
struct nlm_host *host = req->a_host;
struct nlm_res *resp = &req->a_res;
@@ -680,7 +682,7 @@ nlmclnt_unlock(struct nlm_rqst *req, struct file_lock *fl)
}
atomic_inc(&req->a_count);
- status = nlmclnt_async_call(nfs_file_cred(fl->fl_file), req,
+ status = nlmclnt_async_call(ctx->cred, req,
NLMPROC_UNLOCK, &nlmclnt_unlock_ops);
if (status < 0)
goto out;
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index a10e147..2941ab7 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -868,9 +868,7 @@ static void nfs3_proc_commit_setup(struct nfs_commit_data *data, struct rpc_mess
static int
nfs3_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
{
- struct inode *inode = d_inode(ctx->dentry);
-
- return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+ return nlmclnt_proc(ctx, cmd, fl);
}
static int nfs3_have_delegation(struct inode *inode, fmode_t flags)
diff --git a/fs/nfs/proc.c b/fs/nfs/proc.c
index 185deb9..17d892e 100644
--- a/fs/nfs/proc.c
+++ b/fs/nfs/proc.c
@@ -636,9 +636,7 @@ nfs_proc_commit_setup(struct nfs_commit_data *data, struct rpc_message *msg)
static int
nfs_proc_lock(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
{
- struct inode *inode = d_inode(ctx->dentry);
-
- return nlmclnt_proc(NFS_SERVER(inode)->nlm_host, cmd, fl);
+ return nlmclnt_proc(ctx, cmd, fl);
}
/* Helper functions for NFS lock bounds checking */
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 4d24d64..1d62e85 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -18,6 +18,7 @@
/* Dummy declarations */
struct svc_rqst;
+struct nfs_open_context;
/*
* This is the set of functions for lockd->nfsd communication
@@ -52,7 +53,7 @@ struct nlmclnt_initdata {
extern struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init);
extern void nlmclnt_done(struct nlm_host *host);
-extern int nlmclnt_proc(struct nlm_host *host, int cmd,
+extern int nlmclnt_proc(struct nfs_open_context *ctx, int cmd,
struct file_lock *fl);
extern int lockd_up(struct net *net);
extern void lockd_down(struct net *net);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 07/10] lockd: Send the inode to nlmclnt_setlockargs()
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (5 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 06/10] lockd: Plumb nfs_open_context into nlm client unlock Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 18:23 ` [PATCH 08/10] NFS: Move do_vfs_lock to shared inline Benjamin Coddington
` (2 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
Instead of using fl_file to reference the inode in nlmclnt_setlockargs() to
set the filehandle, send the inode as an argument. That way, we can use
nlmclnt_proc without having a valid fl_file, which can happen if we are
releasing locks after a close.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/lockd/clntproc.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 8fe50a9..4a35e7c 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -121,14 +121,15 @@ static struct nlm_lockowner *nlm_find_lockowner(struct nlm_host *host, fl_owner_
/*
* Initialize arguments for TEST/LOCK/UNLOCK/CANCEL calls
*/
-static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl)
+static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl,
+ struct inode *inode)
{
struct nlm_args *argp = &req->a_args;
struct nlm_lock *lock = &argp->lock;
char *nodename = req->a_host->h_rpcclnt->cl_nodename;
nlmclnt_next_cookie(&argp->cookie);
- memcpy(&lock->fh, NFS_FH(file_inode(fl->fl_file)), sizeof(struct nfs_fh));
+ memcpy(&lock->fh, NFS_FH(inode), sizeof(struct nfs_fh));
lock->caller = nodename;
lock->oh.data = req->a_owner;
lock->oh.len = snprintf(req->a_owner, sizeof(req->a_owner), "%u@%s",
@@ -170,7 +171,7 @@ int nlmclnt_proc(struct nfs_open_context *ctx, int cmd, struct file_lock *fl)
return -ENOMEM;
}
/* Set up the argument struct */
- nlmclnt_setlockargs(call, fl);
+ nlmclnt_setlockargs(call, fl, inode);
if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
if (fl->fl_type != F_UNLCK) {
@@ -629,7 +630,7 @@ nlmclnt_reclaim(struct nlm_host *host, struct file_lock *fl,
req->a_host = host;
/* Set up the argument struct */
- nlmclnt_setlockargs(req, fl);
+ nlmclnt_setlockargs(req, fl, file_inode(fl->fl_file));
req->a_args.reclaim = 1;
status = nlmclnt_call(nfs_file_cred(fl->fl_file), req, NLMPROC_LOCK);
@@ -755,7 +756,7 @@ static int nlmclnt_cancel(struct nlm_host *host, int block, struct file_lock *fl
return -ENOMEM;
req->a_flags = RPC_TASK_ASYNC;
- nlmclnt_setlockargs(req, fl);
+ nlmclnt_setlockargs(req, fl, file_inode(fl->fl_file));
req->a_args.block = block;
atomic_inc(&req->a_count);
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (6 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 07/10] lockd: Send the inode to nlmclnt_setlockargs() Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 19:55 ` Jeff Layton
2015-10-14 18:23 ` [PATCH 09/10] locks: Use more file_inode and fix a comment Benjamin Coddington
2015-10-14 18:23 ` [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE Benjamin Coddington
9 siblings, 1 reply; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
Send the inode instead of the file_lock to do_vfs_lock() in
fs/lockd/clintproc.c (because there may not be a fl_file the unlock path).
Now that we have posix_lock_inode_wait() the three copies of do_vfs_lock()
can be collapsed.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/lockd/clntproc.c | 25 +++++--------------------
fs/nfs/file.c | 20 ++------------------
fs/nfs/nfs4proc.c | 16 ----------------
include/linux/fs.h | 16 ++++++++++++++++
4 files changed, 23 insertions(+), 54 deletions(-)
diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index 4a35e7c..bd484f0 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -475,22 +475,6 @@ static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *ho
fl->fl_ops = &nlmclnt_lock_ops;
}
-static int do_vfs_lock(struct file_lock *fl)
-{
- int res = 0;
- switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
- case FL_POSIX:
- res = posix_lock_file_wait(fl->fl_file, fl);
- break;
- case FL_FLOCK:
- res = flock_lock_file_wait(fl->fl_file, fl);
- break;
- default:
- BUG();
- }
- return res;
-}
-
/*
* LOCK: Try to create a lock
*
@@ -518,6 +502,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
struct nlm_host *host = req->a_host;
struct nlm_res *resp = &req->a_res;
struct nlm_wait *block = NULL;
+ struct inode *inode = file_inode(fl->fl_file);
unsigned char fl_flags = fl->fl_flags;
unsigned char fl_type;
int status = -ENOLCK;
@@ -527,7 +512,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
req->a_args.state = nsm_local_state;
fl->fl_flags |= FL_ACCESS;
- status = do_vfs_lock(fl);
+ status = do_vfs_lock(inode, fl);
fl->fl_flags = fl_flags;
if (status < 0)
goto out;
@@ -577,7 +562,7 @@ again:
}
/* Ensure the resulting lock will get added to granted list */
fl->fl_flags |= FL_SLEEP;
- if (do_vfs_lock(fl) < 0)
+ if (do_vfs_lock(inode, fl) < 0)
printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
up_read(&host->h_rwsem);
fl->fl_flags = fl_flags;
@@ -607,7 +592,7 @@ out_unlock:
fl_type = fl->fl_type;
fl->fl_type = F_UNLCK;
down_read(&host->h_rwsem);
- do_vfs_lock(fl);
+ do_vfs_lock(inode, fl);
up_read(&host->h_rwsem);
fl->fl_type = fl_type;
fl->fl_flags = fl_flags;
@@ -674,7 +659,7 @@ nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_l
*/
fl->fl_flags |= FL_EXISTS;
down_read(&host->h_rwsem);
- status = do_vfs_lock(fl);
+ status = do_vfs_lock(d_inode(ctx->dentry), fl);
up_read(&host->h_rwsem);
fl->fl_flags = fl_flags;
if (status == -ENOENT) {
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 319847c..d16c50f 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -737,22 +737,6 @@ out_noconflict:
goto out;
}
-static int do_vfs_lock(struct file *file, struct file_lock *fl)
-{
- int res = 0;
- switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
- case FL_POSIX:
- res = posix_lock_file_wait(file, fl);
- break;
- case FL_FLOCK:
- res = flock_lock_file_wait(file, fl);
- break;
- default:
- BUG();
- }
- return res;
-}
-
static int
do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
{
@@ -786,7 +770,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
if (!is_local)
status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
else
- status = do_vfs_lock(filp, fl);
+ status = do_vfs_lock(inode, fl);
return status;
}
@@ -817,7 +801,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
if (!is_local)
status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
else
- status = do_vfs_lock(filp, fl);
+ status = do_vfs_lock(inode, fl);
if (status < 0)
goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c08f8ac..6b19307 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5511,22 +5511,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
return err;
}
-static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
-{
- int res = 0;
- switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
- case FL_POSIX:
- res = posix_lock_inode_wait(inode, fl);
- break;
- case FL_FLOCK:
- res = flock_lock_inode_wait(inode, fl);
- break;
- default:
- BUG();
- }
- return res;
-}
-
struct nfs4_unlockdata {
struct nfs_locku_args arg;
struct nfs_locku_res res;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 72d8a84..165577a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1225,6 +1225,22 @@ static inline int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
return flock_lock_inode_wait(file_inode(filp), fl);
}
+static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
+{
+ int res = 0;
+ switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
+ case FL_POSIX:
+ res = posix_lock_inode_wait(inode, fl);
+ break;
+ case FL_FLOCK:
+ res = flock_lock_inode_wait(inode, fl);
+ break;
+ default:
+ BUG();
+ }
+ return res;
+}
+
struct fasync_struct {
spinlock_t fa_lock;
int magic;
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-14 18:23 ` [PATCH 08/10] NFS: Move do_vfs_lock to shared inline Benjamin Coddington
@ 2015-10-14 19:55 ` Jeff Layton
2015-10-21 21:48 ` Trond Myklebust
0 siblings, 1 reply; 20+ messages in thread
From: Jeff Layton @ 2015-10-14 19:55 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, trond.myklebust, anna.schumaker
On Wed, 14 Oct 2015 14:23:35 -0400
Benjamin Coddington <bcodding@redhat.com> wrote:
> Send the inode instead of the file_lock to do_vfs_lock() in
> fs/lockd/clintproc.c (because there may not be a fl_file the unlock path).
> Now that we have posix_lock_inode_wait() the three copies of do_vfs_lock()
> can be collapsed.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/lockd/clntproc.c | 25 +++++--------------------
> fs/nfs/file.c | 20 ++------------------
> fs/nfs/nfs4proc.c | 16 ----------------
> include/linux/fs.h | 16 ++++++++++++++++
> 4 files changed, 23 insertions(+), 54 deletions(-)
>
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index 4a35e7c..bd484f0 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -475,22 +475,6 @@ static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *ho
> fl->fl_ops = &nlmclnt_lock_ops;
> }
>
> -static int do_vfs_lock(struct file_lock *fl)
> -{
> - int res = 0;
> - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> - case FL_POSIX:
> - res = posix_lock_file_wait(fl->fl_file, fl);
> - break;
> - case FL_FLOCK:
> - res = flock_lock_file_wait(fl->fl_file, fl);
> - break;
> - default:
> - BUG();
> - }
> - return res;
> -}
> -
> /*
> * LOCK: Try to create a lock
> *
> @@ -518,6 +502,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> struct nlm_host *host = req->a_host;
> struct nlm_res *resp = &req->a_res;
> struct nlm_wait *block = NULL;
> + struct inode *inode = file_inode(fl->fl_file);
> unsigned char fl_flags = fl->fl_flags;
> unsigned char fl_type;
> int status = -ENOLCK;
> @@ -527,7 +512,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> req->a_args.state = nsm_local_state;
>
> fl->fl_flags |= FL_ACCESS;
> - status = do_vfs_lock(fl);
> + status = do_vfs_lock(inode, fl);
> fl->fl_flags = fl_flags;
> if (status < 0)
> goto out;
> @@ -577,7 +562,7 @@ again:
> }
> /* Ensure the resulting lock will get added to granted list */
> fl->fl_flags |= FL_SLEEP;
> - if (do_vfs_lock(fl) < 0)
> + if (do_vfs_lock(inode, fl) < 0)
> printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
> up_read(&host->h_rwsem);
> fl->fl_flags = fl_flags;
> @@ -607,7 +592,7 @@ out_unlock:
> fl_type = fl->fl_type;
> fl->fl_type = F_UNLCK;
> down_read(&host->h_rwsem);
> - do_vfs_lock(fl);
> + do_vfs_lock(inode, fl);
> up_read(&host->h_rwsem);
> fl->fl_type = fl_type;
> fl->fl_flags = fl_flags;
> @@ -674,7 +659,7 @@ nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_l
> */
> fl->fl_flags |= FL_EXISTS;
> down_read(&host->h_rwsem);
> - status = do_vfs_lock(fl);
> + status = do_vfs_lock(d_inode(ctx->dentry), fl);
> up_read(&host->h_rwsem);
> fl->fl_flags = fl_flags;
> if (status == -ENOENT) {
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 319847c..d16c50f 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -737,22 +737,6 @@ out_noconflict:
> goto out;
> }
>
> -static int do_vfs_lock(struct file *file, struct file_lock *fl)
> -{
> - int res = 0;
> - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> - case FL_POSIX:
> - res = posix_lock_file_wait(file, fl);
> - break;
> - case FL_FLOCK:
> - res = flock_lock_file_wait(file, fl);
> - break;
> - default:
> - BUG();
> - }
> - return res;
> -}
> -
> static int
> do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> {
> @@ -786,7 +770,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> if (!is_local)
> status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> else
> - status = do_vfs_lock(filp, fl);
> + status = do_vfs_lock(inode, fl);
> return status;
> }
>
> @@ -817,7 +801,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> if (!is_local)
> status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> else
> - status = do_vfs_lock(filp, fl);
> + status = do_vfs_lock(inode, fl);
> if (status < 0)
> goto out;
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index c08f8ac..6b19307 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5511,22 +5511,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
> return err;
> }
>
> -static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> -{
> - int res = 0;
> - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> - case FL_POSIX:
> - res = posix_lock_inode_wait(inode, fl);
> - break;
> - case FL_FLOCK:
> - res = flock_lock_inode_wait(inode, fl);
> - break;
> - default:
> - BUG();
> - }
> - return res;
> -}
> -
> struct nfs4_unlockdata {
> struct nfs_locku_args arg;
> struct nfs_locku_res res;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 72d8a84..165577a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1225,6 +1225,22 @@ static inline int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
> return flock_lock_inode_wait(file_inode(filp), fl);
> }
>
> +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> +{
> + int res = 0;
> + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> + case FL_POSIX:
> + res = posix_lock_inode_wait(inode, fl);
> + break;
> + case FL_FLOCK:
> + res = flock_lock_inode_wait(inode, fl);
> + break;
> + default:
> + BUG();
> + }
> + return res;
> +}
> +
> struct fasync_struct {
> spinlock_t fa_lock;
> int magic;
Meh ok...a little large for an inline though. Maybe you should move
that into fs/nfs_common and have it as an exported symbol? I'm not too
religious about it though since we don't have many call sites.
--
Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-14 19:55 ` Jeff Layton
@ 2015-10-21 21:48 ` Trond Myklebust
2015-10-21 23:49 ` Jeff Layton
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Trond Myklebust @ 2015-10-21 21:48 UTC (permalink / raw)
To: Jeff Layton; +Cc: Benjamin Coddington, Linux NFS Mailing List, Anna Schumaker
On Wed, Oct 14, 2015 at 12:55 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
>
> On Wed, 14 Oct 2015 14:23:35 -0400
> Benjamin Coddington <bcodding@redhat.com> wrote:
>
> > Send the inode instead of the file_lock to do_vfs_lock() in
> > fs/lockd/clintproc.c (because there may not be a fl_file the unlock path).
> > Now that we have posix_lock_inode_wait() the three copies of do_vfs_lock()
> > can be collapsed.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> > fs/lockd/clntproc.c | 25 +++++--------------------
> > fs/nfs/file.c | 20 ++------------------
> > fs/nfs/nfs4proc.c | 16 ----------------
> > include/linux/fs.h | 16 ++++++++++++++++
> > 4 files changed, 23 insertions(+), 54 deletions(-)
> >
> > diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> > index 4a35e7c..bd484f0 100644
> > --- a/fs/lockd/clntproc.c
> > +++ b/fs/lockd/clntproc.c
> > @@ -475,22 +475,6 @@ static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *ho
> > fl->fl_ops = &nlmclnt_lock_ops;
> > }
> >
> > -static int do_vfs_lock(struct file_lock *fl)
> > -{
> > - int res = 0;
> > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > - case FL_POSIX:
> > - res = posix_lock_file_wait(fl->fl_file, fl);
> > - break;
> > - case FL_FLOCK:
> > - res = flock_lock_file_wait(fl->fl_file, fl);
> > - break;
> > - default:
> > - BUG();
> > - }
> > - return res;
> > -}
> > -
> > /*
> > * LOCK: Try to create a lock
> > *
> > @@ -518,6 +502,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > struct nlm_host *host = req->a_host;
> > struct nlm_res *resp = &req->a_res;
> > struct nlm_wait *block = NULL;
> > + struct inode *inode = file_inode(fl->fl_file);
> > unsigned char fl_flags = fl->fl_flags;
> > unsigned char fl_type;
> > int status = -ENOLCK;
> > @@ -527,7 +512,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > req->a_args.state = nsm_local_state;
> >
> > fl->fl_flags |= FL_ACCESS;
> > - status = do_vfs_lock(fl);
> > + status = do_vfs_lock(inode, fl);
> > fl->fl_flags = fl_flags;
> > if (status < 0)
> > goto out;
> > @@ -577,7 +562,7 @@ again:
> > }
> > /* Ensure the resulting lock will get added to granted list */
> > fl->fl_flags |= FL_SLEEP;
> > - if (do_vfs_lock(fl) < 0)
> > + if (do_vfs_lock(inode, fl) < 0)
> > printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
> > up_read(&host->h_rwsem);
> > fl->fl_flags = fl_flags;
> > @@ -607,7 +592,7 @@ out_unlock:
> > fl_type = fl->fl_type;
> > fl->fl_type = F_UNLCK;
> > down_read(&host->h_rwsem);
> > - do_vfs_lock(fl);
> > + do_vfs_lock(inode, fl);
> > up_read(&host->h_rwsem);
> > fl->fl_type = fl_type;
> > fl->fl_flags = fl_flags;
> > @@ -674,7 +659,7 @@ nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_l
> > */
> > fl->fl_flags |= FL_EXISTS;
> > down_read(&host->h_rwsem);
> > - status = do_vfs_lock(fl);
> > + status = do_vfs_lock(d_inode(ctx->dentry), fl);
> > up_read(&host->h_rwsem);
> > fl->fl_flags = fl_flags;
> > if (status == -ENOENT) {
> > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > index 319847c..d16c50f 100644
> > --- a/fs/nfs/file.c
> > +++ b/fs/nfs/file.c
> > @@ -737,22 +737,6 @@ out_noconflict:
> > goto out;
> > }
> >
> > -static int do_vfs_lock(struct file *file, struct file_lock *fl)
> > -{
> > - int res = 0;
> > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > - case FL_POSIX:
> > - res = posix_lock_file_wait(file, fl);
> > - break;
> > - case FL_FLOCK:
> > - res = flock_lock_file_wait(file, fl);
> > - break;
> > - default:
> > - BUG();
> > - }
> > - return res;
> > -}
> > -
> > static int
> > do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > {
> > @@ -786,7 +770,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > if (!is_local)
> > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > else
> > - status = do_vfs_lock(filp, fl);
> > + status = do_vfs_lock(inode, fl);
> > return status;
> > }
> >
> > @@ -817,7 +801,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > if (!is_local)
> > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > else
> > - status = do_vfs_lock(filp, fl);
> > + status = do_vfs_lock(inode, fl);
> > if (status < 0)
> > goto out;
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index c08f8ac..6b19307 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5511,22 +5511,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
> > return err;
> > }
> >
> > -static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > -{
> > - int res = 0;
> > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > - case FL_POSIX:
> > - res = posix_lock_inode_wait(inode, fl);
> > - break;
> > - case FL_FLOCK:
> > - res = flock_lock_inode_wait(inode, fl);
> > - break;
> > - default:
> > - BUG();
> > - }
> > - return res;
> > -}
> > -
> > struct nfs4_unlockdata {
> > struct nfs_locku_args arg;
> > struct nfs_locku_res res;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 72d8a84..165577a 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1225,6 +1225,22 @@ static inline int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
> > return flock_lock_inode_wait(file_inode(filp), fl);
> > }
> >
> > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > +{
> > + int res = 0;
> > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > + case FL_POSIX:
> > + res = posix_lock_inode_wait(inode, fl);
> > + break;
> > + case FL_FLOCK:
> > + res = flock_lock_inode_wait(inode, fl);
> > + break;
> > + default:
> > + BUG();
> > + }
> > + return res;
> > +}
> > +
> > struct fasync_struct {
> > spinlock_t fa_lock;
> > int magic;
>
> Meh ok...a little large for an inline though. Maybe you should move
> that into fs/nfs_common and have it as an exported symbol? I'm not too
> religious about it though since we don't have many call sites.
Is this ready to merge, or should we hold off pending an update?
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-21 21:48 ` Trond Myklebust
@ 2015-10-21 23:49 ` Jeff Layton
2015-10-22 0:11 ` Benjamin Coddington
2015-10-22 8:34 ` Christoph Hellwig
2 siblings, 0 replies; 20+ messages in thread
From: Jeff Layton @ 2015-10-21 23:49 UTC (permalink / raw)
To: Trond Myklebust
Cc: Benjamin Coddington, Linux NFS Mailing List, Anna Schumaker
On Wed, 21 Oct 2015 14:48:45 -0700
Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> On Wed, Oct 14, 2015 at 12:55 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> >
> > On Wed, 14 Oct 2015 14:23:35 -0400
> > Benjamin Coddington <bcodding@redhat.com> wrote:
> >
> > > Send the inode instead of the file_lock to do_vfs_lock() in
> > > fs/lockd/clintproc.c (because there may not be a fl_file the unlock path).
> > > Now that we have posix_lock_inode_wait() the three copies of do_vfs_lock()
> > > can be collapsed.
> > >
> > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > ---
> > > fs/lockd/clntproc.c | 25 +++++--------------------
> > > fs/nfs/file.c | 20 ++------------------
> > > fs/nfs/nfs4proc.c | 16 ----------------
> > > include/linux/fs.h | 16 ++++++++++++++++
> > > 4 files changed, 23 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> > > index 4a35e7c..bd484f0 100644
> > > --- a/fs/lockd/clntproc.c
> > > +++ b/fs/lockd/clntproc.c
> > > @@ -475,22 +475,6 @@ static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *ho
> > > fl->fl_ops = &nlmclnt_lock_ops;
> > > }
> > >
> > > -static int do_vfs_lock(struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_file_wait(fl->fl_file, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_file_wait(fl->fl_file, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > /*
> > > * LOCK: Try to create a lock
> > > *
> > > @@ -518,6 +502,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > > struct nlm_host *host = req->a_host;
> > > struct nlm_res *resp = &req->a_res;
> > > struct nlm_wait *block = NULL;
> > > + struct inode *inode = file_inode(fl->fl_file);
> > > unsigned char fl_flags = fl->fl_flags;
> > > unsigned char fl_type;
> > > int status = -ENOLCK;
> > > @@ -527,7 +512,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > > req->a_args.state = nsm_local_state;
> > >
> > > fl->fl_flags |= FL_ACCESS;
> > > - status = do_vfs_lock(fl);
> > > + status = do_vfs_lock(inode, fl);
> > > fl->fl_flags = fl_flags;
> > > if (status < 0)
> > > goto out;
> > > @@ -577,7 +562,7 @@ again:
> > > }
> > > /* Ensure the resulting lock will get added to granted list */
> > > fl->fl_flags |= FL_SLEEP;
> > > - if (do_vfs_lock(fl) < 0)
> > > + if (do_vfs_lock(inode, fl) < 0)
> > > printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
> > > up_read(&host->h_rwsem);
> > > fl->fl_flags = fl_flags;
> > > @@ -607,7 +592,7 @@ out_unlock:
> > > fl_type = fl->fl_type;
> > > fl->fl_type = F_UNLCK;
> > > down_read(&host->h_rwsem);
> > > - do_vfs_lock(fl);
> > > + do_vfs_lock(inode, fl);
> > > up_read(&host->h_rwsem);
> > > fl->fl_type = fl_type;
> > > fl->fl_flags = fl_flags;
> > > @@ -674,7 +659,7 @@ nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_l
> > > */
> > > fl->fl_flags |= FL_EXISTS;
> > > down_read(&host->h_rwsem);
> > > - status = do_vfs_lock(fl);
> > > + status = do_vfs_lock(d_inode(ctx->dentry), fl);
> > > up_read(&host->h_rwsem);
> > > fl->fl_flags = fl_flags;
> > > if (status == -ENOENT) {
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index 319847c..d16c50f 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -737,22 +737,6 @@ out_noconflict:
> > > goto out;
> > > }
> > >
> > > -static int do_vfs_lock(struct file *file, struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_file_wait(file, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_file_wait(file, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > static int
> > > do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > {
> > > @@ -786,7 +770,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > if (!is_local)
> > > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > > else
> > > - status = do_vfs_lock(filp, fl);
> > > + status = do_vfs_lock(inode, fl);
> > > return status;
> > > }
> > >
> > > @@ -817,7 +801,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > if (!is_local)
> > > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > > else
> > > - status = do_vfs_lock(filp, fl);
> > > + status = do_vfs_lock(inode, fl);
> > > if (status < 0)
> > > goto out;
> > >
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index c08f8ac..6b19307 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -5511,22 +5511,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
> > > return err;
> > > }
> > >
> > > -static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_inode_wait(inode, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_inode_wait(inode, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > struct nfs4_unlockdata {
> > > struct nfs_locku_args arg;
> > > struct nfs_locku_res res;
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index 72d8a84..165577a 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1225,6 +1225,22 @@ static inline int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
> > > return flock_lock_inode_wait(file_inode(filp), fl);
> > > }
> > >
> > > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > +{
> > > + int res = 0;
> > > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > + case FL_POSIX:
> > > + res = posix_lock_inode_wait(inode, fl);
> > > + break;
> > > + case FL_FLOCK:
> > > + res = flock_lock_inode_wait(inode, fl);
> > > + break;
> > > + default:
> > > + BUG();
> > > + }
> > > + return res;
> > > +}
> > > +
> > > struct fasync_struct {
> > > spinlock_t fa_lock;
> > > int magic;
> >
> > Meh ok...a little large for an inline though. Maybe you should move
> > that into fs/nfs_common and have it as an exported symbol? I'm not too
> > religious about it though since we don't have many call sites.
>
> Is this ready to merge, or should we hold off pending an update?
This one is really fine IMO, but I do have a bit of concern with the
last patch in the series. That one adds an allocation to the unlock
codepath. If that fails then a dangling lock could be left hanging
around on the server. I think it's possible to do this without
allocating anything there, so I'd like to see that changed.
Otherwise, the set looks good to me. Nice work!
--
Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-21 21:48 ` Trond Myklebust
2015-10-21 23:49 ` Jeff Layton
@ 2015-10-22 0:11 ` Benjamin Coddington
2015-10-22 8:34 ` Christoph Hellwig
2 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-22 0:11 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Jeff Layton, Linux NFS Mailing List, Anna Schumaker
On Wed, 21 Oct 2015, Trond Myklebust wrote:
> On Wed, Oct 14, 2015 at 12:55 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> >
> > On Wed, 14 Oct 2015 14:23:35 -0400
> > Benjamin Coddington <bcodding@redhat.com> wrote:
> >
> > > Send the inode instead of the file_lock to do_vfs_lock() in
> > > fs/lockd/clintproc.c (because there may not be a fl_file the unlock path).
> > > Now that we have posix_lock_inode_wait() the three copies of do_vfs_lock()
> > > can be collapsed.
> > >
> > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > ---
> > > fs/lockd/clntproc.c | 25 +++++--------------------
> > > fs/nfs/file.c | 20 ++------------------
> > > fs/nfs/nfs4proc.c | 16 ----------------
> > > include/linux/fs.h | 16 ++++++++++++++++
> > > 4 files changed, 23 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> > > index 4a35e7c..bd484f0 100644
> > > --- a/fs/lockd/clntproc.c
> > > +++ b/fs/lockd/clntproc.c
> > > @@ -475,22 +475,6 @@ static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *ho
> > > fl->fl_ops = &nlmclnt_lock_ops;
> > > }
> > >
> > > -static int do_vfs_lock(struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_file_wait(fl->fl_file, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_file_wait(fl->fl_file, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > /*
> > > * LOCK: Try to create a lock
> > > *
> > > @@ -518,6 +502,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > > struct nlm_host *host = req->a_host;
> > > struct nlm_res *resp = &req->a_res;
> > > struct nlm_wait *block = NULL;
> > > + struct inode *inode = file_inode(fl->fl_file);
> > > unsigned char fl_flags = fl->fl_flags;
> > > unsigned char fl_type;
> > > int status = -ENOLCK;
> > > @@ -527,7 +512,7 @@ nlmclnt_lock(struct nlm_rqst *req, struct file_lock *fl)
> > > req->a_args.state = nsm_local_state;
> > >
> > > fl->fl_flags |= FL_ACCESS;
> > > - status = do_vfs_lock(fl);
> > > + status = do_vfs_lock(inode, fl);
> > > fl->fl_flags = fl_flags;
> > > if (status < 0)
> > > goto out;
> > > @@ -577,7 +562,7 @@ again:
> > > }
> > > /* Ensure the resulting lock will get added to granted list */
> > > fl->fl_flags |= FL_SLEEP;
> > > - if (do_vfs_lock(fl) < 0)
> > > + if (do_vfs_lock(inode, fl) < 0)
> > > printk(KERN_WARNING "%s: VFS is out of sync with lock manager!\n", __func__);
> > > up_read(&host->h_rwsem);
> > > fl->fl_flags = fl_flags;
> > > @@ -607,7 +592,7 @@ out_unlock:
> > > fl_type = fl->fl_type;
> > > fl->fl_type = F_UNLCK;
> > > down_read(&host->h_rwsem);
> > > - do_vfs_lock(fl);
> > > + do_vfs_lock(inode, fl);
> > > up_read(&host->h_rwsem);
> > > fl->fl_type = fl_type;
> > > fl->fl_flags = fl_flags;
> > > @@ -674,7 +659,7 @@ nlmclnt_unlock(struct nfs_open_context *ctx, struct nlm_rqst *req, struct file_l
> > > */
> > > fl->fl_flags |= FL_EXISTS;
> > > down_read(&host->h_rwsem);
> > > - status = do_vfs_lock(fl);
> > > + status = do_vfs_lock(d_inode(ctx->dentry), fl);
> > > up_read(&host->h_rwsem);
> > > fl->fl_flags = fl_flags;
> > > if (status == -ENOENT) {
> > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > > index 319847c..d16c50f 100644
> > > --- a/fs/nfs/file.c
> > > +++ b/fs/nfs/file.c
> > > @@ -737,22 +737,6 @@ out_noconflict:
> > > goto out;
> > > }
> > >
> > > -static int do_vfs_lock(struct file *file, struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_file_wait(file, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_file_wait(file, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > static int
> > > do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > {
> > > @@ -786,7 +770,7 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > if (!is_local)
> > > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > > else
> > > - status = do_vfs_lock(filp, fl);
> > > + status = do_vfs_lock(inode, fl);
> > > return status;
> > > }
> > >
> > > @@ -817,7 +801,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
> > > if (!is_local)
> > > status = NFS_PROTO(inode)->lock(ctx, cmd, fl);
> > > else
> > > - status = do_vfs_lock(filp, fl);
> > > + status = do_vfs_lock(inode, fl);
> > > if (status < 0)
> > > goto out;
> > >
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index c08f8ac..6b19307 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -5511,22 +5511,6 @@ static int nfs4_proc_getlk(struct nfs4_state *state, int cmd, struct file_lock *
> > > return err;
> > > }
> > >
> > > -static int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > -{
> > > - int res = 0;
> > > - switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > - case FL_POSIX:
> > > - res = posix_lock_inode_wait(inode, fl);
> > > - break;
> > > - case FL_FLOCK:
> > > - res = flock_lock_inode_wait(inode, fl);
> > > - break;
> > > - default:
> > > - BUG();
> > > - }
> > > - return res;
> > > -}
> > > -
> > > struct nfs4_unlockdata {
> > > struct nfs_locku_args arg;
> > > struct nfs_locku_res res;
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index 72d8a84..165577a 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1225,6 +1225,22 @@ static inline int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
> > > return flock_lock_inode_wait(file_inode(filp), fl);
> > > }
> > >
> > > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > +{
> > > + int res = 0;
> > > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > + case FL_POSIX:
> > > + res = posix_lock_inode_wait(inode, fl);
> > > + break;
> > > + case FL_FLOCK:
> > > + res = flock_lock_inode_wait(inode, fl);
> > > + break;
> > > + default:
> > > + BUG();
> > > + }
> > > + return res;
> > > +}
> > > +
> > > struct fasync_struct {
> > > spinlock_t fa_lock;
> > > int magic;
> >
> > Meh ok...a little large for an inline though. Maybe you should move
> > that into fs/nfs_common and have it as an exported symbol? I'm not too
> > religious about it though since we don't have many call sites.
>
> Is this ready to merge, or should we hold off pending an update?
I'll send another update. I need to consider Jeff's comments on patch 10
and see if we can get rid of that kmalloc.
Ben
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-21 21:48 ` Trond Myklebust
2015-10-21 23:49 ` Jeff Layton
2015-10-22 0:11 ` Benjamin Coddington
@ 2015-10-22 8:34 ` Christoph Hellwig
2015-10-22 15:50 ` Benjamin Coddington
2 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2015-10-22 8:34 UTC (permalink / raw)
To: Trond Myklebust
Cc: Jeff Layton, Benjamin Coddington, Linux NFS Mailing List,
Anna Schumaker
On Wed, Oct 21, 2015 at 02:48:45PM -0700, Trond Myklebust wrote:
> > > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > +{
> > > + int res = 0;
> > > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > + case FL_POSIX:
> > > + res = posix_lock_inode_wait(inode, fl);
> > > + break;
> > > + case FL_FLOCK:
> > > + res = flock_lock_inode_wait(inode, fl);
> > > + break;
> > > + default:
> > > + BUG();
> > > + }
> > > + return res;
> > > +}
This is a) not a good name for a global function, and b) probably
shouldn't be inline.
Given how similar the functions are I'd rather have a
file_lock_inode_wait that handles both cases right in fs/locks.c
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 08/10] NFS: Move do_vfs_lock to shared inline
2015-10-22 8:34 ` Christoph Hellwig
@ 2015-10-22 15:50 ` Benjamin Coddington
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-22 15:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Trond Myklebust, Jeff Layton, Linux NFS Mailing List,
Anna Schumaker
On Thu, 22 Oct 2015, Christoph Hellwig wrote:
> On Wed, Oct 21, 2015 at 02:48:45PM -0700, Trond Myklebust wrote:
> > > > +static inline int do_vfs_lock(struct inode *inode, struct file_lock *fl)
> > > > +{
> > > > + int res = 0;
> > > > + switch (fl->fl_flags & (FL_POSIX|FL_FLOCK)) {
> > > > + case FL_POSIX:
> > > > + res = posix_lock_inode_wait(inode, fl);
> > > > + break;
> > > > + case FL_FLOCK:
> > > > + res = flock_lock_inode_wait(inode, fl);
> > > > + break;
> > > > + default:
> > > > + BUG();
> > > > + }
> > > > + return res;
> > > > +}
>
> This is a) not a good name for a global function, and b) probably
> shouldn't be inline.
>
> Given how similar the functions are I'd rather have a
> file_lock_inode_wait that handles both cases right in fs/locks.c
Makes sense to me.. I'll do it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 09/10] locks: Use more file_inode and fix a comment
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (7 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 08/10] NFS: Move do_vfs_lock to shared inline Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 19:56 ` Jeff Layton
2015-10-14 18:23 ` [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE Benjamin Coddington
9 siblings, 1 reply; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/locks.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 2a54c80..8efd9f8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1711,8 +1711,7 @@ static int generic_delete_lease(struct file *filp, void *owner)
{
int error = -EAGAIN;
struct file_lock *fl, *victim = NULL;
- struct dentry *dentry = filp->f_path.dentry;
- struct inode *inode = dentry->d_inode;
+ struct inode *inode = file_inode(filp);
struct file_lock_context *ctx = inode->i_flctx;
LIST_HEAD(dispose);
@@ -1751,8 +1750,7 @@ static int generic_delete_lease(struct file *filp, void *owner)
int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
void **priv)
{
- struct dentry *dentry = filp->f_path.dentry;
- struct inode *inode = dentry->d_inode;
+ struct inode *inode = file_inode(filp);
int error;
if ((!uid_eq(current_fsuid(), inode->i_uid)) && !capable(CAP_LEASE))
@@ -2107,7 +2105,7 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd,
return error;
}
-/* Ensure that fl->fl_filp has compatible f_mode for F_SETLK calls */
+/* Ensure that fl->fl_file has compatible f_mode for F_SETLK calls */
static int
check_fmode_for_setlk(struct file_lock *fl)
{
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 09/10] locks: Use more file_inode and fix a comment
2015-10-14 18:23 ` [PATCH 09/10] locks: Use more file_inode and fix a comment Benjamin Coddington
@ 2015-10-14 19:56 ` Jeff Layton
0 siblings, 0 replies; 20+ messages in thread
From: Jeff Layton @ 2015-10-14 19:56 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, trond.myklebust, anna.schumaker
On Wed, 14 Oct 2015 14:23:36 -0400
Benjamin Coddington <bcodding@redhat.com> wrote:
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/locks.c | 8 +++-----
> 1 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 2a54c80..8efd9f8 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1711,8 +1711,7 @@ static int generic_delete_lease(struct file *filp, void *owner)
> {
> int error = -EAGAIN;
> struct file_lock *fl, *victim = NULL;
> - struct dentry *dentry = filp->f_path.dentry;
> - struct inode *inode = dentry->d_inode;
> + struct inode *inode = file_inode(filp);
> struct file_lock_context *ctx = inode->i_flctx;
> LIST_HEAD(dispose);
>
> @@ -1751,8 +1750,7 @@ static int generic_delete_lease(struct file *filp, void *owner)
> int generic_setlease(struct file *filp, long arg, struct file_lock **flp,
> void **priv)
> {
> - struct dentry *dentry = filp->f_path.dentry;
> - struct inode *inode = dentry->d_inode;
> + struct inode *inode = file_inode(filp);
> int error;
>
> if ((!uid_eq(current_fsuid(), inode->i_uid)) && !capable(CAP_LEASE))
> @@ -2107,7 +2105,7 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd,
> return error;
> }
>
> -/* Ensure that fl->fl_filp has compatible f_mode for F_SETLK calls */
> +/* Ensure that fl->fl_file has compatible f_mode for F_SETLK calls */
> static int
> check_fmode_for_setlk(struct file_lock *fl)
> {
Thanks -- I'll merge this for v4.4.
--
Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE
2015-10-14 18:23 [PATCH 00/10] locking fixups for NFS Benjamin Coddington
` (8 preceding siblings ...)
2015-10-14 18:23 ` [PATCH 09/10] locks: Use more file_inode and fix a comment Benjamin Coddington
@ 2015-10-14 18:23 ` Benjamin Coddington
2015-10-14 20:30 ` Jeff Layton
9 siblings, 1 reply; 20+ messages in thread
From: Benjamin Coddington @ 2015-10-14 18:23 UTC (permalink / raw)
To: linux-nfs; +Cc: jlayton, trond.myklebust, anna.schumaker
NFS unlock procedures will wait for IO to complete before sending an unlock.
In the case that this wait is interrupted, an unlock may never be sent if
the unlock is part of cleaning up locks during a close. This lost lock can
then prevent other clients from locking the file.
Fix this by deferring an unlock that should wait for IO during FL_CLOSE by
copying it to a list on the nfs_lock_context, which can then be used to
release the lock when the IO has completed.
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
fs/nfs/file.c | 36 +++++++++++++++++++++++++++++++++++-
fs/nfs/inode.c | 1 +
fs/nfs/pagelist.c | 23 ++++++++++++++++++++---
include/linux/nfs_fs.h | 7 +++++++
4 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index d16c50f..460311a 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -738,6 +738,36 @@ out_noconflict:
}
static int
+defer_unlk(struct nfs_lock_context *l_ctx, int cmd, struct file_lock *fl)
+{
+ struct inode *inode = d_inode(l_ctx->open_context->dentry);
+ struct nfs_io_counter *c = &l_ctx->io_count;
+ struct nfs_deferred_unlock *dunlk;
+ int status = 0;
+
+ if (atomic_read(&c->io_count) == 0)
+ return 0;
+
+ /* free in nfs_iocounter_dec */
+ dunlk = kmalloc(sizeof(*dunlk), GFP_NOFS);
+ if (dunlk == NULL)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&dunlk->list);
+ dunlk->cmd = cmd;
+ memcpy(&dunlk->fl, fl, sizeof(dunlk->fl));
+ spin_lock(&inode->i_lock);
+ if (atomic_read(&c->io_count) != 0) {
+ list_add_tail(&dunlk->list, &l_ctx->dunlk_list);
+ status = -EINPROGRESS;
+ } else {
+ kfree(dunlk);
+ }
+ spin_unlock(&inode->i_lock);
+ return status;
+}
+
+static int
do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
{
struct inode *inode = filp->f_mapping->host;
@@ -753,7 +783,11 @@ do_unlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
if (!IS_ERR(l_ctx)) {
- status = nfs_iocounter_wait(&l_ctx->io_count);
+ if (fl->fl_flags & FL_CLOSE)
+ status = defer_unlk(l_ctx, cmd, fl);
+ else
+ status = nfs_iocounter_wait(&l_ctx->io_count);
+
nfs_put_lock_context(l_ctx);
if (status < 0)
return status;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 326d9e1..af4f846 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -696,6 +696,7 @@ static void nfs_init_lock_context(struct nfs_lock_context *l_ctx)
l_ctx->lockowner.l_owner = current->files;
l_ctx->lockowner.l_pid = current->tgid;
INIT_LIST_HEAD(&l_ctx->list);
+ INIT_LIST_HEAD(&l_ctx->dunlk_list);
nfs_iocounter_init(&l_ctx->io_count);
}
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index fe3ddd2..17dd6c0 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -108,9 +108,26 @@ nfs_iocounter_inc(struct nfs_io_counter *c)
}
static void
-nfs_iocounter_dec(struct nfs_io_counter *c)
+nfs_iocounter_dec(struct nfs_lock_context *l_ctx)
{
- if (atomic_dec_and_test(&c->io_count)) {
+ struct nfs_io_counter *c = &l_ctx->io_count;
+ struct inode *inode = d_inode(l_ctx->open_context->dentry);
+
+ if (atomic_dec_and_lock(&c->io_count, &inode->i_lock)) {
+ if (unlikely(!list_empty(&l_ctx->dunlk_list))) {
+ struct nfs_deferred_unlock *dunlk, *tmp;
+ LIST_HEAD(dunlk_list);
+ list_replace_init(&l_ctx->dunlk_list, &dunlk_list);
+ spin_unlock(&inode->i_lock);
+
+ list_for_each_entry_safe(dunlk, tmp, &dunlk_list, list) {
+ NFS_PROTO(inode)->lock(l_ctx->open_context, dunlk->cmd, &dunlk->fl);
+ locks_release_private(&dunlk->fl);
+ kfree(dunlk);
+ }
+ } else {
+ spin_unlock(&inode->i_lock);
+ }
clear_bit(NFS_IO_INPROGRESS, &c->flags);
smp_mb__after_atomic();
wake_up_bit(&c->flags, NFS_IO_INPROGRESS);
@@ -431,7 +448,7 @@ static void nfs_clear_request(struct nfs_page *req)
req->wb_page = NULL;
}
if (l_ctx != NULL) {
- nfs_iocounter_dec(&l_ctx->io_count);
+ nfs_iocounter_dec(l_ctx);
nfs_put_lock_context(l_ctx);
req->wb_lock_context = NULL;
}
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index c0e9614..ba36498 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -66,12 +66,19 @@ struct nfs_io_counter {
atomic_t io_count;
};
+struct nfs_deferred_unlock {
+ struct list_head list;
+ int cmd;
+ struct file_lock fl;
+};
+
struct nfs_lock_context {
atomic_t count;
struct list_head list;
struct nfs_open_context *open_context;
struct nfs_lockowner lockowner;
struct nfs_io_counter io_count;
+ struct list_head dunlk_list;
};
struct nfs4_state;
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE
2015-10-14 18:23 ` [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE Benjamin Coddington
@ 2015-10-14 20:30 ` Jeff Layton
2015-12-07 16:05 ` Benjamin Coddington
0 siblings, 1 reply; 20+ messages in thread
From: Jeff Layton @ 2015-10-14 20:30 UTC (permalink / raw)
To: Benjamin Coddington; +Cc: linux-nfs, trond.myklebust, anna.schumaker
On Wed, 14 Oct 2015 14:23:37 -0400
Benjamin Coddington <bcodding@redhat.com> wrote:
> NFS unlock procedures will wait for IO to complete before sending an unlock.
> In the case that this wait is interrupted, an unlock may never be sent if
> the unlock is part of cleaning up locks during a close. This lost lock can
> then prevent other clients from locking the file.
>
> Fix this by deferring an unlock that should wait for IO during FL_CLOSE by
> copying it to a list on the nfs_lock_context, which can then be used to
> release the lock when the IO has completed.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
> fs/nfs/file.c | 36 +++++++++++++++++++++++++++++++++++-
> fs/nfs/inode.c | 1 +
> fs/nfs/pagelist.c | 23 ++++++++++++++++++++---
> include/linux/nfs_fs.h | 7 +++++++
> 4 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index d16c50f..460311a 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -738,6 +738,36 @@ out_noconflict:
> }
>
> static int
> +defer_unlk(struct nfs_lock_context *l_ctx, int cmd, struct file_lock *fl)
> +{
> + struct inode *inode = d_inode(l_ctx->open_context->dentry);
> + struct nfs_io_counter *c = &l_ctx->io_count;
> + struct nfs_deferred_unlock *dunlk;
> + int status = 0;
> +
> + if (atomic_read(&c->io_count) == 0)
> + return 0;
> +
> + /* free in nfs_iocounter_dec */
> + dunlk = kmalloc(sizeof(*dunlk), GFP_NOFS);
> + if (dunlk == NULL)
> + return -ENOMEM;
> +
This is a little ugly...
You're probably going to calling this from something like
locks_remove_posix, and if this allocation fails then the unlock will
just never happen.
Is there any way to avoid this allocation?
The "cmd" field in nfs_deferred_unlock is more or less redundant. We're
always calling this with that set to F_UNLCK. We also know that this
will be called on the whole file range. Maybe we can simply add a flag
to the lock context to indicate whether we should send a whole-file
unlock on it when the io_count goes to zero.
Also, on a somewhat related note...we aren't currently setting FL_CLOSE
in locks_remove_flock and we probably should be.
> + INIT_LIST_HEAD(&dunlk->list);
> + dunlk->cmd = cmd;
> + memcpy(&dunlk->fl, fl, sizeof(dunlk->fl));
> + spin_lock(&inode->i_lock);
> + if (atomic_read(&c->io_count) != 0) {
> + list_add_tail(&dunlk->list, &l_ctx->dunlk_list);
> + status = -EINPROGRESS;
> + } else {
> + kfree(dunlk);
> + }
> + spin_unlock(&inode->i_lock);
> + return status;
> +}
> +
> +static int
> do_unlk(struct file *filp, int cmd, struct file_lock *fl, int
> is_local) {
> struct inode *inode = filp->f_mapping->host;
> @@ -753,7 +783,11 @@ do_unlk(struct file *filp, int cmd, struct
> file_lock *fl, int is_local)
> l_ctx = nfs_get_lock_context(nfs_file_open_context(filp));
> if (!IS_ERR(l_ctx)) {
> - status = nfs_iocounter_wait(&l_ctx->io_count);
> + if (fl->fl_flags & FL_CLOSE)
> + status = defer_unlk(l_ctx, cmd, fl);
> + else
> + status =
> nfs_iocounter_wait(&l_ctx->io_count); +
> nfs_put_lock_context(l_ctx);
> if (status < 0)
> return status;
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 326d9e1..af4f846 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -696,6 +696,7 @@ static void nfs_init_lock_context(struct
> nfs_lock_context *l_ctx) l_ctx->lockowner.l_owner = current->files;
> l_ctx->lockowner.l_pid = current->tgid;
> INIT_LIST_HEAD(&l_ctx->list);
> + INIT_LIST_HEAD(&l_ctx->dunlk_list);
> nfs_iocounter_init(&l_ctx->io_count);
> }
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index fe3ddd2..17dd6c0 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -108,9 +108,26 @@ nfs_iocounter_inc(struct nfs_io_counter *c)
> }
>
> static void
> -nfs_iocounter_dec(struct nfs_io_counter *c)
> +nfs_iocounter_dec(struct nfs_lock_context *l_ctx)
> {
> - if (atomic_dec_and_test(&c->io_count)) {
> + struct nfs_io_counter *c = &l_ctx->io_count;
> + struct inode *inode = d_inode(l_ctx->open_context->dentry);
> +
> + if (atomic_dec_and_lock(&c->io_count, &inode->i_lock)) {
> + if (unlikely(!list_empty(&l_ctx->dunlk_list))) {
> + struct nfs_deferred_unlock *dunlk, *tmp;
> + LIST_HEAD(dunlk_list);
> + list_replace_init(&l_ctx->dunlk_list,
> &dunlk_list);
> + spin_unlock(&inode->i_lock);
> +
> + list_for_each_entry_safe(dunlk, tmp,
> &dunlk_list, list) {
> +
> NFS_PROTO(inode)->lock(l_ctx->open_context, dunlk->cmd, &dunlk->fl);
> + locks_release_private(&dunlk->fl);
> + kfree(dunlk);
> + }
> + } else {
> + spin_unlock(&inode->i_lock);
> + }
> clear_bit(NFS_IO_INPROGRESS, &c->flags);
> smp_mb__after_atomic();
> wake_up_bit(&c->flags, NFS_IO_INPROGRESS);
> @@ -431,7 +448,7 @@ static void nfs_clear_request(struct nfs_page
> *req) req->wb_page = NULL;
> }
> if (l_ctx != NULL) {
> - nfs_iocounter_dec(&l_ctx->io_count);
> + nfs_iocounter_dec(l_ctx);
> nfs_put_lock_context(l_ctx);
> req->wb_lock_context = NULL;
> }
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index c0e9614..ba36498 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -66,12 +66,19 @@ struct nfs_io_counter {
> atomic_t io_count;
> };
>
> +struct nfs_deferred_unlock {
> + struct list_head list;
> + int cmd;
> + struct file_lock fl;
> +};
> +
> struct nfs_lock_context {
> atomic_t count;
> struct list_head list;
> struct nfs_open_context *open_context;
> struct nfs_lockowner lockowner;
> struct nfs_io_counter io_count;
> + struct list_head dunlk_list;
> };
>
> struct nfs4_state;
--
Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 10/10] NFS: Deferred unlocks - always unlock on FL_CLOSE
2015-10-14 20:30 ` Jeff Layton
@ 2015-12-07 16:05 ` Benjamin Coddington
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Coddington @ 2015-12-07 16:05 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-nfs, trond.myklebust, anna.schumaker
On Wed, 14 Oct 2015, Jeff Layton wrote:
> On Wed, 14 Oct 2015 14:23:37 -0400
> Benjamin Coddington <bcodding@redhat.com> wrote:
>
> > NFS unlock procedures will wait for IO to complete before sending an unlock.
> > In the case that this wait is interrupted, an unlock may never be sent if
> > the unlock is part of cleaning up locks during a close. This lost lock can
> > then prevent other clients from locking the file.
> >
> > Fix this by deferring an unlock that should wait for IO during FL_CLOSE by
> > copying it to a list on the nfs_lock_context, which can then be used to
> > release the lock when the IO has completed.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> > fs/nfs/file.c | 36 +++++++++++++++++++++++++++++++++++-
> > fs/nfs/inode.c | 1 +
> > fs/nfs/pagelist.c | 23 ++++++++++++++++++++---
> > include/linux/nfs_fs.h | 7 +++++++
> > 4 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> > index d16c50f..460311a 100644
> > --- a/fs/nfs/file.c
> > +++ b/fs/nfs/file.c
> > @@ -738,6 +738,36 @@ out_noconflict:
> > }
> >
> > static int
> > +defer_unlk(struct nfs_lock_context *l_ctx, int cmd, struct file_lock *fl)
> > +{
> > + struct inode *inode = d_inode(l_ctx->open_context->dentry);
> > + struct nfs_io_counter *c = &l_ctx->io_count;
> > + struct nfs_deferred_unlock *dunlk;
> > + int status = 0;
> > +
> > + if (atomic_read(&c->io_count) == 0)
> > + return 0;
> > +
> > + /* free in nfs_iocounter_dec */
> > + dunlk = kmalloc(sizeof(*dunlk), GFP_NOFS);
> > + if (dunlk == NULL)
> > + return -ENOMEM;
> > +
>
> This is a little ugly...
> You're probably going to calling this from something like
> locks_remove_posix, and if this allocation fails then the unlock will
> just never happen.
>
> Is there any way to avoid this allocation?
Yes! As you go on to suggest..
> The "cmd" field in nfs_deferred_unlock is more or less redundant. We're
> always calling this with that set to F_UNLCK. We also know that this
> will be called on the whole file range. Maybe we can simply add a flag
> to the lock context to indicate whether we should send a whole-file
> unlock on it when the io_count goes to zero.
That simplifies things quite a bit.. I'm going to resubmit this with that
approach. Thanks!
> Also, on a somewhat related note...we aren't currently setting FL_CLOSE
> in locks_remove_flock and we probably should be.
I'll add that as well.
Ben
^ permalink raw reply [flat|nested] 20+ messages in thread