* [PATCH 2/3 v2] fs/locks.c: New ops in file_lock_operations for copying/releasing owner
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
@ 2014-08-10 15:42 ` Kinglong Mee
2014-08-10 15:43 ` [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (6 subsequent siblings)
7 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-10 15:42 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.
v2: A new patch isn't exist in v1.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 12 ++++++++++--
include/linux/fs.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 91b0f03..f302a51 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
fl->fl_ops->fl_release_private(fl);
fl->fl_ops = NULL;
}
- fl->fl_lmops = NULL;
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_release_owner)
+ fl->fl_lmops->lm_release_owner(fl);
+ fl->fl_lmops = NULL;
+ }
}
EXPORT_SYMBOL_GPL(locks_release_private);
@@ -262,8 +266,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
fl->fl_ops->fl_copy_lock(new, fl);
new->fl_ops = fl->fl_ops;
}
- if (fl->fl_lmops)
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_copy_owner)
+ fl->fl_lmops->lm_copy_owner(new, fl);
new->fl_lmops = fl->fl_lmops;
+ }
}
void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ced023d..622efca 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -843,6 +843,8 @@ struct file_lock_operations {
struct lock_manager_operations {
int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
unsigned long (*lm_owner_key)(struct file_lock *);
+ void (*lm_copy_owner)(struct file_lock *, struct file_lock *);
+ void (*lm_release_owner)(struct file_lock *);
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, struct file_lock *, int);
void (*lm_break)(struct file_lock *);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
2014-08-10 15:42 ` [PATCH 2/3 v2] fs/locks.c: New ops in file_lock_operations for copying/releasing owner Kinglong Mee
@ 2014-08-10 15:43 ` Kinglong Mee
2014-08-11 16:46 ` Jeff Layton
2014-08-11 16:19 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Jeff Layton
` (5 subsequent siblings)
7 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-10 15:43 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v2: Fix bad using of struct file_lock_operations for handle the owner.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..24a8d91 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4867,9 +4867,33 @@ nfs4_transform_lock_offset(struct file_lock *lock)
lock->fl_end = OFFSET_MAX;
}
+static inline struct nfs4_lockowner *get_lockowner(struct nfs4_lockowner *lo)
+{
+ atomic_inc(&lo->lo_owner.so_count);
+ return lo;
+}
+
+static void nfsd4_fl_copy_owner(struct file_lock *dst, struct file_lock *src)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
+ dst->fl_owner = (fl_owner_t) get_lockowner(lo);
+}
+
+static void nfsd4_fl_release_owner(struct file_lock *fl)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
+
+ if (lo) {
+ nfs4_put_stateowner(&lo->lo_owner);
+ fl->fl_owner = NULL;
+ }
+}
+
/* Hack!: For now, we're defining this just so we can use a pointer to it
* as a unique cookie to identify our (NFSv4's) posix locks. */
static const struct lock_manager_operations nfsd_posix_mng_ops = {
+ .lm_copy_owner = nfsd4_fl_copy_owner,
+ .lm_release_owner = nfsd4_fl_release_owner,
};
static inline void
@@ -5238,7 +5262,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_openmode;
goto out;
}
- file_lock->fl_owner = (fl_owner_t)lock_sop;
+
+ file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
@@ -5405,6 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfs4_ol_stateid *stp;
struct file *filp = NULL;
struct file_lock *file_lock = NULL;
+ struct nfs4_lockowner *lock_sop = NULL;
__be32 status;
int err;
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -5426,6 +5452,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_lock_range;
goto put_stateid;
}
+
+ lock_sop = lockowner(stp->st_stateowner);
file_lock = locks_alloc_lock();
if (!file_lock) {
dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
@@ -5434,7 +5462,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
locks_init_lock(file_lock);
file_lock->fl_type = F_UNLCK;
- file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
+ file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-10 15:43 ` [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
@ 2014-08-11 16:46 ` Jeff Layton
2014-08-14 12:30 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Jeff Layton @ 2014-08-11 16:46 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Sun, 10 Aug 2014 23:43:57 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> v2: Fix bad using of struct file_lock_operations for handle the owner.
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e80a59..24a8d91 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4867,9 +4867,33 @@ nfs4_transform_lock_offset(struct file_lock *lock)
> lock->fl_end = OFFSET_MAX;
> }
>
> +static inline struct nfs4_lockowner *get_lockowner(struct nfs4_lockowner *lo)
> +{
> + atomic_inc(&lo->lo_owner.so_count);
> + return lo;
> +}
> +
The other places that increment the so_count on a lockowner should
probably be changed to use get_lockowner.
Or what may be better is to make a nfs4_get_stateowner call and change
all of the places that bump the so_count to use it. Then you could just
do something like this in the right spot below:
file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(lock_sop));
...
> +static void nfsd4_fl_copy_owner(struct file_lock *dst, struct file_lock *src)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> + dst->fl_owner = (fl_owner_t) get_lockowner(lo);
> +}
> +
> +static void nfsd4_fl_release_owner(struct file_lock *fl)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> +
> + if (lo) {
> + nfs4_put_stateowner(&lo->lo_owner);
> + fl->fl_owner = NULL;
> + }
> +}
> +
> /* Hack!: For now, we're defining this just so we can use a pointer to it
> * as a unique cookie to identify our (NFSv4's) posix locks. */
The above comment should be removed now.
> static const struct lock_manager_operations nfsd_posix_mng_ops = {
> + .lm_copy_owner = nfsd4_fl_copy_owner,
> + .lm_release_owner = nfsd4_fl_release_owner,
> };
>
> static inline void
> @@ -5238,7 +5262,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_openmode;
> goto out;
> }
> - file_lock->fl_owner = (fl_owner_t)lock_sop;
> +
> + file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> @@ -5405,6 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfs4_ol_stateid *stp;
> struct file *filp = NULL;
> struct file_lock *file_lock = NULL;
> + struct nfs4_lockowner *lock_sop = NULL;
> __be32 status;
> int err;
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> @@ -5426,6 +5452,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_lock_range;
> goto put_stateid;
> }
> +
> + lock_sop = lockowner(stp->st_stateowner);
> file_lock = locks_alloc_lock();
> if (!file_lock) {
> dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> @@ -5434,7 +5462,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> }
> locks_init_lock(file_lock);
> file_lock->fl_type = F_UNLCK;
> - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> + file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-11 16:46 ` Jeff Layton
@ 2014-08-14 12:30 ` Kinglong Mee
0 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-14 12:30 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
On 8/12/2014 00:46, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:43:57 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> v2: Fix bad using of struct file_lock_operations for handle the owner.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>> fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 2e80a59..24a8d91 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4867,9 +4867,33 @@ nfs4_transform_lock_offset(struct file_lock *lock)
>> lock->fl_end = OFFSET_MAX;
>> }
>>
>> +static inline struct nfs4_lockowner *get_lockowner(struct nfs4_lockowner *lo)
>> +{
>> + atomic_inc(&lo->lo_owner.so_count);
>> + return lo;
>> +}
>> +
>
> The other places that increment the so_count on a lockowner should
> probably be changed to use get_lockowner.
Got it, thanks.
>
> Or what may be better is to make a nfs4_get_stateowner call and change
> all of the places that bump the so_count to use it. Then you could just
> do something like this in the right spot below:
>
> file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(lock_sop));
Yes, nfs4_get_stateowner corresponds with nfs4_put_stateowner.
>> +static void nfsd4_fl_copy_owner(struct file_lock *dst, struct file_lock *src)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
>> + dst->fl_owner = (fl_owner_t) get_lockowner(lo);
>> +}
>> +
>> +static void nfsd4_fl_release_owner(struct file_lock *fl)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>> +
>> + if (lo) {
>> + nfs4_put_stateowner(&lo->lo_owner);
>> + fl->fl_owner = NULL;
>> + }
>> +}
>> +
>> /* Hack!: For now, we're defining this just so we can use a pointer to it
>> * as a unique cookie to identify our (NFSv4's) posix locks. */
>
> The above comment should be removed now.
OK, will remove it in next version.
thanks,
Kinglong Mee
>
>> static const struct lock_manager_operations nfsd_posix_mng_ops = {
>> + .lm_copy_owner = nfsd4_fl_copy_owner,
>> + .lm_release_owner = nfsd4_fl_release_owner,
>> };
>>
>> static inline void
>> @@ -5238,7 +5262,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = nfserr_openmode;
>> goto out;
>> }
>> - file_lock->fl_owner = (fl_owner_t)lock_sop;
>> +
>> + file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> @@ -5405,6 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> struct nfs4_ol_stateid *stp;
>> struct file *filp = NULL;
>> struct file_lock *file_lock = NULL;
>> + struct nfs4_lockowner *lock_sop = NULL;
>> __be32 status;
>> int err;
>> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> @@ -5426,6 +5452,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = nfserr_lock_range;
>> goto put_stateid;
>> }
>> +
>> + lock_sop = lockowner(stp->st_stateowner);
>> file_lock = locks_alloc_lock();
>> if (!file_lock) {
>> dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
>> @@ -5434,7 +5462,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> }
>> locks_init_lock(file_lock);
>> file_lock->fl_type = F_UNLCK;
>> - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
>> + file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>
>
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
2014-08-10 15:42 ` [PATCH 2/3 v2] fs/locks.c: New ops in file_lock_operations for copying/releasing owner Kinglong Mee
2014-08-10 15:43 ` [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
@ 2014-08-11 16:19 ` Jeff Layton
2014-08-11 16:25 ` Joe Perches
2014-08-14 12:26 ` Kinglong Mee
2014-08-15 0:02 ` [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock Kinglong Mee
` (4 subsequent siblings)
7 siblings, 2 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-11 16:19 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel, Joe Perches
On Sun, 10 Aug 2014 23:38:25 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
>
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>
> v2: Only change the order from 3/3 to 1/3 now.
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/lockd/svclock.c | 2 +-
> fs/locks.c | 25 ++++++-------------------
> include/linux/fs.h | 6 ------
> 3 files changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index ab798a8..e1f209c 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
> block->b_flags |= B_TIMED_OUT;
> if (conf) {
> if (block->b_fl)
> - __locks_copy_lock(block->b_fl, conf);
> + locks_copy_lock(block->b_fl, conf);
> }
> }
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 717fbc4..91b0f03 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> new->fl_lmops = fl->fl_lmops;
> }
>
> -/*
> - * Initialize a new lock from an existing file_lock structure.
> - */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> {
> + locks_release_private(new);
> +
> new->fl_owner = fl->fl_owner;
> new->fl_pid = fl->fl_pid;
> - new->fl_file = NULL;
> + new->fl_file = fl->fl_file;
> new->fl_flags = fl->fl_flags;
> new->fl_type = fl->fl_type;
> new->fl_start = fl->fl_start;
> new->fl_end = fl->fl_end;
> new->fl_ops = NULL;
> new->fl_lmops = NULL;
> -}
> -EXPORT_SYMBOL(__locks_copy_lock);
> -
> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> - locks_release_private(new);
> -
> - __locks_copy_lock(new, fl);
> - new->fl_file = fl->fl_file;
> - new->fl_ops = fl->fl_ops;
> - new->fl_lmops = fl->fl_lmops;
>
> locks_copy_private(new, fl);
> }
(cc'ing Joe Perches)
Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
is that you now need to ensure that any conflock structures are
properly initialized before passing them to locks_copy_lock.
The nfsv4 server code currently doesn't do that and it will need to be
fixed to do so or that will be a regression.
For the NLM code, Joe Perches has proposed a patch to remove the
conflock parameter from lm_grant since the callers always pass in NULL
anyway. You may want to pull in his patch and rebase yours on top of it
since it'll remove that __locks_copy_lock call altogether.
Joe, is Andrew merging that patch or do I need to pull it into the
locks tree?
> -
> EXPORT_SYMBOL(locks_copy_lock);
>
> static inline int flock_translate_cmd(int cmd) {
> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> break;
> }
> if (cfl) {
> - __locks_copy_lock(fl, cfl);
> + locks_copy_lock(fl, cfl);
> if (cfl->fl_nspid)
> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> } else
> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> if (!posix_locks_conflict(request, fl))
> continue;
> if (conflock)
> - __locks_copy_lock(conflock, fl);
> + locks_copy_lock(conflock, fl);
> error = -EAGAIN;
> if (!(request->fl_flags & FL_SLEEP))
> goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e11d60c..ced023d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
> extern void locks_init_lock(struct file_lock *);
> extern struct file_lock * locks_alloc_lock(void);
> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> extern void locks_remove_posix(struct file *, fl_owner_t);
> extern void locks_remove_file(struct file *);
> extern void locks_release_private(struct file_lock *);
> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> return;
> }
>
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> - return;
> -}
> -
> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> {
> return;
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-11 16:19 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Jeff Layton
@ 2014-08-11 16:25 ` Joe Perches
2014-08-14 12:59 ` Kinglong Mee
2014-08-14 12:26 ` Kinglong Mee
1 sibling, 1 reply; 64+ messages in thread
From: Joe Perches @ 2014-08-11 16:25 UTC (permalink / raw)
To: Jeff Layton, Andrew Morton
Cc: Kinglong Mee, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Mon, 2014-08-11 at 12:19 -0400, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
> > Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> > fl_lmops field in file_lock for checking nfsd4 lockowner.
> >
> > But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> > of conflicting locks) causes the fl_lmops of conflock always be NULL.
> >
> > Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> > locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> >
> > v2: Only change the order from 3/3 to 1/3 now.
> >
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> > fs/lockd/svclock.c | 2 +-
> > fs/locks.c | 25 ++++++-------------------
> > include/linux/fs.h | 6 ------
> > 3 files changed, 7 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> > index ab798a8..e1f209c 100644
> > --- a/fs/lockd/svclock.c
> > +++ b/fs/lockd/svclock.c
> > @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
> > block->b_flags |= B_TIMED_OUT;
> > if (conf) {
> > if (block->b_fl)
> > - __locks_copy_lock(block->b_fl, conf);
> > + locks_copy_lock(block->b_fl, conf);
> > }
> > }
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 717fbc4..91b0f03 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> > new->fl_lmops = fl->fl_lmops;
> > }
> >
> > -/*
> > - * Initialize a new lock from an existing file_lock structure.
> > - */
> > -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> > +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> > {
> > + locks_release_private(new);
> > +
> > new->fl_owner = fl->fl_owner;
> > new->fl_pid = fl->fl_pid;
> > - new->fl_file = NULL;
> > + new->fl_file = fl->fl_file;
> > new->fl_flags = fl->fl_flags;
> > new->fl_type = fl->fl_type;
> > new->fl_start = fl->fl_start;
> > new->fl_end = fl->fl_end;
> > new->fl_ops = NULL;
> > new->fl_lmops = NULL;
> > -}
> > -EXPORT_SYMBOL(__locks_copy_lock);
> > -
> > -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> > -{
> > - locks_release_private(new);
> > -
> > - __locks_copy_lock(new, fl);
> > - new->fl_file = fl->fl_file;
> > - new->fl_ops = fl->fl_ops;
> > - new->fl_lmops = fl->fl_lmops;
> >
> > locks_copy_private(new, fl);
> > }
>
> (cc'ing Joe Perches)
(cc'ing Andrew Morton too)
> Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
> is that you now need to ensure that any conflock structures are
> properly initialized before passing them to locks_copy_lock.
>
> The nfsv4 server code currently doesn't do that and it will need to be
> fixed to do so or that will be a regression.
>
> For the NLM code, Joe Perches has proposed a patch to remove the
> conflock parameter from lm_grant since the callers always pass in NULL
> anyway. You may want to pull in his patch and rebase yours on top of it
> since it'll remove that __locks_copy_lock call altogether.
>
> Joe, is Andrew merging that patch or do I need to pull it into the
> locks tree?
I believe Andrew is merging it.
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-11 16:25 ` Joe Perches
@ 2014-08-14 12:59 ` Kinglong Mee
0 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-14 12:59 UTC (permalink / raw)
To: Joe Perches, Jeff Layton, Andrew Morton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
On 8/12/2014 00:25, Joe Perches wrote:
> On Mon, 2014-08-11 at 12:19 -0400, Jeff Layton wrote:
>> On Sun, 10 Aug 2014 23:38:25 +0800
>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>
>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>
>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>
>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>
>>> v2: Only change the order from 3/3 to 1/3 now.
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>> fs/lockd/svclock.c | 2 +-
>>> fs/locks.c | 25 ++++++-------------------
>>> include/linux/fs.h | 6 ------
>>> 3 files changed, 7 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>>> index ab798a8..e1f209c 100644
>>> --- a/fs/lockd/svclock.c
>>> +++ b/fs/lockd/svclock.c
>>> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
>>> block->b_flags |= B_TIMED_OUT;
>>> if (conf) {
>>> if (block->b_fl)
>>> - __locks_copy_lock(block->b_fl, conf);
>>> + locks_copy_lock(block->b_fl, conf);
>>> }
>>> }
>>>
>>> diff --git a/fs/locks.c b/fs/locks.c
>>> index 717fbc4..91b0f03 100644
>>> --- a/fs/locks.c
>>> +++ b/fs/locks.c
>>> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>> new->fl_lmops = fl->fl_lmops;
>>> }
>>>
>>> -/*
>>> - * Initialize a new lock from an existing file_lock structure.
>>> - */
>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> {
>>> + locks_release_private(new);
>>> +
>>> new->fl_owner = fl->fl_owner;
>>> new->fl_pid = fl->fl_pid;
>>> - new->fl_file = NULL;
>>> + new->fl_file = fl->fl_file;
>>> new->fl_flags = fl->fl_flags;
>>> new->fl_type = fl->fl_type;
>>> new->fl_start = fl->fl_start;
>>> new->fl_end = fl->fl_end;
>>> new->fl_ops = NULL;
>>> new->fl_lmops = NULL;
>>> -}
>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>> -
>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> -{
>>> - locks_release_private(new);
>>> -
>>> - __locks_copy_lock(new, fl);
>>> - new->fl_file = fl->fl_file;
>>> - new->fl_ops = fl->fl_ops;
>>> - new->fl_lmops = fl->fl_lmops;
>>>
>>> locks_copy_private(new, fl);
>>> }
>>
>> (cc'ing Joe Perches)
>
> (cc'ing Andrew Morton too)
>
>> Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
>> is that you now need to ensure that any conflock structures are
>> properly initialized before passing them to locks_copy_lock.
>>
>> The nfsv4 server code currently doesn't do that and it will need to be
>> fixed to do so or that will be a regression.
>>
>> For the NLM code, Joe Perches has proposed a patch to remove the
>> conflock parameter from lm_grant since the callers always pass in NULL
>> anyway. You may want to pull in his patch and rebase yours on top of it
>> since it'll remove that __locks_copy_lock call altogether.
>>
>> Joe, is Andrew merging that patch or do I need to pull it into the
>> locks tree?
>
> I believe Andrew is merging it.
Sorry for I don't known Andrew's git tree,
Can you offer me? Thank you very much.
thanks,
Kinglong Mee
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-11 16:19 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Jeff Layton
2014-08-11 16:25 ` Joe Perches
@ 2014-08-14 12:26 ` Kinglong Mee
2014-08-14 14:00 ` Jeff Layton
1 sibling, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-14 12:26 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, Joe Perches, kinglongmee
On 8/12/2014 00:19, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>
>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>
>> v2: Only change the order from 3/3 to 1/3 now.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>> fs/lockd/svclock.c | 2 +-
>> fs/locks.c | 25 ++++++-------------------
>> include/linux/fs.h | 6 ------
>> 3 files changed, 7 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>> index ab798a8..e1f209c 100644
>> --- a/fs/lockd/svclock.c
>> +++ b/fs/lockd/svclock.c
>> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
>> block->b_flags |= B_TIMED_OUT;
>> if (conf) {
>> if (block->b_fl)
>> - __locks_copy_lock(block->b_fl, conf);
>> + locks_copy_lock(block->b_fl, conf);
>> }
>> }
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index 717fbc4..91b0f03 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>> new->fl_lmops = fl->fl_lmops;
>> }
>>
>> -/*
>> - * Initialize a new lock from an existing file_lock structure.
>> - */
>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> {
>> + locks_release_private(new);
>> +
>> new->fl_owner = fl->fl_owner;
>> new->fl_pid = fl->fl_pid;
>> - new->fl_file = NULL;
>> + new->fl_file = fl->fl_file;
>> new->fl_flags = fl->fl_flags;
>> new->fl_type = fl->fl_type;
>> new->fl_start = fl->fl_start;
>> new->fl_end = fl->fl_end;
>> new->fl_ops = NULL;
>> new->fl_lmops = NULL;
>> -}
>> -EXPORT_SYMBOL(__locks_copy_lock);
>> -
>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> -{
>> - locks_release_private(new);
>> -
>> - __locks_copy_lock(new, fl);
>> - new->fl_file = fl->fl_file;
>> - new->fl_ops = fl->fl_ops;
>> - new->fl_lmops = fl->fl_lmops;
>>
>> locks_copy_private(new, fl);
>> }
>
> (cc'ing Joe Perches)
>
> Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
> is that you now need to ensure that any conflock structures are
> properly initialized before passing them to locks_copy_lock.
>
> The nfsv4 server code currently doesn't do that and it will need to be
> fixed to do so or that will be a regression.
I don't think so.
locks_alloc_lock() has initialize the file_lock struct,
the same as locks_init_lock().
I will clean the duplicate initialize for file_lock in nfs4state.c in v3.
> For the NLM code, Joe Perches has proposed a patch to remove the
> conflock parameter from lm_grant since the callers always pass in NULL
> anyway. You may want to pull in his patch and rebase yours on top of it
> since it'll remove that __locks_copy_lock call altogether.
>
> Joe, is Andrew merging that patch or do I need to pull it into the
> locks tree?
I will update this patch based on that patch and your new patch for locks.c.
thanks,
Kinglong Mee
>
>> -
>> EXPORT_SYMBOL(locks_copy_lock);
>>
>> static inline int flock_translate_cmd(int cmd) {
>> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>> break;
>> }
>> if (cfl) {
>> - __locks_copy_lock(fl, cfl);
>> + locks_copy_lock(fl, cfl);
>> if (cfl->fl_nspid)
>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>> } else
>> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>> if (!posix_locks_conflict(request, fl))
>> continue;
>> if (conflock)
>> - __locks_copy_lock(conflock, fl);
>> + locks_copy_lock(conflock, fl);
>> error = -EAGAIN;
>> if (!(request->fl_flags & FL_SLEEP))
>> goto out;
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e11d60c..ced023d 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
>> extern void locks_init_lock(struct file_lock *);
>> extern struct file_lock * locks_alloc_lock(void);
>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>> extern void locks_remove_posix(struct file *, fl_owner_t);
>> extern void locks_remove_file(struct file *);
>> extern void locks_release_private(struct file_lock *);
>> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>> return;
>> }
>>
>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> -{
>> - return;
>> -}
>> -
>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> {
>> return;
>
>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-14 12:26 ` Kinglong Mee
@ 2014-08-14 14:00 ` Jeff Layton
2014-08-14 14:04 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Jeff Layton @ 2014-08-14 14:00 UTC (permalink / raw)
To: Kinglong Mee
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, Joe Perches
On Thu, 14 Aug 2014 20:26:03 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> On 8/12/2014 00:19, Jeff Layton wrote:
> > On Sun, 10 Aug 2014 23:38:25 +0800
> > Kinglong Mee <kinglongmee@gmail.com> wrote:
> >
> >> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> >> fl_lmops field in file_lock for checking nfsd4 lockowner.
> >>
> >> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> >> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> >>
> >> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> >> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> >>
> >> v2: Only change the order from 3/3 to 1/3 now.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >> fs/lockd/svclock.c | 2 +-
> >> fs/locks.c | 25 ++++++-------------------
> >> include/linux/fs.h | 6 ------
> >> 3 files changed, 7 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> >> index ab798a8..e1f209c 100644
> >> --- a/fs/lockd/svclock.c
> >> +++ b/fs/lockd/svclock.c
> >> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
> >> block->b_flags |= B_TIMED_OUT;
> >> if (conf) {
> >> if (block->b_fl)
> >> - __locks_copy_lock(block->b_fl, conf);
> >> + locks_copy_lock(block->b_fl, conf);
> >> }
> >> }
> >>
> >> diff --git a/fs/locks.c b/fs/locks.c
> >> index 717fbc4..91b0f03 100644
> >> --- a/fs/locks.c
> >> +++ b/fs/locks.c
> >> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> >> new->fl_lmops = fl->fl_lmops;
> >> }
> >>
> >> -/*
> >> - * Initialize a new lock from an existing file_lock structure.
> >> - */
> >> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> >> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> {
> >> + locks_release_private(new);
> >> +
> >> new->fl_owner = fl->fl_owner;
> >> new->fl_pid = fl->fl_pid;
> >> - new->fl_file = NULL;
> >> + new->fl_file = fl->fl_file;
> >> new->fl_flags = fl->fl_flags;
> >> new->fl_type = fl->fl_type;
> >> new->fl_start = fl->fl_start;
> >> new->fl_end = fl->fl_end;
> >> new->fl_ops = NULL;
> >> new->fl_lmops = NULL;
> >> -}
> >> -EXPORT_SYMBOL(__locks_copy_lock);
> >> -
> >> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> -{
> >> - locks_release_private(new);
> >> -
> >> - __locks_copy_lock(new, fl);
> >> - new->fl_file = fl->fl_file;
> >> - new->fl_ops = fl->fl_ops;
> >> - new->fl_lmops = fl->fl_lmops;
> >>
> >> locks_copy_private(new, fl);
> >> }
> >
> > (cc'ing Joe Perches)
> >
> > Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
> > is that you now need to ensure that any conflock structures are
> > properly initialized before passing them to locks_copy_lock.
> >
> > The nfsv4 server code currently doesn't do that and it will need to be
> > fixed to do so or that will be a regression.
>
> I don't think so.
> locks_alloc_lock() has initialize the file_lock struct,
> the same as locks_init_lock().
>
> I will clean the duplicate initialize for file_lock in nfs4state.c in v3.
>
Ahh, you're correct. Yes, please just remove that instead. You might
also want to look for other places in the kernel that call
locks_init_lock unnecessarily. We might as well get rid of all of
them while we're looking.
> > For the NLM code, Joe Perches has proposed a patch to remove the
> > conflock parameter from lm_grant since the callers always pass in NULL
> > anyway. You may want to pull in his patch and rebase yours on top of it
> > since it'll remove that __locks_copy_lock call altogether.
> >
> > Joe, is Andrew merging that patch or do I need to pull it into the
> > locks tree?
>
> I will update this patch based on that patch and your new patch for locks.c.
>
> thanks,
> Kinglong Mee
>
Thanks. I wiggled Joe's patch on top of my current set of locking
patches and will plan to merge it for v3.18 unless there are any
objections.
> >
> >> -
> >> EXPORT_SYMBOL(locks_copy_lock);
> >>
> >> static inline int flock_translate_cmd(int cmd) {
> >> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> >> break;
> >> }
> >> if (cfl) {
> >> - __locks_copy_lock(fl, cfl);
> >> + locks_copy_lock(fl, cfl);
> >> if (cfl->fl_nspid)
> >> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> >> } else
> >> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> >> if (!posix_locks_conflict(request, fl))
> >> continue;
> >> if (conflock)
> >> - __locks_copy_lock(conflock, fl);
> >> + locks_copy_lock(conflock, fl);
> >> error = -EAGAIN;
> >> if (!(request->fl_flags & FL_SLEEP))
> >> goto out;
> >> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >> index e11d60c..ced023d 100644
> >> --- a/include/linux/fs.h
> >> +++ b/include/linux/fs.h
> >> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
> >> extern void locks_init_lock(struct file_lock *);
> >> extern struct file_lock * locks_alloc_lock(void);
> >> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> >> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> >> extern void locks_remove_posix(struct file *, fl_owner_t);
> >> extern void locks_remove_file(struct file *);
> >> extern void locks_release_private(struct file_lock *);
> >> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> >> return;
> >> }
> >>
> >> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> -{
> >> - return;
> >> -}
> >> -
> >> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> {
> >> return;
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock
2014-08-14 14:00 ` Jeff Layton
@ 2014-08-14 14:04 ` Kinglong Mee
0 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-14 14:04 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, Joe Perches
On 8/14/2014 22:00, Jeff Layton wrote:
> On Thu, 14 Aug 2014 20:26:03 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> On 8/12/2014 00:19, Jeff Layton wrote:
>>> On Sun, 10 Aug 2014 23:38:25 +0800
>>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>
>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>
>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>
>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>
>>>> v2: Only change the order from 3/3 to 1/3 now.
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>> ---
>>>> fs/lockd/svclock.c | 2 +-
>>>> fs/locks.c | 25 ++++++-------------------
>>>> include/linux/fs.h | 6 ------
>>>> 3 files changed, 7 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>>>> index ab798a8..e1f209c 100644
>>>> --- a/fs/lockd/svclock.c
>>>> +++ b/fs/lockd/svclock.c
>>>> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
>>>> block->b_flags |= B_TIMED_OUT;
>>>> if (conf) {
>>>> if (block->b_fl)
>>>> - __locks_copy_lock(block->b_fl, conf);
>>>> + locks_copy_lock(block->b_fl, conf);
>>>> }
>>>> }
>>>>
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index 717fbc4..91b0f03 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>> new->fl_lmops = fl->fl_lmops;
>>>> }
>>>>
>>>> -/*
>>>> - * Initialize a new lock from an existing file_lock structure.
>>>> - */
>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> {
>>>> + locks_release_private(new);
>>>> +
>>>> new->fl_owner = fl->fl_owner;
>>>> new->fl_pid = fl->fl_pid;
>>>> - new->fl_file = NULL;
>>>> + new->fl_file = fl->fl_file;
>>>> new->fl_flags = fl->fl_flags;
>>>> new->fl_type = fl->fl_type;
>>>> new->fl_start = fl->fl_start;
>>>> new->fl_end = fl->fl_end;
>>>> new->fl_ops = NULL;
>>>> new->fl_lmops = NULL;
>>>> -}
>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>> -
>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> - locks_release_private(new);
>>>> -
>>>> - __locks_copy_lock(new, fl);
>>>> - new->fl_file = fl->fl_file;
>>>> - new->fl_ops = fl->fl_ops;
>>>> - new->fl_lmops = fl->fl_lmops;
>>>>
>>>> locks_copy_private(new, fl);
>>>> }
>>>
>>> (cc'ing Joe Perches)
>>>
>>> Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
>>> is that you now need to ensure that any conflock structures are
>>> properly initialized before passing them to locks_copy_lock.
>>>
>>> The nfsv4 server code currently doesn't do that and it will need to be
>>> fixed to do so or that will be a regression.
>>
>> I don't think so.
>> locks_alloc_lock() has initialize the file_lock struct,
>> the same as locks_init_lock().
>>
>> I will clean the duplicate initialize for file_lock in nfs4state.c in v3.
>>
>
> Ahh, you're correct. Yes, please just remove that instead. You might
> also want to look for other places in the kernel that call
> locks_init_lock unnecessarily. We might as well get rid of all of
> them while we're looking.
OK, I will review those codes where calling locks_init_lock().
>
>>> For the NLM code, Joe Perches has proposed a patch to remove the
>>> conflock parameter from lm_grant since the callers always pass in NULL
>>> anyway. You may want to pull in his patch and rebase yours on top of it
>>> since it'll remove that __locks_copy_lock call altogether.
>>>
>>> Joe, is Andrew merging that patch or do I need to pull it into the
>>> locks tree?
>>
>> I will update this patch based on that patch and your new patch for locks.c.
>>
>> thanks,
>> Kinglong Mee
>>
>
> Thanks. I wiggled Joe's patch on top of my current set of locking
> patches and will plan to merge it for v3.18 unless there are any
> objections.
I saw your patch, thank you very much.
thanks,
Kinglong Mee
>
>>>
>>>> -
>>>> EXPORT_SYMBOL(locks_copy_lock);
>>>>
>>>> static inline int flock_translate_cmd(int cmd) {
>>>> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>> break;
>>>> }
>>>> if (cfl) {
>>>> - __locks_copy_lock(fl, cfl);
>>>> + locks_copy_lock(fl, cfl);
>>>> if (cfl->fl_nspid)
>>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>> } else
>>>> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>> if (!posix_locks_conflict(request, fl))
>>>> continue;
>>>> if (conflock)
>>>> - __locks_copy_lock(conflock, fl);
>>>> + locks_copy_lock(conflock, fl);
>>>> error = -EAGAIN;
>>>> if (!(request->fl_flags & FL_SLEEP))
>>>> goto out;
>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>> index e11d60c..ced023d 100644
>>>> --- a/include/linux/fs.h
>>>> +++ b/include/linux/fs.h
>>>> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
>>>> extern void locks_init_lock(struct file_lock *);
>>>> extern struct file_lock * locks_alloc_lock(void);
>>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>> extern void locks_remove_posix(struct file *, fl_owner_t);
>>>> extern void locks_remove_file(struct file *);
>>>> extern void locks_release_private(struct file_lock *);
>>>> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>> return;
>>>> }
>>>>
>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> - return;
>>>> -}
>>>> -
>>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> {
>>>> return;
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
` (2 preceding siblings ...)
2014-08-11 16:19 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Jeff Layton
@ 2014-08-15 0:02 ` Kinglong Mee
2014-08-15 10:57 ` Jeff Layton
2014-08-15 0:07 ` [PATCH 2/5 v3] locks: Copy all infomation for conflock Kinglong Mee
` (3 subsequent siblings)
7 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 0:02 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
locks_alloc_lock() has initialize struct file_lock,
don't need re-initialize it by locks_init_lock().
v3: A new patch isn't exit in v2/v1.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..98edf97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
fl = locks_alloc_lock();
if (!fl)
return NULL;
- locks_init_lock(fl);
fl->fl_lmops = &nfsd_lease_mng_ops;
fl->fl_flags = FL_DELEG;
fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
@@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
fp = lock_stp->st_stid.sc_file;
- locks_init_lock(file_lock);
switch (lock->lk_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto out;
}
- locks_init_lock(file_lock);
+
switch (lockt->lt_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto fput;
}
- locks_init_lock(file_lock);
+
file_lock->fl_type = F_UNLCK;
file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
file_lock->fl_pid = current->tgid;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock
2014-08-15 0:02 ` [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock Kinglong Mee
@ 2014-08-15 10:57 ` Jeff Layton
2014-08-15 21:35 ` J. Bruce Fields
0 siblings, 1 reply; 64+ messages in thread
From: Jeff Layton @ 2014-08-15 10:57 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Fri, 15 Aug 2014 08:02:55 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> locks_alloc_lock() has initialize struct file_lock,
> don't need re-initialize it by locks_init_lock().
>
> v3: A new patch isn't exit in v2/v1.
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/nfsd/nfs4state.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e80a59..98edf97 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
> fl = locks_alloc_lock();
> if (!fl)
> return NULL;
> - locks_init_lock(fl);
> fl->fl_lmops = &nfsd_lease_mng_ops;
> fl->fl_flags = FL_DELEG;
> fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
> @@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> }
>
> fp = lock_stp->st_stid.sc_file;
> - locks_init_lock(file_lock);
> switch (lock->lk_type) {
> case NFS4_READ_LT:
> case NFS4_READW_LT:
> @@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_jukebox;
> goto out;
> }
> - locks_init_lock(file_lock);
> +
> switch (lockt->lt_type) {
> case NFS4_READ_LT:
> case NFS4_READW_LT:
> @@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_jukebox;
> goto fput;
> }
> - locks_init_lock(file_lock);
> +
> file_lock->fl_type = F_UNLCK;
> file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> file_lock->fl_pid = current->tgid;
Bruce, since this is an nfsd patch, do you mind picking this one up?
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock
2014-08-15 10:57 ` Jeff Layton
@ 2014-08-15 21:35 ` J. Bruce Fields
0 siblings, 0 replies; 64+ messages in thread
From: J. Bruce Fields @ 2014-08-15 21:35 UTC (permalink / raw)
To: Jeff Layton
Cc: Kinglong Mee, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
On Fri, Aug 15, 2014 at 06:57:41AM -0400, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:02:55 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
> > locks_alloc_lock() has initialize struct file_lock,
> > don't need re-initialize it by locks_init_lock().
> >
> > v3: A new patch isn't exit in v2/v1.
> >
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> > fs/nfsd/nfs4state.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 2e80a59..98edf97 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
> > fl = locks_alloc_lock();
> > if (!fl)
> > return NULL;
> > - locks_init_lock(fl);
> > fl->fl_lmops = &nfsd_lease_mng_ops;
> > fl->fl_flags = FL_DELEG;
> > fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
> > @@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > }
> >
> > fp = lock_stp->st_stid.sc_file;
> > - locks_init_lock(file_lock);
> > switch (lock->lk_type) {
> > case NFS4_READ_LT:
> > case NFS4_READW_LT:
> > @@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > status = nfserr_jukebox;
> > goto out;
> > }
> > - locks_init_lock(file_lock);
> > +
> > switch (lockt->lt_type) {
> > case NFS4_READ_LT:
> > case NFS4_READW_LT:
> > @@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > status = nfserr_jukebox;
> > goto fput;
> > }
> > - locks_init_lock(file_lock);
> > +
> > file_lock->fl_type = F_UNLCK;
> > file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> > file_lock->fl_pid = current->tgid;
>
> Bruce, since this is an nfsd patch, do you mind picking this one up?
>
> Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Yep, got it, thanks!
--b.
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
` (3 preceding siblings ...)
2014-08-15 0:02 ` [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock Kinglong Mee
@ 2014-08-15 0:07 ` Kinglong Mee
2014-08-15 11:14 ` Jeff Layton
2014-08-15 0:09 ` [PATCH 3/5 v3] locks: New ops in file_lock_operations for copy/release owner Kinglong Mee
` (2 subsequent siblings)
7 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 0:07 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
fl_lmops field in file_lock for checking nfsd4 lockowner.
But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
of conflicting locks) causes the fl_lmops of conflock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call
locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
Make sure copy the private information by fl_copy_lock() in struct
file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
v3: Update based on Joe and Jeff's patch.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 24 +++++++-----------------
include/linux/fs.h | 6 ------
2 files changed, 7 insertions(+), 23 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index cb66fb0..fe52abb 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
/*
* Initialize a new lock from an existing file_lock structure.
*/
-void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
+void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
{
+ /* "new" must be a freshly-initialized lock */
+ WARN_ON_ONCE(new->fl_ops);
+
new->fl_owner = fl->fl_owner;
new->fl_pid = fl->fl_pid;
- new->fl_file = NULL;
+ new->fl_file = fl->fl_file;
new->fl_flags = fl->fl_flags;
new->fl_type = fl->fl_type;
new->fl_start = fl->fl_start;
new->fl_end = fl->fl_end;
new->fl_ops = NULL;
new->fl_lmops = NULL;
-}
-EXPORT_SYMBOL(__locks_copy_lock);
-
-void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
-{
- /* "new" must be a freshly-initialized lock */
- WARN_ON_ONCE(new->fl_ops);
-
- __locks_copy_lock(new, fl);
- new->fl_file = fl->fl_file;
- new->fl_ops = fl->fl_ops;
- new->fl_lmops = fl->fl_lmops;
locks_copy_private(new, fl);
}
-
EXPORT_SYMBOL(locks_copy_lock);
static inline int flock_translate_cmd(int cmd) {
@@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
break;
}
if (cfl) {
- __locks_copy_lock(fl, cfl);
+ locks_copy_lock(fl, cfl);
if (cfl->fl_nspid)
fl->fl_pid = pid_vnr(cfl->fl_nspid);
} else
@@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
if (!posix_locks_conflict(request, fl))
continue;
if (conflock)
- __locks_copy_lock(conflock, fl);
+ locks_copy_lock(conflock, fl);
error = -EAGAIN;
if (!(request->fl_flags & FL_SLEEP))
goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 908af4f..a383a30 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
extern void locks_init_lock(struct file_lock *);
extern struct file_lock * locks_alloc_lock(void);
extern void locks_copy_lock(struct file_lock *, struct file_lock *);
-extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
extern void locks_remove_posix(struct file *, fl_owner_t);
extern void locks_remove_file(struct file *);
extern void locks_release_private(struct file_lock *);
@@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
return;
}
-static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
-{
- return;
-}
-
static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
{
return;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-15 0:07 ` [PATCH 2/5 v3] locks: Copy all infomation for conflock Kinglong Mee
@ 2014-08-15 11:14 ` Jeff Layton
2014-08-15 14:33 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Jeff Layton @ 2014-08-15 11:14 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Fri, 15 Aug 2014 08:07:12 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
>
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>
> Make sure copy the private information by fl_copy_lock() in struct
> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>
> v3: Update based on Joe and Jeff's patch.
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/locks.c | 24 +++++++-----------------
> include/linux/fs.h | 6 ------
> 2 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index cb66fb0..fe52abb 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> /*
> * Initialize a new lock from an existing file_lock structure.
> */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> {
> + /* "new" must be a freshly-initialized lock */
> + WARN_ON_ONCE(new->fl_ops);
> +
> new->fl_owner = fl->fl_owner;
> new->fl_pid = fl->fl_pid;
> - new->fl_file = NULL;
> + new->fl_file = fl->fl_file;
> new->fl_flags = fl->fl_flags;
> new->fl_type = fl->fl_type;
> new->fl_start = fl->fl_start;
> new->fl_end = fl->fl_end;
> new->fl_ops = NULL;
> new->fl_lmops = NULL;
> -}
> -EXPORT_SYMBOL(__locks_copy_lock);
> -
> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> - /* "new" must be a freshly-initialized lock */
> - WARN_ON_ONCE(new->fl_ops);
> -
> - __locks_copy_lock(new, fl);
> - new->fl_file = fl->fl_file;
> - new->fl_ops = fl->fl_ops;
> - new->fl_lmops = fl->fl_lmops;
>
> locks_copy_private(new, fl);
> }
> -
> EXPORT_SYMBOL(locks_copy_lock);
>
> static inline int flock_translate_cmd(int cmd) {
> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> break;
> }
> if (cfl) {
> - __locks_copy_lock(fl, cfl);
> + locks_copy_lock(fl, cfl);
> if (cfl->fl_nspid)
> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> } else
> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> if (!posix_locks_conflict(request, fl))
> continue;
> if (conflock)
> - __locks_copy_lock(conflock, fl);
> + locks_copy_lock(conflock, fl);
> error = -EAGAIN;
> if (!(request->fl_flags & FL_SLEEP))
> goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 908af4f..a383a30 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
> extern void locks_init_lock(struct file_lock *);
> extern struct file_lock * locks_alloc_lock(void);
> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> extern void locks_remove_posix(struct file *, fl_owner_t);
> extern void locks_remove_file(struct file *);
> extern void locks_release_private(struct file_lock *);
> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> return;
> }
>
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> - return;
> -}
> -
> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> {
> return;
I'm not sure this is really what you want to do. Calling fl_copy_lock
for a conflock looks relatively harmless for nfs and nlm. AFS though
seems to add the lock to a list associated with the inode. That seems a
little suspicious for a conflock and could be problematic. It may be
best to avoid dealing with fl_ops for a conflock.
Also in the case of fcntl_getlk, the struct file_lock lives on the
stack, and locks_release_private is never called on it. You'll need to
audit all of the current callers of __locks_copy_lock to ensure that
any resources you end up taking references on when copying the conflock
are eventually released.
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-15 11:14 ` Jeff Layton
@ 2014-08-15 14:33 ` Kinglong Mee
2014-08-16 13:35 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 14:33 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
On 8/15/2014 19:14, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:07:12 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>
>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>
>> Make sure copy the private information by fl_copy_lock() in struct
>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>
>> v3: Update based on Joe and Jeff's patch.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>> fs/locks.c | 24 +++++++-----------------
>> include/linux/fs.h | 6 ------
>> 2 files changed, 7 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/locks.c b/fs/locks.c
>> index cb66fb0..fe52abb 100644
>> --- a/fs/locks.c
>> +++ b/fs/locks.c
>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>> /*
>> * Initialize a new lock from an existing file_lock structure.
>> */
>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> {
>> + /* "new" must be a freshly-initialized lock */
>> + WARN_ON_ONCE(new->fl_ops);
>> +
>> new->fl_owner = fl->fl_owner;
>> new->fl_pid = fl->fl_pid;
>> - new->fl_file = NULL;
>> + new->fl_file = fl->fl_file;
>> new->fl_flags = fl->fl_flags;
>> new->fl_type = fl->fl_type;
>> new->fl_start = fl->fl_start;
>> new->fl_end = fl->fl_end;
>> new->fl_ops = NULL;
>> new->fl_lmops = NULL;
>> -}
>> -EXPORT_SYMBOL(__locks_copy_lock);
>> -
>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> -{
>> - /* "new" must be a freshly-initialized lock */
>> - WARN_ON_ONCE(new->fl_ops);
>> -
>> - __locks_copy_lock(new, fl);
>> - new->fl_file = fl->fl_file;
>> - new->fl_ops = fl->fl_ops;
>> - new->fl_lmops = fl->fl_lmops;
>>
>> locks_copy_private(new, fl);
>> }
>> -
>> EXPORT_SYMBOL(locks_copy_lock);
>>
>> static inline int flock_translate_cmd(int cmd) {
>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>> break;
>> }
>> if (cfl) {
>> - __locks_copy_lock(fl, cfl);
>> + locks_copy_lock(fl, cfl);
>> if (cfl->fl_nspid)
>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>> } else
>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>> if (!posix_locks_conflict(request, fl))
>> continue;
>> if (conflock)
>> - __locks_copy_lock(conflock, fl);
>> + locks_copy_lock(conflock, fl);
>> error = -EAGAIN;
>> if (!(request->fl_flags & FL_SLEEP))
>> goto out;
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 908af4f..a383a30 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>> extern void locks_init_lock(struct file_lock *);
>> extern struct file_lock * locks_alloc_lock(void);
>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>> extern void locks_remove_posix(struct file *, fl_owner_t);
>> extern void locks_remove_file(struct file *);
>> extern void locks_release_private(struct file_lock *);
>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>> return;
>> }
>>
>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> -{
>> - return;
>> -}
>> -
>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>> {
>> return;
>
> I'm not sure this is really what you want to do. Calling fl_copy_lock
> for a conflock looks relatively harmless for nfs and nlm. AFS though
> seems to add the lock to a list associated with the inode. That seems a
> little suspicious for a conflock and could be problematic. It may be
> best to avoid dealing with fl_ops for a conflock.
>
> Also in the case of fcntl_getlk, the struct file_lock lives on the
> stack, and locks_release_private is never called on it. You'll need to
> audit all of the current callers of __locks_copy_lock to ensure that
> any resources you end up taking references on when copying the conflock
> are eventually released.
Sorry for my no further think about it.
I will check that again next day.
Thanks for your comment again.
thanks,
Kinglong Mee
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-15 14:33 ` Kinglong Mee
@ 2014-08-16 13:35 ` Kinglong Mee
2014-08-17 13:42 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-16 13:35 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
On 8/15/2014 22:33, Kinglong Mee wrote:
> On 8/15/2014 19:14, Jeff Layton wrote:
>> On Fri, 15 Aug 2014 08:07:12 +0800
>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>
>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>
>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>
>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>
>>> Make sure copy the private information by fl_copy_lock() in struct
>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>
>>> v3: Update based on Joe and Jeff's patch.
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>> fs/locks.c | 24 +++++++-----------------
>>> include/linux/fs.h | 6 ------
>>> 2 files changed, 7 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/fs/locks.c b/fs/locks.c
>>> index cb66fb0..fe52abb 100644
>>> --- a/fs/locks.c
>>> +++ b/fs/locks.c
>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>> /*
>>> * Initialize a new lock from an existing file_lock structure.
>>> */
>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> {
>>> + /* "new" must be a freshly-initialized lock */
>>> + WARN_ON_ONCE(new->fl_ops);
>>> +
>>> new->fl_owner = fl->fl_owner;
>>> new->fl_pid = fl->fl_pid;
>>> - new->fl_file = NULL;
>>> + new->fl_file = fl->fl_file;
>>> new->fl_flags = fl->fl_flags;
>>> new->fl_type = fl->fl_type;
>>> new->fl_start = fl->fl_start;
>>> new->fl_end = fl->fl_end;
>>> new->fl_ops = NULL;
>>> new->fl_lmops = NULL;
>>> -}
>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>> -
>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> -{
>>> - /* "new" must be a freshly-initialized lock */
>>> - WARN_ON_ONCE(new->fl_ops);
>>> -
>>> - __locks_copy_lock(new, fl);
>>> - new->fl_file = fl->fl_file;
>>> - new->fl_ops = fl->fl_ops;
>>> - new->fl_lmops = fl->fl_lmops;
>>>
>>> locks_copy_private(new, fl);
>>> }
>>> -
>>> EXPORT_SYMBOL(locks_copy_lock);
>>>
>>> static inline int flock_translate_cmd(int cmd) {
>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>> break;
>>> }
>>> if (cfl) {
>>> - __locks_copy_lock(fl, cfl);
>>> + locks_copy_lock(fl, cfl);
>>> if (cfl->fl_nspid)
>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>> } else
>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>> if (!posix_locks_conflict(request, fl))
>>> continue;
>>> if (conflock)
>>> - __locks_copy_lock(conflock, fl);
>>> + locks_copy_lock(conflock, fl);
>>> error = -EAGAIN;
>>> if (!(request->fl_flags & FL_SLEEP))
>>> goto out;
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 908af4f..a383a30 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>>> extern void locks_init_lock(struct file_lock *);
>>> extern struct file_lock * locks_alloc_lock(void);
>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>> extern void locks_remove_posix(struct file *, fl_owner_t);
>>> extern void locks_remove_file(struct file *);
>>> extern void locks_release_private(struct file_lock *);
>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>> return;
>>> }
>>>
>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> -{
>>> - return;
>>> -}
>>> -
>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>> {
>>> return;
>>
>> I'm not sure this is really what you want to do. Calling fl_copy_lock
>> for a conflock looks relatively harmless for nfs and nlm. AFS though
>> seems to add the lock to a list associated with the inode. That seems a
>> little suspicious for a conflock and could be problematic. It may be
>> best to avoid dealing with fl_ops for a conflock.
>>
>> Also in the case of fcntl_getlk, the struct file_lock lives on the
>> stack, and locks_release_private is never called on it. You'll need to
>> audit all of the current callers of __locks_copy_lock to ensure that
>> any resources you end up taking references on when copying the conflock
>> are eventually released.
>
> Sorry for my no further think about it.
> I will check that again next day.
I think we should not change the logical of coping lock,
leave fl_ops and fl_lmops as private data as right now.
I have plan to,
1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
I think it should be a private data, am I right?
2. call locks_copy_private() coping private data specifically.
a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
to point whether coping private data.
b. hack the conflock's fl_flags to do the same thing as a,
adds FL_NEED_PRIV fl_flags only valid for conflock.
I don't think 2.a is a nice resolve, because it changes the interface
and many caller don't care the private data (I think contains fl_owner)
for conflock except nfsd.
So, I'd like *2.b*. A draft is appended as following,
thanks,
Kinglong Mee
------------------snip--------------------------
diff --git a/fs/locks.c b/fs/locks.c
index be1858e..5f0349d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -282,6 +282,8 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
if (fl->fl_lmops) {
if (fl->fl_lmops->lm_copy_owner)
fl->fl_lmops->lm_copy_owner(new, fl);
+ else
+ new->fl_owner = fl->fl_owner;
new->fl_lmops = fl->fl_lmops;
}
}
@@ -291,7 +293,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
*/
void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
{
- new->fl_owner = fl->fl_owner;
+ new->fl_owner = NULL;
new->fl_pid = fl->fl_pid;
new->fl_file = NULL;
new->fl_flags = fl->fl_flags;
@@ -734,6 +736,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
{
struct file_lock *cfl;
struct inode *inode = file_inode(filp);
+ bool need_priv = !!(fl->fl_flags & FL_NEED_PRIV);
spin_lock(&inode->i_lock);
for (cfl = file_inode(filp)->i_flock; cfl; cfl = cfl->fl_next) {
@@ -746,6 +749,8 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
__locks_copy_lock(fl, cfl);
if (cfl->fl_nspid)
fl->fl_pid = pid_vnr(cfl->fl_nspid);
+ if (need_priv)
+ locks_copy_private(fl, cfl);
} else
fl->fl_type = F_UNLCK;
spin_unlock(&inode->i_lock);
@@ -919,7 +924,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
struct file_lock *right = NULL;
struct file_lock **before;
int error;
- bool added = false;
+ bool added = false, need_priv = false;
LIST_HEAD(dispose);
/*
@@ -948,8 +953,12 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
continue;
if (!posix_locks_conflict(request, fl))
continue;
- if (conflock)
+ if (conflock) {
+ need_priv = !!(conflock->fl_flags & FL_NEED_PRIV);
__locks_copy_lock(conflock, fl);
+ if (need_priv)
+ locks_copy_private(conflock, fl);
+ }
error = -EAGAIN;
if (!(request->fl_flags & FL_SLEEP))
goto out;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5076497..3db43f9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5275,6 +5275,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
}
+ conflock->fl_flags = FL_NEED_PRIV;
err = vfs_lock_file(filp, F_SETLK, file_lock, conflock);
switch (-err) {
case 0: /* success! */
@@ -5396,7 +5397,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (lo)
file_lock->fl_owner = (fl_owner_t)lo;
file_lock->fl_pid = current->tgid;
- file_lock->fl_flags = FL_POSIX;
+ file_lock->fl_flags = FL_POSIX | FL_NEED_PRIV;
file_lock->fl_start = lockt->lt_offset;
file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 56f2acd..f257d0d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f)
#define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */
#define FL_UNLOCK_PENDING 512 /* Lease is being broken */
#define FL_OFDLCK 1024 /* lock is "owned" by struct file */
+#define FL_NEED_PRIV 2048 /* Need copy private data from conflock */
/*
* Special return value from posix_lock_file() and vfs_lock_file() for
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-16 13:35 ` Kinglong Mee
@ 2014-08-17 13:42 ` Kinglong Mee
2014-08-18 11:54 ` Jeff Layton
0 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-17 13:42 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
On 8/16/2014 21:35, Kinglong Mee wrote:
> On 8/15/2014 22:33, Kinglong Mee wrote:
>> On 8/15/2014 19:14, Jeff Layton wrote:
>>> On Fri, 15 Aug 2014 08:07:12 +0800
>>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>
>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>
>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>
>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>
>>>> Make sure copy the private information by fl_copy_lock() in struct
>>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>>
>>>> v3: Update based on Joe and Jeff's patch.
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>> ---
>>>> fs/locks.c | 24 +++++++-----------------
>>>> include/linux/fs.h | 6 ------
>>>> 2 files changed, 7 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index cb66fb0..fe52abb 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>> /*
>>>> * Initialize a new lock from an existing file_lock structure.
>>>> */
>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> {
>>>> + /* "new" must be a freshly-initialized lock */
>>>> + WARN_ON_ONCE(new->fl_ops);
>>>> +
>>>> new->fl_owner = fl->fl_owner;
>>>> new->fl_pid = fl->fl_pid;
>>>> - new->fl_file = NULL;
>>>> + new->fl_file = fl->fl_file;
>>>> new->fl_flags = fl->fl_flags;
>>>> new->fl_type = fl->fl_type;
>>>> new->fl_start = fl->fl_start;
>>>> new->fl_end = fl->fl_end;
>>>> new->fl_ops = NULL;
>>>> new->fl_lmops = NULL;
>>>> -}
>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>> -
>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> - /* "new" must be a freshly-initialized lock */
>>>> - WARN_ON_ONCE(new->fl_ops);
>>>> -
>>>> - __locks_copy_lock(new, fl);
>>>> - new->fl_file = fl->fl_file;
>>>> - new->fl_ops = fl->fl_ops;
>>>> - new->fl_lmops = fl->fl_lmops;
>>>>
>>>> locks_copy_private(new, fl);
>>>> }
>>>> -
>>>> EXPORT_SYMBOL(locks_copy_lock);
>>>>
>>>> static inline int flock_translate_cmd(int cmd) {
>>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>> break;
>>>> }
>>>> if (cfl) {
>>>> - __locks_copy_lock(fl, cfl);
>>>> + locks_copy_lock(fl, cfl);
>>>> if (cfl->fl_nspid)
>>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>> } else
>>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>> if (!posix_locks_conflict(request, fl))
>>>> continue;
>>>> if (conflock)
>>>> - __locks_copy_lock(conflock, fl);
>>>> + locks_copy_lock(conflock, fl);
>>>> error = -EAGAIN;
>>>> if (!(request->fl_flags & FL_SLEEP))
>>>> goto out;
>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>> index 908af4f..a383a30 100644
>>>> --- a/include/linux/fs.h
>>>> +++ b/include/linux/fs.h
>>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>>>> extern void locks_init_lock(struct file_lock *);
>>>> extern struct file_lock * locks_alloc_lock(void);
>>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>> extern void locks_remove_posix(struct file *, fl_owner_t);
>>>> extern void locks_remove_file(struct file *);
>>>> extern void locks_release_private(struct file_lock *);
>>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>> return;
>>>> }
>>>>
>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> - return;
>>>> -}
>>>> -
>>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> {
>>>> return;
>>>
>>> I'm not sure this is really what you want to do. Calling fl_copy_lock
>>> for a conflock looks relatively harmless for nfs and nlm. AFS though
>>> seems to add the lock to a list associated with the inode. That seems a
>>> little suspicious for a conflock and could be problematic. It may be
>>> best to avoid dealing with fl_ops for a conflock.
>>>
>>> Also in the case of fcntl_getlk, the struct file_lock lives on the
>>> stack, and locks_release_private is never called on it. You'll need to
>>> audit all of the current callers of __locks_copy_lock to ensure that
>>> any resources you end up taking references on when copying the conflock
>>> are eventually released.
>>
>> Sorry for my no further think about it.
>> I will check that again next day.
>
> I think we should not change the logical of coping lock,
> leave fl_ops and fl_lmops as private data as right now.
>
> I have plan to,
> 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
> I think it should be a private data, am I right?
> 2. call locks_copy_private() coping private data specifically.
> a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
> to point whether coping private data.
> b. hack the conflock's fl_flags to do the same thing as a,
> adds FL_NEED_PRIV fl_flags only valid for conflock.
>
> I don't think 2.a is a nice resolve, because it changes the interface
> and many caller don't care the private data (I think contains fl_owner)
> for conflock except nfsd.
>
> So, I'd like *2.b*. A draft is appended as following,
I'm so sorry for the first draft, there is a bug of it.
Please using the new draft.
thanks,
Kinglong Mee
------------------snip---------------------------------------------------------
diff --git a/fs/locks.c b/fs/locks.c
index be1858e..128e34f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -279,6 +279,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
new->fl_ops = fl->fl_ops;
}
+ new->fl_owner = fl->fl_owner;
if (fl->fl_lmops) {
if (fl->fl_lmops->lm_copy_owner)
fl->fl_lmops->lm_copy_owner(new, fl);
@@ -291,7 +292,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
*/
void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
{
- new->fl_owner = fl->fl_owner;
+ new->fl_owner = NULL;
new->fl_pid = fl->fl_pid;
new->fl_file = NULL;
new->fl_flags = fl->fl_flags;
@@ -734,6 +735,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
{
struct file_lock *cfl;
struct inode *inode = file_inode(filp);
+ bool need_priv = !!(fl->fl_flags & FL_NEED_PRIV);
spin_lock(&inode->i_lock);
for (cfl = file_inode(filp)->i_flock; cfl; cfl = cfl->fl_next) {
@@ -746,6 +748,8 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
__locks_copy_lock(fl, cfl);
if (cfl->fl_nspid)
fl->fl_pid = pid_vnr(cfl->fl_nspid);
+ if (need_priv)
+ locks_copy_private(fl, cfl);
} else
fl->fl_type = F_UNLCK;
spin_unlock(&inode->i_lock);
@@ -919,7 +923,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
struct file_lock *right = NULL;
struct file_lock **before;
int error;
- bool added = false;
+ bool added = false, need_priv = false;
LIST_HEAD(dispose);
/*
@@ -948,8 +952,12 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
continue;
if (!posix_locks_conflict(request, fl))
continue;
- if (conflock)
+ if (conflock) {
+ need_priv = !!(conflock->fl_flags & FL_NEED_PRIV);
__locks_copy_lock(conflock, fl);
+ if (need_priv)
+ locks_copy_private(conflock, fl);
+ }
error = -EAGAIN;
if (!(request->fl_flags & FL_SLEEP))
goto out;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5076497..3db43f9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5275,6 +5275,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
goto out;
}
+ conflock->fl_flags = FL_NEED_PRIV;
err = vfs_lock_file(filp, F_SETLK, file_lock, conflock);
switch (-err) {
case 0: /* success! */
@@ -5396,7 +5397,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (lo)
file_lock->fl_owner = (fl_owner_t)lo;
file_lock->fl_pid = current->tgid;
- file_lock->fl_flags = FL_POSIX;
+ file_lock->fl_flags = FL_POSIX | FL_NEED_PRIV;
file_lock->fl_start = lockt->lt_offset;
file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 56f2acd..f257d0d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f)
#define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */
#define FL_UNLOCK_PENDING 512 /* Lease is being broken */
#define FL_OFDLCK 1024 /* lock is "owned" by struct file */
+#define FL_NEED_PRIV 2048 /* Need copy private data from conflock */
/*
* Special return value from posix_lock_file() and vfs_lock_file() for
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-17 13:42 ` Kinglong Mee
@ 2014-08-18 11:54 ` Jeff Layton
2014-08-19 15:10 ` Kinglong Mee
0 siblings, 1 reply; 64+ messages in thread
From: Jeff Layton @ 2014-08-18 11:54 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Sun, 17 Aug 2014 21:42:21 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> On 8/16/2014 21:35, Kinglong Mee wrote:
> > On 8/15/2014 22:33, Kinglong Mee wrote:
> >> On 8/15/2014 19:14, Jeff Layton wrote:
> >>> On Fri, 15 Aug 2014 08:07:12 +0800
> >>> Kinglong Mee <kinglongmee@gmail.com> wrote:
> >>>
> >>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> >>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
> >>>>
> >>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> >>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> >>>>
> >>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> >>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> >>>>
> >>>> Make sure copy the private information by fl_copy_lock() in struct
> >>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
> >>>>
> >>>> v3: Update based on Joe and Jeff's patch.
> >>>>
> >>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >>>> ---
> >>>> fs/locks.c | 24 +++++++-----------------
> >>>> include/linux/fs.h | 6 ------
> >>>> 2 files changed, 7 insertions(+), 23 deletions(-)
> >>>>
> >>>> diff --git a/fs/locks.c b/fs/locks.c
> >>>> index cb66fb0..fe52abb 100644
> >>>> --- a/fs/locks.c
> >>>> +++ b/fs/locks.c
> >>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> >>>> /*
> >>>> * Initialize a new lock from an existing file_lock structure.
> >>>> */
> >>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> >>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> {
> >>>> + /* "new" must be a freshly-initialized lock */
> >>>> + WARN_ON_ONCE(new->fl_ops);
> >>>> +
> >>>> new->fl_owner = fl->fl_owner;
> >>>> new->fl_pid = fl->fl_pid;
> >>>> - new->fl_file = NULL;
> >>>> + new->fl_file = fl->fl_file;
> >>>> new->fl_flags = fl->fl_flags;
> >>>> new->fl_type = fl->fl_type;
> >>>> new->fl_start = fl->fl_start;
> >>>> new->fl_end = fl->fl_end;
> >>>> new->fl_ops = NULL;
> >>>> new->fl_lmops = NULL;
> >>>> -}
> >>>> -EXPORT_SYMBOL(__locks_copy_lock);
> >>>> -
> >>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> -{
> >>>> - /* "new" must be a freshly-initialized lock */
> >>>> - WARN_ON_ONCE(new->fl_ops);
> >>>> -
> >>>> - __locks_copy_lock(new, fl);
> >>>> - new->fl_file = fl->fl_file;
> >>>> - new->fl_ops = fl->fl_ops;
> >>>> - new->fl_lmops = fl->fl_lmops;
> >>>>
> >>>> locks_copy_private(new, fl);
> >>>> }
> >>>> -
> >>>> EXPORT_SYMBOL(locks_copy_lock);
> >>>>
> >>>> static inline int flock_translate_cmd(int cmd) {
> >>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> >>>> break;
> >>>> }
> >>>> if (cfl) {
> >>>> - __locks_copy_lock(fl, cfl);
> >>>> + locks_copy_lock(fl, cfl);
> >>>> if (cfl->fl_nspid)
> >>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> >>>> } else
> >>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> >>>> if (!posix_locks_conflict(request, fl))
> >>>> continue;
> >>>> if (conflock)
> >>>> - __locks_copy_lock(conflock, fl);
> >>>> + locks_copy_lock(conflock, fl);
> >>>> error = -EAGAIN;
> >>>> if (!(request->fl_flags & FL_SLEEP))
> >>>> goto out;
> >>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >>>> index 908af4f..a383a30 100644
> >>>> --- a/include/linux/fs.h
> >>>> +++ b/include/linux/fs.h
> >>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
> >>>> extern void locks_init_lock(struct file_lock *);
> >>>> extern struct file_lock * locks_alloc_lock(void);
> >>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> >>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> >>>> extern void locks_remove_posix(struct file *, fl_owner_t);
> >>>> extern void locks_remove_file(struct file *);
> >>>> extern void locks_release_private(struct file_lock *);
> >>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> >>>> return;
> >>>> }
> >>>>
> >>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> -{
> >>>> - return;
> >>>> -}
> >>>> -
> >>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> {
> >>>> return;
> >>>
> >>> I'm not sure this is really what you want to do. Calling fl_copy_lock
> >>> for a conflock looks relatively harmless for nfs and nlm. AFS though
> >>> seems to add the lock to a list associated with the inode. That seems a
> >>> little suspicious for a conflock and could be problematic. It may be
> >>> best to avoid dealing with fl_ops for a conflock.
> >>>
> >>> Also in the case of fcntl_getlk, the struct file_lock lives on the
> >>> stack, and locks_release_private is never called on it. You'll need to
> >>> audit all of the current callers of __locks_copy_lock to ensure that
> >>> any resources you end up taking references on when copying the conflock
> >>> are eventually released.
> >>
> >> Sorry for my no further think about it.
> >> I will check that again next day.
> >
> > I think we should not change the logical of coping lock,
> > leave fl_ops and fl_lmops as private data as right now.
Why not? I think we'd benefit from making conflock creation a more
distinct operation.
> >
> > I have plan to,
> > 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
> > I think it should be a private data, am I right?
> > 2. call locks_copy_private() coping private data specifically.
> > a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
> > to point whether coping private data.
> > b. hack the conflock's fl_flags to do the same thing as a,
> > adds FL_NEED_PRIV fl_flags only valid for conflock.
> >
> > I don't think 2.a is a nice resolve, because it changes the interface
> > and many caller don't care the private data (I think contains fl_owner)
> > for conflock except nfsd.
> >
> > So, I'd like *2.b*. A draft is appended as following,
>
> I'm so sorry for the first draft, there is a bug of it.
> Please using the new draft.
>
> thanks,
> Kinglong Mee
>
Personally, I'm not a fan of the approach below. I don't think we need
a new flag for this and it doesn't do anything to solve the problem
where the lockowner could go away while you're operating on it in a
conflock.
I think we need several smaller changes:
1. Right now __locks_copy_lock is only used to copy conflocks. It would
be good to rename that to something more distinct (i.e.
locks_copy_conflock), to make it clear that we're generating a conflock
there.
2. Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are
superfluous, since they are callbacks into the filesystem. There should
be no need to bother the filesystem at all with info in a conflock.
But, lock _ownership_ matters for conflocks and that's indicated by the
fl_lmops. So you really do want to copy the fl_lmops for conflocks I
think.
3. Add the operations you added before to fl_lmops to copy and release
the owner (maybe even rename them lm_get_owner/lm_put_owner?), and
ensure that the places that copy conflocks call those operations
appropriately.
That should be all that's required here.
> ------------------snip---------------------------------------------------------
>
> diff --git a/fs/locks.c b/fs/locks.c
> index be1858e..128e34f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -279,6 +279,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> new->fl_ops = fl->fl_ops;
> }
>
> + new->fl_owner = fl->fl_owner;
> if (fl->fl_lmops) {
> if (fl->fl_lmops->lm_copy_owner)
> fl->fl_lmops->lm_copy_owner(new, fl);
> @@ -291,7 +292,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> */
> void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> {
> - new->fl_owner = fl->fl_owner;
> + new->fl_owner = NULL;
> new->fl_pid = fl->fl_pid;
> new->fl_file = NULL;
> new->fl_flags = fl->fl_flags;
> @@ -734,6 +735,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> {
> struct file_lock *cfl;
> struct inode *inode = file_inode(filp);
> + bool need_priv = !!(fl->fl_flags & FL_NEED_PRIV);
>
> spin_lock(&inode->i_lock);
> for (cfl = file_inode(filp)->i_flock; cfl; cfl = cfl->fl_next) {
> @@ -746,6 +748,8 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> __locks_copy_lock(fl, cfl);
> if (cfl->fl_nspid)
> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> + if (need_priv)
> + locks_copy_private(fl, cfl);
> } else
> fl->fl_type = F_UNLCK;
> spin_unlock(&inode->i_lock);
> @@ -919,7 +923,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> struct file_lock *right = NULL;
> struct file_lock **before;
> int error;
> - bool added = false;
> + bool added = false, need_priv = false;
> LIST_HEAD(dispose);
>
> /*
> @@ -948,8 +952,12 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> continue;
> if (!posix_locks_conflict(request, fl))
> continue;
> - if (conflock)
> + if (conflock) {
> + need_priv = !!(conflock->fl_flags & FL_NEED_PRIV);
> __locks_copy_lock(conflock, fl);
> + if (need_priv)
> + locks_copy_private(conflock, fl);
> + }
> error = -EAGAIN;
> if (!(request->fl_flags & FL_SLEEP))
> goto out;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 5076497..3db43f9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5275,6 +5275,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out;
> }
>
> + conflock->fl_flags = FL_NEED_PRIV;
> err = vfs_lock_file(filp, F_SETLK, file_lock, conflock);
> switch (-err) {
> case 0: /* success! */
> @@ -5396,7 +5397,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (lo)
> file_lock->fl_owner = (fl_owner_t)lo;
> file_lock->fl_pid = current->tgid;
> - file_lock->fl_flags = FL_POSIX;
> + file_lock->fl_flags = FL_POSIX | FL_NEED_PRIV;
>
> file_lock->fl_start = lockt->lt_offset;
> file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 56f2acd..f257d0d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f)
> #define FL_DOWNGRADE_PENDING 256 /* Lease is being downgraded */
> #define FL_UNLOCK_PENDING 512 /* Lease is being broken */
> #define FL_OFDLCK 1024 /* lock is "owned" by struct file */
> +#define FL_NEED_PRIV 2048 /* Need copy private data from conflock */
>
> /*
> * Special return value from posix_lock_file() and vfs_lock_file() for
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock
2014-08-18 11:54 ` Jeff Layton
@ 2014-08-19 15:10 ` Kinglong Mee
0 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:10 UTC (permalink / raw)
To: Jeff Layton
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel, kinglongmee
On 8/18/2014 19:54, Jeff Layton wrote:
> On Sun, 17 Aug 2014 21:42:21 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> On 8/16/2014 21:35, Kinglong Mee wrote:
>>> On 8/15/2014 22:33, Kinglong Mee wrote:
>>>> On 8/15/2014 19:14, Jeff Layton wrote:
>>>>> On Fri, 15 Aug 2014 08:07:12 +0800
>>>>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>>>
>>>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>>>
>>>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>>>
>>>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>>>
>>>>>> Make sure copy the private information by fl_copy_lock() in struct
>>>>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>>>>
>>>>>> v3: Update based on Joe and Jeff's patch.
>>>>>>
>>>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>>>> ---
>>>>>> fs/locks.c | 24 +++++++-----------------
>>>>>> include/linux/fs.h | 6 ------
>>>>>> 2 files changed, 7 insertions(+), 23 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>> index cb66fb0..fe52abb 100644
>>>>>> --- a/fs/locks.c
>>>>>> +++ b/fs/locks.c
>>>>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>>>> /*
>>>>>> * Initialize a new lock from an existing file_lock structure.
>>>>>> */
>>>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> {
>>>>>> + /* "new" must be a freshly-initialized lock */
>>>>>> + WARN_ON_ONCE(new->fl_ops);
>>>>>> +
>>>>>> new->fl_owner = fl->fl_owner;
>>>>>> new->fl_pid = fl->fl_pid;
>>>>>> - new->fl_file = NULL;
>>>>>> + new->fl_file = fl->fl_file;
>>>>>> new->fl_flags = fl->fl_flags;
>>>>>> new->fl_type = fl->fl_type;
>>>>>> new->fl_start = fl->fl_start;
>>>>>> new->fl_end = fl->fl_end;
>>>>>> new->fl_ops = NULL;
>>>>>> new->fl_lmops = NULL;
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>>>> -
>>>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> - /* "new" must be a freshly-initialized lock */
>>>>>> - WARN_ON_ONCE(new->fl_ops);
>>>>>> -
>>>>>> - __locks_copy_lock(new, fl);
>>>>>> - new->fl_file = fl->fl_file;
>>>>>> - new->fl_ops = fl->fl_ops;
>>>>>> - new->fl_lmops = fl->fl_lmops;
>>>>>>
>>>>>> locks_copy_private(new, fl);
>>>>>> }
>>>>>> -
>>>>>> EXPORT_SYMBOL(locks_copy_lock);
>>>>>>
>>>>>> static inline int flock_translate_cmd(int cmd) {
>>>>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>>>> break;
>>>>>> }
>>>>>> if (cfl) {
>>>>>> - __locks_copy_lock(fl, cfl);
>>>>>> + locks_copy_lock(fl, cfl);
>>>>>> if (cfl->fl_nspid)
>>>>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>>>> } else
>>>>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>>>> if (!posix_locks_conflict(request, fl))
>>>>>> continue;
>>>>>> if (conflock)
>>>>>> - __locks_copy_lock(conflock, fl);
>>>>>> + locks_copy_lock(conflock, fl);
>>>>>> error = -EAGAIN;
>>>>>> if (!(request->fl_flags & FL_SLEEP))
>>>>>> goto out;
>>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>>>> index 908af4f..a383a30 100644
>>>>>> --- a/include/linux/fs.h
>>>>>> +++ b/include/linux/fs.h
>>>>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>>>>>> extern void locks_init_lock(struct file_lock *);
>>>>>> extern struct file_lock * locks_alloc_lock(void);
>>>>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>>>> extern void locks_remove_posix(struct file *, fl_owner_t);
>>>>>> extern void locks_remove_file(struct file *);
>>>>>> extern void locks_release_private(struct file_lock *);
>>>>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> - return;
>>>>>> -}
>>>>>> -
>>>>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> {
>>>>>> return;
>>>>>
>>>>> I'm not sure this is really what you want to do. Calling fl_copy_lock
>>>>> for a conflock looks relatively harmless for nfs and nlm. AFS though
>>>>> seems to add the lock to a list associated with the inode. That seems a
>>>>> little suspicious for a conflock and could be problematic. It may be
>>>>> best to avoid dealing with fl_ops for a conflock.
>>>>>
>>>>> Also in the case of fcntl_getlk, the struct file_lock lives on the
>>>>> stack, and locks_release_private is never called on it. You'll need to
>>>>> audit all of the current callers of __locks_copy_lock to ensure that
>>>>> any resources you end up taking references on when copying the conflock
>>>>> are eventually released.
>>>>
>>>> Sorry for my no further think about it.
>>>> I will check that again next day.
>>>
>>> I think we should not change the logical of coping lock,
>>> leave fl_ops and fl_lmops as private data as right now.
>
> Why not? I think we'd benefit from making conflock creation a more
> distinct operation.
>
>>>
>>> I have plan to,
>>> 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
>>> I think it should be a private data, am I right?
>>> 2. call locks_copy_private() coping private data specifically.
>>> a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
>>> to point whether coping private data.
>>> b. hack the conflock's fl_flags to do the same thing as a,
>>> adds FL_NEED_PRIV fl_flags only valid for conflock.
>>>
>>> I don't think 2.a is a nice resolve, because it changes the interface
>>> and many caller don't care the private data (I think contains fl_owner)
>>> for conflock except nfsd.
>>>
>>> So, I'd like *2.b*. A draft is appended as following,
>>
>> I'm so sorry for the first draft, there is a bug of it.
>> Please using the new draft.
>>
>> thanks,
>> Kinglong Mee
>>
>
> Personally, I'm not a fan of the approach below. I don't think we need
> a new flag for this and it doesn't do anything to solve the problem
> where the lockowner could go away while you're operating on it in a
> conflock.
I just want fix the bug with minimal code change.
But, I known my fault now.
>
> I think we need several smaller changes:
>
> 1. Right now __locks_copy_lock is only used to copy conflocks. It would
> be good to rename that to something more distinct (i.e.
> locks_copy_conflock), to make it clear that we're generating a conflock
> there.
>
> 2. Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are
> superfluous, since they are callbacks into the filesystem. There should
> be no need to bother the filesystem at all with info in a conflock.
> But, lock _ownership_ matters for conflocks and that's indicated by the
> fl_lmops. So you really do want to copy the fl_lmops for conflocks I
> think.
>
> 3. Add the operations you added before to fl_lmops to copy and release
> the owner (maybe even rename them lm_get_owner/lm_put_owner?), and
> ensure that the places that copy conflocks call those operations
> appropriately.
>
> That should be all that's required here.
I think I know what you are advice.
I will send an update patch as v4.
thanks,
Kinglong Mee
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 3/5 v3] locks: New ops in file_lock_operations for copy/release owner
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
` (4 preceding siblings ...)
2014-08-15 0:07 ` [PATCH 2/5 v3] locks: Copy all infomation for conflock Kinglong Mee
@ 2014-08-15 0:09 ` Kinglong Mee
2014-08-15 0:10 ` [PATCH 4/5 v3] NFSD: New helper nfs4_get_stateowner() for atomic_inc reference Kinglong Mee
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
7 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 0:09 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.
v3: same as v2, isn't exist in v1.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 12 ++++++++++--
include/linux/fs.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index fe52abb..cbe9b82 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
fl->fl_ops->fl_release_private(fl);
fl->fl_ops = NULL;
}
- fl->fl_lmops = NULL;
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_release_owner)
+ fl->fl_lmops->lm_release_owner(fl);
+ fl->fl_lmops = NULL;
+ }
}
EXPORT_SYMBOL_GPL(locks_release_private);
@@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
fl->fl_ops->fl_copy_lock(new, fl);
new->fl_ops = fl->fl_ops;
}
- if (fl->fl_lmops)
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_copy_owner)
+ fl->fl_lmops->lm_copy_owner(new, fl);
new->fl_lmops = fl->fl_lmops;
+ }
}
/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a383a30..54bf69d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -868,6 +868,8 @@ struct file_lock_operations {
struct lock_manager_operations {
int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
unsigned long (*lm_owner_key)(struct file_lock *);
+ void (*lm_copy_owner)(struct file_lock *, struct file_lock *);
+ void (*lm_release_owner)(struct file_lock *);
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 4/5 v3] NFSD: New helper nfs4_get_stateowner() for atomic_inc reference
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
` (5 preceding siblings ...)
2014-08-15 0:09 ` [PATCH 3/5 v3] locks: New ops in file_lock_operations for copy/release owner Kinglong Mee
@ 2014-08-15 0:10 ` Kinglong Mee
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
7 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 0:10 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v3: new patch isn't exist in v2/v1.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 98edf97..e087a71 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
spin_unlock(&nn->client_lock);
}
+static inline struct nfs4_stateowner *
+nfs4_get_stateowner(struct nfs4_stateowner *sop)
+{
+ atomic_inc(&sop->so_count);
+ return sop;
+}
+
static int
same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
{
@@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
so_strhash) {
if (!so->so_is_open_owner)
continue;
- if (same_owner_str(so, &open->op_owner)) {
- atomic_inc(&so->so_count);
- return openowner(so);
- }
+ if (same_owner_str(so, &open->op_owner))
+ return openowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
}
while (!list_empty(&clp->cl_openowners)) {
oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
- atomic_inc(&oo->oo_owner.so_count);
+ nfs4_get_stateowner(&oo->oo_owner);
release_openowner(oo);
}
nfsd4_shutdown_callback(clp);
@@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
{
if (!nfsd4_has_session(cstate)) {
mutex_lock(&so->so_replay.rp_mutex);
- cstate->replay_owner = so;
- atomic_inc(&so->so_count);
+ cstate->replay_owner = nfs4_get_stateowner(so);
}
}
@@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_OPEN_STID;
INIT_LIST_HEAD(&stp->st_locks);
- stp->st_stateowner = &oo->oo_owner;
- atomic_inc(&stp->st_stateowner->so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_access_bmap = 0;
@@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
so_strhash) {
if (so->so_is_open_owner)
continue;
- if (!same_owner_str(so, owner))
- continue;
- atomic_inc(&so->so_count);
- return lockowner(so);
+ if (same_owner_str(so, owner))
+ return lockowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_LOCK_STID;
- stp->st_stateowner = &lo->lo_owner;
- atomic_inc(&lo->lo_owner.so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_stid.sc_free = nfs4_free_lock_stateid;
@@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
}
}
- atomic_inc(&sop->so_count);
+ nfs4_get_stateowner(sop);
break;
}
spin_unlock(&clp->cl_lock);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-10 15:38 ` [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock Kinglong Mee
` (6 preceding siblings ...)
2014-08-15 0:10 ` [PATCH 4/5 v3] NFSD: New helper nfs4_get_stateowner() for atomic_inc reference Kinglong Mee
@ 2014-08-15 0:13 ` Kinglong Mee
2014-08-19 15:16 ` [PATCH 1/6 v4] NFSD: Remove the duplicate initialize of file_lock Kinglong Mee
` (5 more replies)
7 siblings, 6 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-15 0:13 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e087a71..5076497 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
lock->fl_end = OFFSET_MAX;
}
-/* Hack!: For now, we're defining this just so we can use a pointer to it
- * as a unique cookie to identify our (NFSv4's) posix locks. */
+static inline struct nfs4_lockowner *
+nfs4_get_lockowner(struct nfs4_lockowner *lo)
+{
+ return lockowner(nfs4_get_stateowner(&lo->lo_owner));
+}
+
+static void nfsd4_fl_copy_owner(struct file_lock *dst, struct file_lock *src)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
+ dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
+}
+
+static void nfsd4_fl_release_owner(struct file_lock *fl)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
+
+ if (lo) {
+ nfs4_put_stateowner(&lo->lo_owner);
+ fl->fl_owner = NULL;
+ }
+}
+
static const struct lock_manager_operations nfsd_posix_mng_ops = {
+ .lm_copy_owner = nfsd4_fl_copy_owner,
+ .lm_release_owner = nfsd4_fl_release_owner,
};
static inline void
@@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_openmode;
goto out;
}
- file_lock->fl_owner = (fl_owner_t)lock_sop;
+
+ file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
@@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfs4_ol_stateid *stp;
struct file *filp = NULL;
struct file_lock *file_lock = NULL;
+ struct nfs4_lockowner *lock_sop = NULL;
__be32 status;
int err;
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_lock_range;
goto put_stateid;
}
+
+ lock_sop = lockowner(stp->st_stateowner);
file_lock = locks_alloc_lock();
if (!file_lock) {
dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
@@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
file_lock->fl_type = F_UNLCK;
- file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
+ file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 1/6 v4] NFSD: Remove the duplicate initialize of file_lock
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
@ 2014-08-19 15:16 ` Kinglong Mee
2014-08-19 15:18 ` [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner Kinglong Mee
` (4 subsequent siblings)
5 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:16 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
locks_alloc_lock() has initialize the struct file_lock,
so, don't need re-initialize it by locks_init_lock().
v4: same as v3, only cleanup
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..98edf97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
fl = locks_alloc_lock();
if (!fl)
return NULL;
- locks_init_lock(fl);
fl->fl_lmops = &nfsd_lease_mng_ops;
fl->fl_flags = FL_DELEG;
fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
@@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
fp = lock_stp->st_stid.sc_file;
- locks_init_lock(file_lock);
switch (lock->lk_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto out;
}
- locks_init_lock(file_lock);
+
switch (lockt->lt_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto fput;
}
- locks_init_lock(file_lock);
+
file_lock->fl_type = F_UNLCK;
file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
file_lock->fl_pid = current->tgid;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
2014-08-19 15:16 ` [PATCH 1/6 v4] NFSD: Remove the duplicate initialize of file_lock Kinglong Mee
@ 2014-08-19 15:18 ` Kinglong Mee
2014-08-19 19:42 ` Jeff Layton
2014-08-19 15:21 ` [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock() Kinglong Mee
` (3 subsequent siblings)
5 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:18 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.
v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 12 ++++++++++--
include/linux/fs.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index cb66fb0..08342e0 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
fl->fl_ops->fl_release_private(fl);
fl->fl_ops = NULL;
}
- fl->fl_lmops = NULL;
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_put_owner)
+ fl->fl_lmops->lm_put_owner(fl);
+ fl->fl_lmops = NULL;
+ }
}
EXPORT_SYMBOL_GPL(locks_release_private);
@@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
fl->fl_ops->fl_copy_lock(new, fl);
new->fl_ops = fl->fl_ops;
}
- if (fl->fl_lmops)
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_get_owner)
+ fl->fl_lmops->lm_get_owner(new, fl);
new->fl_lmops = fl->fl_lmops;
+ }
}
/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 908af4f..6829340 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -868,6 +868,8 @@ struct file_lock_operations {
struct lock_manager_operations {
int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
unsigned long (*lm_owner_key)(struct file_lock *);
+ void (*lm_get_owner)(struct file_lock *, struct file_lock *);
+ void (*lm_put_owner)(struct file_lock *);
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner
2014-08-19 15:18 ` [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner Kinglong Mee
@ 2014-08-19 19:42 ` Jeff Layton
0 siblings, 0 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-19 19:42 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Tue, 19 Aug 2014 23:18:58 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> NFSD or other lockmanager may increase the owner's reference,
> so adds two new options for copying and releasing owner.
>
> v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/locks.c | 12 ++++++++++--
> include/linux/fs.h | 2 ++
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index cb66fb0..08342e0 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
> fl->fl_ops->fl_release_private(fl);
> fl->fl_ops = NULL;
> }
> - fl->fl_lmops = NULL;
>
> + if (fl->fl_lmops) {
> + if (fl->fl_lmops->lm_put_owner)
> + fl->fl_lmops->lm_put_owner(fl);
> + fl->fl_lmops = NULL;
> + }
> }
> EXPORT_SYMBOL_GPL(locks_release_private);
>
> @@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> fl->fl_ops->fl_copy_lock(new, fl);
> new->fl_ops = fl->fl_ops;
> }
> - if (fl->fl_lmops)
> +
> + if (fl->fl_lmops) {
> + if (fl->fl_lmops->lm_get_owner)
> + fl->fl_lmops->lm_get_owner(new, fl);
> new->fl_lmops = fl->fl_lmops;
> + }
> }
>
> /*
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 908af4f..6829340 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -868,6 +868,8 @@ struct file_lock_operations {
> struct lock_manager_operations {
> int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
> unsigned long (*lm_owner_key)(struct file_lock *);
> + void (*lm_get_owner)(struct file_lock *, struct file_lock *);
> + void (*lm_put_owner)(struct file_lock *);
> void (*lm_notify)(struct file_lock *); /* unblock callback */
> int (*lm_grant)(struct file_lock *, int);
> void (*lm_break)(struct file_lock *);
Looks good. I'd probably move patch #3 before this one but that's just
a minor nit.
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock()
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
2014-08-19 15:16 ` [PATCH 1/6 v4] NFSD: Remove the duplicate initialize of file_lock Kinglong Mee
2014-08-19 15:18 ` [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner Kinglong Mee
@ 2014-08-19 15:21 ` Kinglong Mee
2014-08-19 19:46 ` Jeff Layton
2014-08-19 15:24 ` [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock() Kinglong Mee
` (2 subsequent siblings)
5 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:21 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
Jeff advice, " Right now __locks_copy_lock is only used to copy
conflocks. It would be good to rename that to something more
distinct (i.e.locks_copy_conflock), to make it clear that we're
generating a conflock there."
v4: a new patch only rename
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 10 +++++-----
include/linux/fs.h | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 08342e0..c376561 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -289,7 +289,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
/*
* Initialize a new lock from an existing file_lock structure.
*/
-void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
+void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
{
new->fl_owner = fl->fl_owner;
new->fl_pid = fl->fl_pid;
@@ -301,14 +301,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
new->fl_ops = NULL;
new->fl_lmops = NULL;
}
-EXPORT_SYMBOL(__locks_copy_lock);
+EXPORT_SYMBOL(locks_copy_conflock);
void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
{
/* "new" must be a freshly-initialized lock */
WARN_ON_ONCE(new->fl_ops);
- __locks_copy_lock(new, fl);
+ locks_copy_conflock(new, fl);
new->fl_file = fl->fl_file;
new->fl_ops = fl->fl_ops;
new->fl_lmops = fl->fl_lmops;
@@ -743,7 +743,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
break;
}
if (cfl) {
- __locks_copy_lock(fl, cfl);
+ locks_copy_conflock(fl, cfl);
if (cfl->fl_nspid)
fl->fl_pid = pid_vnr(cfl->fl_nspid);
} else
@@ -949,7 +949,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
if (!posix_locks_conflict(request, fl))
continue;
if (conflock)
- __locks_copy_lock(conflock, fl);
+ locks_copy_conflock(conflock, fl);
error = -EAGAIN;
if (!(request->fl_flags & FL_SLEEP))
goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6829340..3b07ce2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -968,7 +968,7 @@ void locks_free_lock(struct file_lock *fl);
extern void locks_init_lock(struct file_lock *);
extern struct file_lock * locks_alloc_lock(void);
extern void locks_copy_lock(struct file_lock *, struct file_lock *);
-extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
+extern void locks_copy_conflock(struct file_lock *, struct file_lock *);
extern void locks_remove_posix(struct file *, fl_owner_t);
extern void locks_remove_file(struct file *);
extern void locks_release_private(struct file_lock *);
@@ -1028,7 +1028,7 @@ static inline void locks_init_lock(struct file_lock *fl)
return;
}
-static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
+static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
{
return;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock()
2014-08-19 15:21 ` [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock() Kinglong Mee
@ 2014-08-19 19:46 ` Jeff Layton
0 siblings, 0 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-19 19:46 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Tue, 19 Aug 2014 23:21:19 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> Jeff advice, " Right now __locks_copy_lock is only used to copy
> conflocks. It would be good to rename that to something more
> distinct (i.e.locks_copy_conflock), to make it clear that we're
> generating a conflock there."
>
> v4: a new patch only rename
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/locks.c | 10 +++++-----
> include/linux/fs.h | 4 ++--
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 08342e0..c376561 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -289,7 +289,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> /*
> * Initialize a new lock from an existing file_lock structure.
> */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
> {
> new->fl_owner = fl->fl_owner;
> new->fl_pid = fl->fl_pid;
> @@ -301,14 +301,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> new->fl_ops = NULL;
> new->fl_lmops = NULL;
> }
> -EXPORT_SYMBOL(__locks_copy_lock);
> +EXPORT_SYMBOL(locks_copy_conflock);
>
> void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> {
> /* "new" must be a freshly-initialized lock */
> WARN_ON_ONCE(new->fl_ops);
>
> - __locks_copy_lock(new, fl);
> + locks_copy_conflock(new, fl);
> new->fl_file = fl->fl_file;
> new->fl_ops = fl->fl_ops;
> new->fl_lmops = fl->fl_lmops;
> @@ -743,7 +743,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> break;
> }
> if (cfl) {
> - __locks_copy_lock(fl, cfl);
> + locks_copy_conflock(fl, cfl);
> if (cfl->fl_nspid)
> fl->fl_pid = pid_vnr(cfl->fl_nspid);
> } else
> @@ -949,7 +949,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> if (!posix_locks_conflict(request, fl))
> continue;
> if (conflock)
> - __locks_copy_lock(conflock, fl);
> + locks_copy_conflock(conflock, fl);
> error = -EAGAIN;
> if (!(request->fl_flags & FL_SLEEP))
> goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6829340..3b07ce2 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -968,7 +968,7 @@ void locks_free_lock(struct file_lock *fl);
> extern void locks_init_lock(struct file_lock *);
> extern struct file_lock * locks_alloc_lock(void);
> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> +extern void locks_copy_conflock(struct file_lock *, struct file_lock *);
> extern void locks_remove_posix(struct file *, fl_owner_t);
> extern void locks_remove_file(struct file *);
> extern void locks_release_private(struct file_lock *);
> @@ -1028,7 +1028,7 @@ static inline void locks_init_lock(struct file_lock *fl)
> return;
> }
>
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> +static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
> {
> return;
> }
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock()
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (2 preceding siblings ...)
2014-08-19 15:21 ` [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock() Kinglong Mee
@ 2014-08-19 15:24 ` Kinglong Mee
2014-08-19 20:08 ` Jeff Layton
2014-08-19 15:25 ` [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference Kinglong Mee
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
5 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:24 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
fl_lmops field in file_lock for checking nfsd4 lockowner.
But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
of conflicting locks) causes the fl_lmops of conflock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call
locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
Make sure copy the private information by fl_copy_lock() in struct
file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops.
fl_ops are superfluous, since they are callbacks into the filesystem.
There should be no need to bother the filesystem at all with info
in a conflock. But, lock _ownership_ matters for conflocks and that's
indicated by the fl_lmops. So you really do want to copy the fl_lmops
for conflocks I think."
v4: only copy fl_lmops for conflock, don't copy fl_ops
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 36 ++++++++++++++++--------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index c376561..0ee775d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl)
EXPORT_SYMBOL(locks_init_lock);
-static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
-{
- if (fl->fl_ops) {
- if (fl->fl_ops->fl_copy_lock)
- fl->fl_ops->fl_copy_lock(new, fl);
- new->fl_ops = fl->fl_ops;
- }
-
- if (fl->fl_lmops) {
- if (fl->fl_lmops->lm_get_owner)
- fl->fl_lmops->lm_get_owner(new, fl);
- new->fl_lmops = fl->fl_lmops;
- }
-}
-
/*
* Initialize a new lock from an existing file_lock structure.
*/
@@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
new->fl_type = fl->fl_type;
new->fl_start = fl->fl_start;
new->fl_end = fl->fl_end;
+ new->fl_lmops = fl->fl_lmops;
new->fl_ops = NULL;
- new->fl_lmops = NULL;
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_get_owner)
+ fl->fl_lmops->lm_get_owner(new, fl);
+ }
}
EXPORT_SYMBOL(locks_copy_conflock);
@@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
WARN_ON_ONCE(new->fl_ops);
locks_copy_conflock(new, fl);
+
new->fl_file = fl->fl_file;
new->fl_ops = fl->fl_ops;
- new->fl_lmops = fl->fl_lmops;
- locks_copy_private(new, fl);
+ if (fl->fl_ops) {
+ if (fl->fl_ops->fl_copy_lock)
+ fl->fl_ops->fl_copy_lock(new, fl);
+ }
}
EXPORT_SYMBOL(locks_copy_lock);
@@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
if (file_lock.fl_type != F_UNLCK) {
error = posix_lock_to_flock(&flock, &file_lock);
if (error)
- goto out;
+ goto rel_priv;
}
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
+rel_priv:
+ locks_release_private(&file_lock);
out:
return error;
}
@@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
-
+
+ locks_release_private(&file_lock);
out:
return error;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock()
2014-08-19 15:24 ` [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock() Kinglong Mee
@ 2014-08-19 20:08 ` Jeff Layton
0 siblings, 0 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-19 20:08 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Tue, 19 Aug 2014 23:24:23 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
>
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>
> Make sure copy the private information by fl_copy_lock() in struct
> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>
> Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops.
> fl_ops are superfluous, since they are callbacks into the filesystem.
> There should be no need to bother the filesystem at all with info
> in a conflock. But, lock _ownership_ matters for conflocks and that's
> indicated by the fl_lmops. So you really do want to copy the fl_lmops
> for conflocks I think."
>
> v4: only copy fl_lmops for conflock, don't copy fl_ops
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/locks.c | 36 ++++++++++++++++--------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index c376561..0ee775d 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl)
>
> EXPORT_SYMBOL(locks_init_lock);
>
> -static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> -{
> - if (fl->fl_ops) {
> - if (fl->fl_ops->fl_copy_lock)
> - fl->fl_ops->fl_copy_lock(new, fl);
> - new->fl_ops = fl->fl_ops;
> - }
> -
> - if (fl->fl_lmops) {
> - if (fl->fl_lmops->lm_get_owner)
> - fl->fl_lmops->lm_get_owner(new, fl);
> - new->fl_lmops = fl->fl_lmops;
> - }
> -}
> -
> /*
> * Initialize a new lock from an existing file_lock structure.
> */
> @@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
> new->fl_type = fl->fl_type;
> new->fl_start = fl->fl_start;
> new->fl_end = fl->fl_end;
> + new->fl_lmops = fl->fl_lmops;
> new->fl_ops = NULL;
> - new->fl_lmops = NULL;
> +
> + if (fl->fl_lmops) {
> + if (fl->fl_lmops->lm_get_owner)
> + fl->fl_lmops->lm_get_owner(new, fl);
> + }
> }
> EXPORT_SYMBOL(locks_copy_conflock);
>
> @@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> WARN_ON_ONCE(new->fl_ops);
>
> locks_copy_conflock(new, fl);
> +
> new->fl_file = fl->fl_file;
> new->fl_ops = fl->fl_ops;
> - new->fl_lmops = fl->fl_lmops;
>
> - locks_copy_private(new, fl);
> + if (fl->fl_ops) {
> + if (fl->fl_ops->fl_copy_lock)
> + fl->fl_ops->fl_copy_lock(new, fl);
> + }
> }
>
> EXPORT_SYMBOL(locks_copy_lock);
> @@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
> if (file_lock.fl_type != F_UNLCK) {
> error = posix_lock_to_flock(&flock, &file_lock);
> if (error)
> - goto out;
> + goto rel_priv;
> }
> error = -EFAULT;
> if (!copy_to_user(l, &flock, sizeof(flock)))
> error = 0;
> +rel_priv:
> + locks_release_private(&file_lock);
> out:
> return error;
> }
> @@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
> error = -EFAULT;
> if (!copy_to_user(l, &flock, sizeof(flock)))
> error = 0;
> -
> +
> + locks_release_private(&file_lock);
> out:
> return error;
> }
Looks good for the most part. I think there is one small piece missing
though.
There is a vfs_test_lock call in nlmsvc_testlock. The conflock in that
case is embedded inside a nlm_lock and locks_release_private is not
called on it (AFAICT). You're not planning to set these new lmops for
lockd, but someone could in the future.
I think it would be good to go ahead and plumb in a call to
locks_release_private there as well to help ensure that no one will
break this in the future. It'll be a no-op for the time being of course.
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (3 preceding siblings ...)
2014-08-19 15:24 ` [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock() Kinglong Mee
@ 2014-08-19 15:25 ` Kinglong Mee
2014-08-19 20:14 ` Jeff Layton
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
5 siblings, 1 reply; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:25 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v4: same as v3
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 98edf97..e087a71 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
spin_unlock(&nn->client_lock);
}
+static inline struct nfs4_stateowner *
+nfs4_get_stateowner(struct nfs4_stateowner *sop)
+{
+ atomic_inc(&sop->so_count);
+ return sop;
+}
+
static int
same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
{
@@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
so_strhash) {
if (!so->so_is_open_owner)
continue;
- if (same_owner_str(so, &open->op_owner)) {
- atomic_inc(&so->so_count);
- return openowner(so);
- }
+ if (same_owner_str(so, &open->op_owner))
+ return openowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
}
while (!list_empty(&clp->cl_openowners)) {
oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
- atomic_inc(&oo->oo_owner.so_count);
+ nfs4_get_stateowner(&oo->oo_owner);
release_openowner(oo);
}
nfsd4_shutdown_callback(clp);
@@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
{
if (!nfsd4_has_session(cstate)) {
mutex_lock(&so->so_replay.rp_mutex);
- cstate->replay_owner = so;
- atomic_inc(&so->so_count);
+ cstate->replay_owner = nfs4_get_stateowner(so);
}
}
@@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_OPEN_STID;
INIT_LIST_HEAD(&stp->st_locks);
- stp->st_stateowner = &oo->oo_owner;
- atomic_inc(&stp->st_stateowner->so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_access_bmap = 0;
@@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
so_strhash) {
if (so->so_is_open_owner)
continue;
- if (!same_owner_str(so, owner))
- continue;
- atomic_inc(&so->so_count);
- return lockowner(so);
+ if (same_owner_str(so, owner))
+ return lockowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_LOCK_STID;
- stp->st_stateowner = &lo->lo_owner;
- atomic_inc(&lo->lo_owner.so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_stid.sc_free = nfs4_free_lock_stateid;
@@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
}
}
- atomic_inc(&sop->so_count);
+ nfs4_get_stateowner(sop);
break;
}
spin_unlock(&clp->cl_lock);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference
2014-08-19 15:25 ` [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference Kinglong Mee
@ 2014-08-19 20:14 ` Jeff Layton
0 siblings, 0 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-19 20:14 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Tue, 19 Aug 2014 23:25:32 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> v4: same as v3
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 98edf97..e087a71 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
> spin_unlock(&nn->client_lock);
> }
>
> +static inline struct nfs4_stateowner *
> +nfs4_get_stateowner(struct nfs4_stateowner *sop)
> +{
> + atomic_inc(&sop->so_count);
> + return sop;
> +}
> +
> static int
> same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
> {
> @@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
> so_strhash) {
> if (!so->so_is_open_owner)
> continue;
> - if (same_owner_str(so, &open->op_owner)) {
> - atomic_inc(&so->so_count);
> - return openowner(so);
> - }
> + if (same_owner_str(so, &open->op_owner))
> + return openowner(nfs4_get_stateowner(so));
> }
> return NULL;
> }
> @@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
> }
> while (!list_empty(&clp->cl_openowners)) {
> oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
> - atomic_inc(&oo->oo_owner.so_count);
> + nfs4_get_stateowner(&oo->oo_owner);
> release_openowner(oo);
> }
> nfsd4_shutdown_callback(clp);
> @@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
> {
> if (!nfsd4_has_session(cstate)) {
> mutex_lock(&so->so_replay.rp_mutex);
> - cstate->replay_owner = so;
> - atomic_inc(&so->so_count);
> + cstate->replay_owner = nfs4_get_stateowner(so);
> }
> }
>
> @@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> atomic_inc(&stp->st_stid.sc_count);
> stp->st_stid.sc_type = NFS4_OPEN_STID;
> INIT_LIST_HEAD(&stp->st_locks);
> - stp->st_stateowner = &oo->oo_owner;
> - atomic_inc(&stp->st_stateowner->so_count);
> + stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
> get_nfs4_file(fp);
> stp->st_stid.sc_file = fp;
> stp->st_access_bmap = 0;
> @@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> so_strhash) {
> if (so->so_is_open_owner)
> continue;
> - if (!same_owner_str(so, owner))
> - continue;
> - atomic_inc(&so->so_count);
> - return lockowner(so);
> + if (same_owner_str(so, owner))
> + return lockowner(nfs4_get_stateowner(so));
> }
> return NULL;
> }
> @@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>
> atomic_inc(&stp->st_stid.sc_count);
> stp->st_stid.sc_type = NFS4_LOCK_STID;
> - stp->st_stateowner = &lo->lo_owner;
> - atomic_inc(&lo->lo_owner.so_count);
> + stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
> get_nfs4_file(fp);
> stp->st_stid.sc_file = fp;
> stp->st_stid.sc_free = nfs4_free_lock_stateid;
> @@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> }
> }
>
> - atomic_inc(&sop->so_count);
> + nfs4_get_stateowner(sop);
> break;
> }
> spin_unlock(&clp->cl_lock);
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-15 0:13 ` [PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (4 preceding siblings ...)
2014-08-19 15:25 ` [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference Kinglong Mee
@ 2014-08-19 15:26 ` Kinglong Mee
2014-08-19 20:23 ` Jeff Layton
` (6 more replies)
5 siblings, 7 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-19 15:26 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v4: same as v3, no change
v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner.
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e087a71..7161111 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
lock->fl_end = OFFSET_MAX;
}
-/* Hack!: For now, we're defining this just so we can use a pointer to it
- * as a unique cookie to identify our (NFSv4's) posix locks. */
+static inline struct nfs4_lockowner *
+nfs4_get_lockowner(struct nfs4_lockowner *lo)
+{
+ return lockowner(nfs4_get_stateowner(&lo->lo_owner));
+}
+
+static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
+ dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
+}
+
+static void nfsd4_fl_put_owner(struct file_lock *fl)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
+
+ if (lo) {
+ nfs4_put_stateowner(&lo->lo_owner);
+ fl->fl_owner = NULL;
+ }
+}
+
static const struct lock_manager_operations nfsd_posix_mng_ops = {
+ .lm_get_owner = nfsd4_fl_get_owner,
+ .lm_put_owner = nfsd4_fl_put_owner,
};
static inline void
@@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_openmode;
goto out;
}
- file_lock->fl_owner = (fl_owner_t)lock_sop;
+
+ file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
@@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
struct nfs4_ol_stateid *stp;
struct file *filp = NULL;
struct file_lock *file_lock = NULL;
+ struct nfs4_lockowner *lock_sop = NULL;
__be32 status;
int err;
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_lock_range;
goto put_stateid;
}
+
+ lock_sop = lockowner(stp->st_stateowner);
file_lock = locks_alloc_lock();
if (!file_lock) {
dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
@@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
file_lock->fl_type = F_UNLCK;
- file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
+ file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
@ 2014-08-19 20:23 ` Jeff Layton
2014-08-19 20:24 ` J. Bruce Fields
2014-08-20 10:02 ` Kinglong Mee
2014-08-20 9:51 ` [PATCH 1/6 v5] NFSD: Remove the duplicate initialize of file_lock Kinglong Mee
` (5 subsequent siblings)
6 siblings, 2 replies; 64+ messages in thread
From: Jeff Layton @ 2014-08-19 20:23 UTC (permalink / raw)
To: Kinglong Mee
Cc: Jeff Layton, J. Bruce Fields, Linux NFS Mailing List,
Trond Myklebust, linux-fsdevel
On Tue, 19 Aug 2014 23:26:45 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:
> v4: same as v3, no change
> v3: Update based on Jeff's comments
> v2: Fix bad using of struct file_lock_operations for handle the owner.
>
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e087a71..7161111 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
> lock->fl_end = OFFSET_MAX;
> }
>
> -/* Hack!: For now, we're defining this just so we can use a pointer to it
> - * as a unique cookie to identify our (NFSv4's) posix locks. */
> +static inline struct nfs4_lockowner *
> +nfs4_get_lockowner(struct nfs4_lockowner *lo)
> +{
> + return lockowner(nfs4_get_stateowner(&lo->lo_owner));
> +}
> +
I'd probably not bother with this inline function. Just open code that
into the callers.
> +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> + dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
> +}
> +
> +static void nfsd4_fl_put_owner(struct file_lock *fl)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> +
> + if (lo) {
> + nfs4_put_stateowner(&lo->lo_owner);
> + fl->fl_owner = NULL;
> + }
> +}
> +
> static const struct lock_manager_operations nfsd_posix_mng_ops = {
> + .lm_get_owner = nfsd4_fl_get_owner,
> + .lm_put_owner = nfsd4_fl_put_owner,
> };
>
> static inline void
> @@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_openmode;
> goto out;
> }
> - file_lock->fl_owner = (fl_owner_t)lock_sop;
> +
> + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> @@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> struct nfs4_ol_stateid *stp;
> struct file *filp = NULL;
> struct file_lock *file_lock = NULL;
> + struct nfs4_lockowner *lock_sop = NULL;
nit: Probably no need to initialize lock_sop to NULL. Even better, I'd
just drop that and change the fl_owner assignment below.
> __be32 status;
> int err;
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> @@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> status = nfserr_lock_range;
> goto put_stateid;
> }
> +
> + lock_sop = lockowner(stp->st_stateowner);
> file_lock = locks_alloc_lock();
> if (!file_lock) {
> dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> @@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> }
>
> file_lock->fl_type = F_UNLCK;
> - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
I'd do this instead and not bother with a nfs4_get_lockowner at all...
file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
But those are minor nits. This looks fine otherwise.
Bruce, if it's OK by you, I'll just take the whole series once Kinglong
respins. It does touch some nfsd code, but it hopefully shouldn't cause
much in the way of conflicts with anything you have queued for v3.18.
Acked-by: Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-19 20:23 ` Jeff Layton
@ 2014-08-19 20:24 ` J. Bruce Fields
2014-08-20 10:02 ` Kinglong Mee
1 sibling, 0 replies; 64+ messages in thread
From: J. Bruce Fields @ 2014-08-19 20:24 UTC (permalink / raw)
To: Jeff Layton
Cc: Kinglong Mee, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
On Tue, Aug 19, 2014 at 04:23:44PM -0400, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
> > v4: same as v3, no change
> > v3: Update based on Jeff's comments
> > v2: Fix bad using of struct file_lock_operations for handle the owner.
> >
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> > fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e087a71..7161111 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
> > lock->fl_end = OFFSET_MAX;
> > }
> >
> > -/* Hack!: For now, we're defining this just so we can use a pointer to it
> > - * as a unique cookie to identify our (NFSv4's) posix locks. */
> > +static inline struct nfs4_lockowner *
> > +nfs4_get_lockowner(struct nfs4_lockowner *lo)
> > +{
> > + return lockowner(nfs4_get_stateowner(&lo->lo_owner));
> > +}
> > +
>
> I'd probably not bother with this inline function. Just open code that
> into the callers.
>
> > +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
> > +{
> > + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> > + dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
> > +}
> > +
> > +static void nfsd4_fl_put_owner(struct file_lock *fl)
> > +{
> > + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> > +
> > + if (lo) {
> > + nfs4_put_stateowner(&lo->lo_owner);
> > + fl->fl_owner = NULL;
> > + }
> > +}
> > +
> > static const struct lock_manager_operations nfsd_posix_mng_ops = {
> > + .lm_get_owner = nfsd4_fl_get_owner,
> > + .lm_put_owner = nfsd4_fl_put_owner,
> > };
> >
> > static inline void
> > @@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > status = nfserr_openmode;
> > goto out;
> > }
> > - file_lock->fl_owner = (fl_owner_t)lock_sop;
> > +
> > + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
> > file_lock->fl_pid = current->tgid;
> > file_lock->fl_file = filp;
> > file_lock->fl_flags = FL_POSIX;
> > @@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > struct nfs4_ol_stateid *stp;
> > struct file *filp = NULL;
> > struct file_lock *file_lock = NULL;
> > + struct nfs4_lockowner *lock_sop = NULL;
>
> nit: Probably no need to initialize lock_sop to NULL. Even better, I'd
> just drop that and change the fl_owner assignment below.
>
> > __be32 status;
> > int err;
> > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > @@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > status = nfserr_lock_range;
> > goto put_stateid;
> > }
> > +
> > + lock_sop = lockowner(stp->st_stateowner);
> > file_lock = locks_alloc_lock();
> > if (!file_lock) {
> > dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> > @@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > }
> >
> > file_lock->fl_type = F_UNLCK;
> > - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> > + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
>
> I'd do this instead and not bother with a nfs4_get_lockowner at all...
>
> file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));
>
> > file_lock->fl_pid = current->tgid;
> > file_lock->fl_file = filp;
> > file_lock->fl_flags = FL_POSIX;
>
> But those are minor nits. This looks fine otherwise.
>
> Bruce, if it's OK by you, I'll just take the whole series once Kinglong
> respins. It does touch some nfsd code, but it hopefully shouldn't cause
> much in the way of conflicts with anything you have queued for v3.18.
That's fine by me.
--b.
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock
2014-08-19 20:23 ` Jeff Layton
2014-08-19 20:24 ` J. Bruce Fields
@ 2014-08-20 10:02 ` Kinglong Mee
1 sibling, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 10:02 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
On 8/20/2014 04:23, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
>
>> v4: same as v3, no change
>> v3: Update based on Jeff's comments
>> v2: Fix bad using of struct file_lock_operations for handle the owner.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
>> 1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index e087a71..7161111 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
>> lock->fl_end = OFFSET_MAX;
>> }
>>
>> -/* Hack!: For now, we're defining this just so we can use a pointer to it
>> - * as a unique cookie to identify our (NFSv4's) posix locks. */
>> +static inline struct nfs4_lockowner *
>> +nfs4_get_lockowner(struct nfs4_lockowner *lo)
>> +{
>> + return lockowner(nfs4_get_stateowner(&lo->lo_owner));
>> +}
>> +
>
> I'd probably not bother with this inline function. Just open code that
> into the callers.
>
>> +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
>> + dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
>> +}
>> +
>> +static void nfsd4_fl_put_owner(struct file_lock *fl)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>> +
>> + if (lo) {
>> + nfs4_put_stateowner(&lo->lo_owner);
>> + fl->fl_owner = NULL;
>> + }
>> +}
>> +
>> static const struct lock_manager_operations nfsd_posix_mng_ops = {
>> + .lm_get_owner = nfsd4_fl_get_owner,
>> + .lm_put_owner = nfsd4_fl_put_owner,
>> };
>>
>> static inline void
>> @@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = nfserr_openmode;
>> goto out;
>> }
>> - file_lock->fl_owner = (fl_owner_t)lock_sop;
>> +
>> + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> @@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> struct nfs4_ol_stateid *stp;
>> struct file *filp = NULL;
>> struct file_lock *file_lock = NULL;
>> + struct nfs4_lockowner *lock_sop = NULL;
>
> nit: Probably no need to initialize lock_sop to NULL. Even better, I'd
> just drop that and change the fl_owner assignment below.
>
>> __be32 status;
>> int err;
>> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>> @@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = nfserr_lock_range;
>> goto put_stateid;
>> }
>> +
>> + lock_sop = lockowner(stp->st_stateowner);
>> file_lock = locks_alloc_lock();
>> if (!file_lock) {
>> dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
>> @@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> }
>>
>> file_lock->fl_type = F_UNLCK;
>> - file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
>> + file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
>
> I'd do this instead and not bother with a nfs4_get_lockowner at all...
>
> file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));
>
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>
> But those are minor nits. This looks fine otherwise.
>
> Bruce, if it's OK by you, I'll just take the whole series once Kinglong
> respins. It does touch some nfsd code, but it hopefully shouldn't cause
> much in the way of conflicts with anything you have queued for v3.18.
Thank you very much for your all comments before.
A new version have be sent, please have a check again.
thanks,
Kinglong Mee
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH 1/6 v5] NFSD: Remove the duplicate initialize of file_lock
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
2014-08-19 20:23 ` Jeff Layton
@ 2014-08-20 9:51 ` Kinglong Mee
2014-08-20 9:53 ` [PATCH 2/6 v5] locks: Rename __locks_copy_lock() to locks_copy_conflock() Kinglong Mee
` (4 subsequent siblings)
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:51 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
locks_alloc_lock() has initialize the struct file_lock,
so, don't need re-initialize it by locks_init_lock().
v5: same the first version, only cleanup
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..98edf97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
fl = locks_alloc_lock();
if (!fl)
return NULL;
- locks_init_lock(fl);
fl->fl_lmops = &nfsd_lease_mng_ops;
fl->fl_flags = FL_DELEG;
fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
@@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
fp = lock_stp->st_stid.sc_file;
- locks_init_lock(file_lock);
switch (lock->lk_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto out;
}
- locks_init_lock(file_lock);
+
switch (lockt->lt_type) {
case NFS4_READ_LT:
case NFS4_READW_LT:
@@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_jukebox;
goto fput;
}
- locks_init_lock(file_lock);
+
file_lock->fl_type = F_UNLCK;
file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
file_lock->fl_pid = current->tgid;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 2/6 v5] locks: Rename __locks_copy_lock() to locks_copy_conflock()
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
2014-08-19 20:23 ` Jeff Layton
2014-08-20 9:51 ` [PATCH 1/6 v5] NFSD: Remove the duplicate initialize of file_lock Kinglong Mee
@ 2014-08-20 9:53 ` Kinglong Mee
2014-08-20 9:54 ` [PATCH 3/6 v5] locks: New ops in file_lock_operations for get/put owner Kinglong Mee
` (3 subsequent siblings)
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:53 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
Jeff advice, " Right now __locks_copy_lock is only used to copy
conflocks. It would be good to rename that to something more
distinct (i.e.locks_copy_conflock), to make it clear that we're
generating a conflock there."
v5: change order from 3/6 to 2/6
v4: new patch only renaming function name
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 10 +++++-----
include/linux/fs.h | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index cb66fb0..49ce390 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -281,7 +281,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
/*
* Initialize a new lock from an existing file_lock structure.
*/
-void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
+void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
{
new->fl_owner = fl->fl_owner;
new->fl_pid = fl->fl_pid;
@@ -293,14 +293,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
new->fl_ops = NULL;
new->fl_lmops = NULL;
}
-EXPORT_SYMBOL(__locks_copy_lock);
+EXPORT_SYMBOL(locks_copy_conflock);
void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
{
/* "new" must be a freshly-initialized lock */
WARN_ON_ONCE(new->fl_ops);
- __locks_copy_lock(new, fl);
+ locks_copy_conflock(new, fl);
new->fl_file = fl->fl_file;
new->fl_ops = fl->fl_ops;
new->fl_lmops = fl->fl_lmops;
@@ -735,7 +735,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
break;
}
if (cfl) {
- __locks_copy_lock(fl, cfl);
+ locks_copy_conflock(fl, cfl);
if (cfl->fl_nspid)
fl->fl_pid = pid_vnr(cfl->fl_nspid);
} else
@@ -941,7 +941,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
if (!posix_locks_conflict(request, fl))
continue;
if (conflock)
- __locks_copy_lock(conflock, fl);
+ locks_copy_conflock(conflock, fl);
error = -EAGAIN;
if (!(request->fl_flags & FL_SLEEP))
goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 908af4f..5ab86f4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -966,7 +966,7 @@ void locks_free_lock(struct file_lock *fl);
extern void locks_init_lock(struct file_lock *);
extern struct file_lock * locks_alloc_lock(void);
extern void locks_copy_lock(struct file_lock *, struct file_lock *);
-extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
+extern void locks_copy_conflock(struct file_lock *, struct file_lock *);
extern void locks_remove_posix(struct file *, fl_owner_t);
extern void locks_remove_file(struct file *);
extern void locks_release_private(struct file_lock *);
@@ -1026,7 +1026,7 @@ static inline void locks_init_lock(struct file_lock *fl)
return;
}
-static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
+static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
{
return;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 3/6 v5] locks: New ops in file_lock_operations for get/put owner
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (2 preceding siblings ...)
2014-08-20 9:53 ` [PATCH 2/6 v5] locks: Rename __locks_copy_lock() to locks_copy_conflock() Kinglong Mee
@ 2014-08-20 9:54 ` Kinglong Mee
2014-08-20 9:56 ` [PATCH 4/6 v5] locks: Copy fl_lmops information for conflock in locks_copy_conflock() Kinglong Mee
` (2 subsequent siblings)
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:54 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.
v5: change order from 2/6 to 3/6
v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/locks.c | 12 ++++++++++--
include/linux/fs.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 49ce390..c376561 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
fl->fl_ops->fl_release_private(fl);
fl->fl_ops = NULL;
}
- fl->fl_lmops = NULL;
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_put_owner)
+ fl->fl_lmops->lm_put_owner(fl);
+ fl->fl_lmops = NULL;
+ }
}
EXPORT_SYMBOL_GPL(locks_release_private);
@@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
fl->fl_ops->fl_copy_lock(new, fl);
new->fl_ops = fl->fl_ops;
}
- if (fl->fl_lmops)
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_get_owner)
+ fl->fl_lmops->lm_get_owner(new, fl);
new->fl_lmops = fl->fl_lmops;
+ }
}
/*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5ab86f4..3b07ce2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -868,6 +868,8 @@ struct file_lock_operations {
struct lock_manager_operations {
int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
unsigned long (*lm_owner_key)(struct file_lock *);
+ void (*lm_get_owner)(struct file_lock *, struct file_lock *);
+ void (*lm_put_owner)(struct file_lock *);
void (*lm_notify)(struct file_lock *); /* unblock callback */
int (*lm_grant)(struct file_lock *, int);
void (*lm_break)(struct file_lock *);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 4/6 v5] locks: Copy fl_lmops information for conflock in locks_copy_conflock()
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (3 preceding siblings ...)
2014-08-20 9:54 ` [PATCH 3/6 v5] locks: New ops in file_lock_operations for get/put owner Kinglong Mee
@ 2014-08-20 9:56 ` Kinglong Mee
2014-08-20 9:57 ` [PATCH 5/6 v5] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference Kinglong Mee
2014-08-20 9:59 ` [PATCH 6/6 v5] NFSD: Get reference of lockowner when coping file_lock Kinglong Mee
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:56 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel
Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
fl_lmops field in file_lock for checking nfsd4 lockowner.
But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
of conflicting locks) causes the fl_lmops of conflock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call
locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
Make sure copy the private information by fl_copy_lock() in struct
file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops.
fl_ops are superfluous, since they are callbacks into the filesystem.
There should be no need to bother the filesystem at all with info
in a conflock. But, lock _ownership_ matters for conflocks and that's
indicated by the fl_lmops. So you really do want to copy the fl_lmops
for conflocks I think."
v5: add missing calling of locks_release_private() in nlmsvc_testlock()
v4: only copy fl_lmops for conflock, don't copy fl_ops
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/lockd/svclock.c | 1 +
fs/locks.c | 36 ++++++++++++++++--------------------
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 796e63b..2e1b292 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -570,6 +570,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
conflock->fl.fl_type = lock->fl.fl_type;
conflock->fl.fl_start = lock->fl.fl_start;
conflock->fl.fl_end = lock->fl.fl_end;
+ locks_release_private(&lock->fl);
ret = nlm_lck_denied;
out:
if (block)
diff --git a/fs/locks.c b/fs/locks.c
index c376561..0ee775d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl)
EXPORT_SYMBOL(locks_init_lock);
-static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
-{
- if (fl->fl_ops) {
- if (fl->fl_ops->fl_copy_lock)
- fl->fl_ops->fl_copy_lock(new, fl);
- new->fl_ops = fl->fl_ops;
- }
-
- if (fl->fl_lmops) {
- if (fl->fl_lmops->lm_get_owner)
- fl->fl_lmops->lm_get_owner(new, fl);
- new->fl_lmops = fl->fl_lmops;
- }
-}
-
/*
* Initialize a new lock from an existing file_lock structure.
*/
@@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
new->fl_type = fl->fl_type;
new->fl_start = fl->fl_start;
new->fl_end = fl->fl_end;
+ new->fl_lmops = fl->fl_lmops;
new->fl_ops = NULL;
- new->fl_lmops = NULL;
+
+ if (fl->fl_lmops) {
+ if (fl->fl_lmops->lm_get_owner)
+ fl->fl_lmops->lm_get_owner(new, fl);
+ }
}
EXPORT_SYMBOL(locks_copy_conflock);
@@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
WARN_ON_ONCE(new->fl_ops);
locks_copy_conflock(new, fl);
+
new->fl_file = fl->fl_file;
new->fl_ops = fl->fl_ops;
- new->fl_lmops = fl->fl_lmops;
- locks_copy_private(new, fl);
+ if (fl->fl_ops) {
+ if (fl->fl_ops->fl_copy_lock)
+ fl->fl_ops->fl_copy_lock(new, fl);
+ }
}
EXPORT_SYMBOL(locks_copy_lock);
@@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
if (file_lock.fl_type != F_UNLCK) {
error = posix_lock_to_flock(&flock, &file_lock);
if (error)
- goto out;
+ goto rel_priv;
}
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
+rel_priv:
+ locks_release_private(&file_lock);
out:
return error;
}
@@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
-
+
+ locks_release_private(&file_lock);
out:
return error;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 5/6 v5] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (4 preceding siblings ...)
2014-08-20 9:56 ` [PATCH 4/6 v5] locks: Copy fl_lmops information for conflock in locks_copy_conflock() Kinglong Mee
@ 2014-08-20 9:57 ` Kinglong Mee
2014-08-20 9:59 ` [PATCH 6/6 v5] NFSD: Get reference of lockowner when coping file_lock Kinglong Mee
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:57 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v5: same as the first version
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 98edf97..e087a71 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
spin_unlock(&nn->client_lock);
}
+static inline struct nfs4_stateowner *
+nfs4_get_stateowner(struct nfs4_stateowner *sop)
+{
+ atomic_inc(&sop->so_count);
+ return sop;
+}
+
static int
same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
{
@@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
so_strhash) {
if (!so->so_is_open_owner)
continue;
- if (same_owner_str(so, &open->op_owner)) {
- atomic_inc(&so->so_count);
- return openowner(so);
- }
+ if (same_owner_str(so, &open->op_owner))
+ return openowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
}
while (!list_empty(&clp->cl_openowners)) {
oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
- atomic_inc(&oo->oo_owner.so_count);
+ nfs4_get_stateowner(&oo->oo_owner);
release_openowner(oo);
}
nfsd4_shutdown_callback(clp);
@@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
{
if (!nfsd4_has_session(cstate)) {
mutex_lock(&so->so_replay.rp_mutex);
- cstate->replay_owner = so;
- atomic_inc(&so->so_count);
+ cstate->replay_owner = nfs4_get_stateowner(so);
}
}
@@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_OPEN_STID;
INIT_LIST_HEAD(&stp->st_locks);
- stp->st_stateowner = &oo->oo_owner;
- atomic_inc(&stp->st_stateowner->so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_access_bmap = 0;
@@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
so_strhash) {
if (so->so_is_open_owner)
continue;
- if (!same_owner_str(so, owner))
- continue;
- atomic_inc(&so->so_count);
- return lockowner(so);
+ if (same_owner_str(so, owner))
+ return lockowner(nfs4_get_stateowner(so));
}
return NULL;
}
@@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
atomic_inc(&stp->st_stid.sc_count);
stp->st_stid.sc_type = NFS4_LOCK_STID;
- stp->st_stateowner = &lo->lo_owner;
- atomic_inc(&lo->lo_owner.so_count);
+ stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
get_nfs4_file(fp);
stp->st_stid.sc_file = fp;
stp->st_stid.sc_free = nfs4_free_lock_stateid;
@@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
}
}
- atomic_inc(&sop->so_count);
+ nfs4_get_stateowner(sop);
break;
}
spin_unlock(&clp->cl_lock);
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH 6/6 v5] NFSD: Get reference of lockowner when coping file_lock
2014-08-19 15:26 ` [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock Kinglong Mee
` (5 preceding siblings ...)
2014-08-20 9:57 ` [PATCH 5/6 v5] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference Kinglong Mee
@ 2014-08-20 9:59 ` Kinglong Mee
6 siblings, 0 replies; 64+ messages in thread
From: Kinglong Mee @ 2014-08-20 9:59 UTC (permalink / raw)
To: Jeff Layton
Cc: J. Bruce Fields, Linux NFS Mailing List, Trond Myklebust,
linux-fsdevel, kinglongmee
v5: using nfs4_get_stateowner() instead of an inline function
v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner
Acked-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
fs/nfsd/nfs4state.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e087a71..fd5ff4b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4869,9 +4869,25 @@ nfs4_transform_lock_offset(struct file_lock *lock)
lock->fl_end = OFFSET_MAX;
}
-/* Hack!: For now, we're defining this just so we can use a pointer to it
- * as a unique cookie to identify our (NFSv4's) posix locks. */
+static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *)src->fl_owner;
+ dst->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lo->lo_owner));
+}
+
+static void nfsd4_fl_put_owner(struct file_lock *fl)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner;
+
+ if (lo) {
+ nfs4_put_stateowner(&lo->lo_owner);
+ fl->fl_owner = NULL;
+ }
+}
+
static const struct lock_manager_operations nfsd_posix_mng_ops = {
+ .lm_get_owner = nfsd4_fl_get_owner,
+ .lm_put_owner = nfsd4_fl_put_owner,
};
static inline void
@@ -5236,7 +5252,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = nfserr_openmode;
goto out;
}
- file_lock->fl_owner = (fl_owner_t)lock_sop;
+
+ file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lock_sop->lo_owner));
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
@@ -5432,7 +5449,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
}
file_lock->fl_type = F_UNLCK;
- file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
+ file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
--
1.9.3
^ permalink raw reply related [flat|nested] 64+ messages in thread