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 for nfsd4_lock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
fs/nfsd/state.h | 1 +
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 9b9474c..1b5afc1 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
-};
+static bool
+nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
+{
+ struct nfs4_stateowner *local = NULL;
+ unsigned hashval = lo->lo_hashval;
+
+ if (hashval >= OWNER_HASH_SIZE)
+ return false;
+
+ list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
+ if (local == &lo->lo_owner)
+ return true;
+ }
+
+ return false;
+}
static inline void
-nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
+nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
+ struct nfsd_net *nn)
{
- struct nfs4_lockowner *lo;
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
- if (fl->fl_lmops == &nfsd_posix_mng_ops) {
- lo = (struct nfs4_lockowner *) fl->fl_owner;
+ if (nfs4_verify_lock_stateowner(lo, nn)) {
deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
lo->lo_owner.so_owner.len, GFP_KERNEL);
if (!deny->ld_owner.data)
@@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
if (!lo)
return NULL;
INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
+ lo->lo_hashval = strhashval;
lo->lo_owner.so_is_open_owner = 0;
/* It is the openowner seqid that will be incremented in encode in the
* case of new lockowners; so increment the lock seqid manually: */
@@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
file_lock->fl_start = lock->lk_offset;
file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
nfs4_transform_lock_offset(file_lock);
@@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
case (EAGAIN): /* conflock holds conflicting lock */
status = nfserr_denied;
dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
- nfs4_set_lock_denied(conflock, &lock->lk_denied);
+ nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
break;
case (EDEADLK):
status = nfserr_deadlock;
@@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (file_lock->fl_type != F_UNLCK) {
status = nfserr_denied;
- nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
+ nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
}
out:
nfs4_unlock_state();
@@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
file_lock->fl_start = locku->lu_offset;
file_lock->fl_end = last_byte_offset(locku->lu_offset,
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1848fed..9ecae58 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -362,6 +362,7 @@ struct nfs4_openowner {
struct nfs4_lockowner {
struct nfs4_stateowner lo_owner; /* must be first element */
+ unsigned int lo_hashval;
};
static inline struct nfs4_openowner * openowner(struct nfs4_stateowner *so)
--
1.9.3
On 7/8/2014 01:26, Jeff Layton wrote:
> On Mon, 07 Jul 2014 22:12:21 +0800
> Kinglong Mee <[email protected]> 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 for nfsd4_lock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
>>
>> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
>>
>> This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
>> fs/nfsd/state.h | 1 +
>> 2 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9b9474c..1b5afc1 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
>> -};
>> +static bool
>> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
>> +{
>> + struct nfs4_stateowner *local = NULL;
>> + unsigned hashval = lo->lo_hashval;
>> +
>> + if (hashval >= OWNER_HASH_SIZE)
>> + return false;
>> +
>> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
>> + if (local == &lo->lo_owner)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>>
>> static inline void
>> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
>> + struct nfsd_net *nn)
>> {
>> - struct nfs4_lockowner *lo;
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>>
>> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
>> - lo = (struct nfs4_lockowner *) fl->fl_owner;
>> + if (nfs4_verify_lock_stateowner(lo, nn)) {
>> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
>> lo->lo_owner.so_owner.len, GFP_KERNEL);
>> if (!deny->ld_owner.data)
>> @@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
>> if (!lo)
>> return NULL;
>> INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
>> + lo->lo_hashval = strhashval;
>> lo->lo_owner.so_is_open_owner = 0;
>> /* It is the openowner seqid that will be incremented in encode in the
>> * case of new lockowners; so increment the lock seqid manually: */
>> @@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> file_lock->fl_start = lock->lk_offset;
>> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
>> nfs4_transform_lock_offset(file_lock);
>> @@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> case (EAGAIN): /* conflock holds conflicting lock */
>> status = nfserr_denied;
>> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
>> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
>> + nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
>> break;
>> case (EDEADLK):
>> status = nfserr_deadlock;
>> @@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>
>> if (file_lock->fl_type != F_UNLCK) {
>> status = nfserr_denied;
>> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
>> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
>> }
>> out:
>> nfs4_unlock_state();
>> @@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> file_lock->fl_start = locku->lu_offset;
>>
>> file_lock->fl_end = last_byte_offset(locku->lu_offset,
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 1848fed..9ecae58 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -362,6 +362,7 @@ struct nfs4_openowner {
>>
>> struct nfs4_lockowner {
>> struct nfs4_stateowner lo_owner; /* must be first element */
>> + unsigned int lo_hashval;
>
> Actually, I have another comment here too. I'm not sure there's a lot
> of value in tracking the lo_hashval here. Why not just recompute it in
> nfs4_verify_lock_stateowner?
No, ownerstr_hashval() needs a client which holds the conflicting lock,
in nfsd4_lock/nfsd4_lockt cannot know the client from vfs_lock_file()
or vfs_test_lock().
>
> You'd need to pass down a pointer to the nfs4_client in order to do so,
> but you could do that instead of passing down 'nn' and then just call
> net_generic on clp->net where you needed it.
I find patch "[PATCH v3 069/114] nfsd: Move the open owner hash table into struct nfs4_client"
have remove the client argument from ownerstr_hashval().
So, the lo_hashval isn't needed after that patch.
After Bruce merging that patch, I will resend a new patch recomputing
the hash value instead recording in lo_hashval.
thanks,
Kinglong Mee
On Mon, 07 Jul 2014 22:12:21 +0800
Kinglong Mee <[email protected]> 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 for nfsd4_lock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
>
> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
>
> This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
> fs/nfsd/state.h | 1 +
> 2 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9b9474c..1b5afc1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> -};
> +static bool
> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
> +{
> + struct nfs4_stateowner *local = NULL;
> + unsigned hashval = lo->lo_hashval;
> +
> + if (hashval >= OWNER_HASH_SIZE)
> + return false;
> +
^^^^
Maybe a WARN_ON_ONCE or something here would be appropriate?
> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> + if (local == &lo->lo_owner)
> + return true;
> + }
> +
> + return false;
> +}
>
> static inline void
> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
> + struct nfsd_net *nn)
> {
> - struct nfs4_lockowner *lo;
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>
> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> - lo = (struct nfs4_lockowner *) fl->fl_owner;
> + if (nfs4_verify_lock_stateowner(lo, nn)) {
> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> lo->lo_owner.so_owner.len, GFP_KERNEL);
> if (!deny->ld_owner.data)
> @@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
> if (!lo)
> return NULL;
> INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
> + lo->lo_hashval = strhashval;
> lo->lo_owner.so_is_open_owner = 0;
> /* It is the openowner seqid that will be incremented in encode in the
> * case of new lockowners; so increment the lock seqid manually: */
> @@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> file_lock->fl_start = lock->lk_offset;
> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> nfs4_transform_lock_offset(file_lock);
> @@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> case (EAGAIN): /* conflock holds conflicting lock */
> status = nfserr_denied;
> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
> + nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
> break;
> case (EDEADLK):
> status = nfserr_deadlock;
> @@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
> if (file_lock->fl_type != F_UNLCK) {
> status = nfserr_denied;
> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
> }
> out:
> nfs4_unlock_state();
> @@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> file_lock->fl_start = locku->lu_offset;
>
> file_lock->fl_end = last_byte_offset(locku->lu_offset,
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1848fed..9ecae58 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -362,6 +362,7 @@ struct nfs4_openowner {
>
> struct nfs4_lockowner {
> struct nfs4_stateowner lo_owner; /* must be first element */
> + unsigned int lo_hashval;
> };
>
> static inline struct nfs4_openowner * openowner(struct nfs4_stateowner *so)
Looks fine other than the comment about the warning. I do have some
patches that change the ownerstr_hashtbl to be a per-client entity.
I'll need to respin them to account for this change, but that shouldn't
be too onerous.
Acked-by: Jeff Layton <[email protected]>
On Tue, Jul 08, 2014 at 08:39:56AM -0400, Jeff Layton wrote:
> <facepalm>
> Ahh ok, I see what you're saying now. I'll leave it to you to respin
> once the rest of the client_mutex removal series is merged.
> </facepalm>
Apologies, ignored this thread, but I'm dropping this patch for now
based on that comment.
--b.
On Mon, 07 Jul 2014 22:12:21 +0800
Kinglong Mee <[email protected]> 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 for nfsd4_lock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
>
> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
>
> This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
> fs/nfsd/state.h | 1 +
> 2 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9b9474c..1b5afc1 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> -};
> +static bool
> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
> +{
> + struct nfs4_stateowner *local = NULL;
> + unsigned hashval = lo->lo_hashval;
> +
> + if (hashval >= OWNER_HASH_SIZE)
> + return false;
> +
> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> + if (local == &lo->lo_owner)
> + return true;
> + }
> +
> + return false;
> +}
>
> static inline void
> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
> + struct nfsd_net *nn)
> {
> - struct nfs4_lockowner *lo;
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>
> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> - lo = (struct nfs4_lockowner *) fl->fl_owner;
> + if (nfs4_verify_lock_stateowner(lo, nn)) {
> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> lo->lo_owner.so_owner.len, GFP_KERNEL);
> if (!deny->ld_owner.data)
> @@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
> if (!lo)
> return NULL;
> INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
> + lo->lo_hashval = strhashval;
> lo->lo_owner.so_is_open_owner = 0;
> /* It is the openowner seqid that will be incremented in encode in the
> * case of new lockowners; so increment the lock seqid manually: */
> @@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> file_lock->fl_start = lock->lk_offset;
> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> nfs4_transform_lock_offset(file_lock);
> @@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> case (EAGAIN): /* conflock holds conflicting lock */
> status = nfserr_denied;
> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
> + nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
> break;
> case (EDEADLK):
> status = nfserr_deadlock;
> @@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
> if (file_lock->fl_type != F_UNLCK) {
> status = nfserr_denied;
> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
> }
> out:
> nfs4_unlock_state();
> @@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> file_lock->fl_start = locku->lu_offset;
>
> file_lock->fl_end = last_byte_offset(locku->lu_offset,
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1848fed..9ecae58 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -362,6 +362,7 @@ struct nfs4_openowner {
>
> struct nfs4_lockowner {
> struct nfs4_stateowner lo_owner; /* must be first element */
> + unsigned int lo_hashval;
Actually, I have another comment here too. I'm not sure there's a lot
of value in tracking the lo_hashval here. Why not just recompute it in
nfs4_verify_lock_stateowner?
You'd need to pass down a pointer to the nfs4_client in order to do so,
but you could do that instead of passing down 'nn' and then just call
net_generic on clp->net where you needed it.
> };
>
> static inline struct nfs4_openowner * openowner(struct
> nfs4_stateowner *so)
--
Jeff Layton <[email protected]>
On 7/8/2014 19:03, Jeff Layton wrote:
> On Tue, 08 Jul 2014 11:23:49 +0800
> Kinglong Mee <[email protected]> wrote:
>
>> On 7/8/2014 01:26, Jeff Layton wrote:
>>> On Mon, 07 Jul 2014 22:12:21 +0800
>>> Kinglong Mee <[email protected]> wrote:
>>>
> -------------------------[snip]------------------------------
>
> [PATCH] NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"
>
> 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 for nfsd4_lock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
>
> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c400ec17915e..00cd7fd17b0f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4312,18 +4312,29 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> -};
> +static bool
> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfs4_client *clp)
> +{
> + struct nfs4_stateowner *local = NULL;
> + struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> + unsigned int hashval = ownerstr_hashval(clp->cl_clientid.cl_id,
> + &lo->lo_owner.so_owner);
Needs the conflicting client which holds the lock,
Not the new client which try to locking the file has be locked.
> +
> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> + if (local == &lo->lo_owner)
> + return true;
> + }
> +
> + return false;
> +}
>
> static inline void
> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
> + struct nfs4_client *clp)
> {
> - struct nfs4_lockowner *lo;
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner;
>
> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> - lo = (struct nfs4_lockowner *) fl->fl_owner;
> + if (nfs4_verify_lock_stateowner(lo, clp)) {
> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> lo->lo_owner.so_owner.len, GFP_KERNEL);
> if (!deny->ld_owner.data)
> @@ -4594,7 +4605,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> file_lock->fl_start = lock->lk_offset;
> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> nfs4_transform_lock_offset(file_lock);
> @@ -4617,7 +4627,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> case (EAGAIN): /* conflock holds conflicting lock */
> status = nfserr_denied;
> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
> + nfs4_set_lock_denied(conflock, &lock->lk_denied,
> + cstate->clp);
> break;
> case (EDEADLK):
> status = nfserr_deadlock;
> @@ -4725,7 +4736,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>
> if (file_lock->fl_type != F_UNLCK) {
> status = nfserr_denied;
> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, cstate->clp);
The clp is the client which try to lock the file here,
but the file have be locked by other client or server.
With this patch, the hashval is wrong.
*correct* *this patch*
clid conflicting client new request lock client
owner from vfs_lock_file() from vfs_lock_file()
or vfs_test_lock() or vfs_test_lock()
hashval right hashval bad hashval (never exist)
With this patch, LOCK/LOCKT compound gets client with all zero, and owner with all zero.
thanks,
Kinglong Mee
On 7/8/2014 00:45, Jeff Layton wrote:
> On Mon, 07 Jul 2014 22:12:21 +0800
> Kinglong Mee <[email protected]> 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 for nfsd4_lock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
>>
>> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
>>
>> This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
>> fs/nfsd/state.h | 1 +
>> 2 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9b9474c..1b5afc1 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
>> -};
>> +static bool
>> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
>> +{
>> + struct nfs4_stateowner *local = NULL;
>> + unsigned hashval = lo->lo_hashval;
>> +
>> + if (hashval >= OWNER_HASH_SIZE)
>> + return false;
>> +
>
> ^^^^
> Maybe a WARN_ON_ONCE or something here would be appropriate?
No, I don't think so.
If the owner stored in fl->fl_owner isn't a nfs4_lockowner,
the lo->lo_hashval field maybe any value, but it's all right.
Just return false here.
>
>> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
>> + if (local == &lo->lo_owner)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>>
>> static inline void
>> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
>> + struct nfsd_net *nn)
>> {
>> - struct nfs4_lockowner *lo;
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>>
>> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
>> - lo = (struct nfs4_lockowner *) fl->fl_owner;
>> + if (nfs4_verify_lock_stateowner(lo, nn)) {
>> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
>> lo->lo_owner.so_owner.len, GFP_KERNEL);
>> if (!deny->ld_owner.data)
>> @@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
>> if (!lo)
>> return NULL;
>> INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
>> + lo->lo_hashval = strhashval;
>> lo->lo_owner.so_is_open_owner = 0;
>> /* It is the openowner seqid that will be incremented in encode in the
>> * case of new lockowners; so increment the lock seqid manually: */
>> @@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> file_lock->fl_start = lock->lk_offset;
>> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
>> nfs4_transform_lock_offset(file_lock);
>> @@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> case (EAGAIN): /* conflock holds conflicting lock */
>> status = nfserr_denied;
>> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
>> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
>> + nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
>> break;
>> case (EDEADLK):
>> status = nfserr_deadlock;
>> @@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>
>> if (file_lock->fl_type != F_UNLCK) {
>> status = nfserr_denied;
>> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
>> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
>> }
>> out:
>> nfs4_unlock_state();
>> @@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> file_lock->fl_start = locku->lu_offset;
>>
>> file_lock->fl_end = last_byte_offset(locku->lu_offset,
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 1848fed..9ecae58 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -362,6 +362,7 @@ struct nfs4_openowner {
>>
>> struct nfs4_lockowner {
>> struct nfs4_stateowner lo_owner; /* must be first element */
>> + unsigned int lo_hashval;
>> };
>>
>> static inline struct nfs4_openowner * openowner(struct nfs4_stateowner *so)
>
> Looks fine other than the comment about the warning. I do have some
> patches that change the ownerstr_hashtbl to be a per-client entity.
> I'll need to respin them to account for this change, but that shouldn't
> be too onerous.
Thanks for your review.
I will follow Bruce's tree and try to remake this patch after your change.
thanks,
Kinglong Mee
On Tue, 08 Jul 2014 11:23:49 +0800
Kinglong Mee <[email protected]> wrote:
> On 7/8/2014 01:26, Jeff Layton wrote:
> > On Mon, 07 Jul 2014 22:12:21 +0800
> > Kinglong Mee <[email protected]> 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 for nfsd4_lock always be NULL.
> >>
> >> Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
> >>
> >> So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
> >>
> >> This patch save the strhashval in nfs4_lockowner for searching lockownerlike before.
> >>
> >> Signed-off-by: Kinglong Mee <[email protected]>
> >> ---
> >> fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
> >> fs/nfsd/state.h | 1 +
> >> 2 files changed, 24 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 9b9474c..1b5afc1 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -4282,18 +4282,30 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> >> -};
> >> +static bool
> >> +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfsd_net *nn)
> >> +{
> >> + struct nfs4_stateowner *local = NULL;
> >> + unsigned hashval = lo->lo_hashval;
> >> +
> >> + if (hashval >= OWNER_HASH_SIZE)
> >> + return false;
> >> +
> >> + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> >> + if (local == &lo->lo_owner)
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >>
> >> static inline void
> >> -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> >> +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
> >> + struct nfsd_net *nn)
> >> {
> >> - struct nfs4_lockowner *lo;
> >> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> >>
> >> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> >> - lo = (struct nfs4_lockowner *) fl->fl_owner;
> >> + if (nfs4_verify_lock_stateowner(lo, nn)) {
> >> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> >> lo->lo_owner.so_owner.len, GFP_KERNEL);
> >> if (!deny->ld_owner.data)
> >> @@ -4350,6 +4362,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
> >> if (!lo)
> >> return NULL;
> >> INIT_LIST_HEAD(&lo->lo_owner.so_stateids);
> >> + lo->lo_hashval = strhashval;
> >> lo->lo_owner.so_is_open_owner = 0;
> >> /* It is the openowner seqid that will be incremented in encode in the
> >> * case of new lockowners; so increment the lock seqid manually: */
> >> @@ -4564,7 +4577,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >> file_lock->fl_pid = current->tgid;
> >> file_lock->fl_file = filp;
> >> file_lock->fl_flags = FL_POSIX;
> >> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> >> file_lock->fl_start = lock->lk_offset;
> >> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> >> nfs4_transform_lock_offset(file_lock);
> >> @@ -4587,7 +4599,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >> case (EAGAIN): /* conflock holds conflicting lock */
> >> status = nfserr_denied;
> >> dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> >> - nfs4_set_lock_denied(conflock, &lock->lk_denied);
> >> + nfs4_set_lock_denied(conflock, &lock->lk_denied, nn);
> >> break;
> >> case (EDEADLK):
> >> status = nfserr_deadlock;
> >> @@ -4695,7 +4707,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>
> >> if (file_lock->fl_type != F_UNLCK) {
> >> status = nfserr_denied;
> >> - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
> >> + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, nn);
> >> }
> >> out:
> >> nfs4_unlock_state();
> >> @@ -4746,7 +4758,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >> file_lock->fl_pid = current->tgid;
> >> file_lock->fl_file = filp;
> >> file_lock->fl_flags = FL_POSIX;
> >> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> >> file_lock->fl_start = locku->lu_offset;
> >>
> >> file_lock->fl_end = last_byte_offset(locku->lu_offset,
> >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> >> index 1848fed..9ecae58 100644
> >> --- a/fs/nfsd/state.h
> >> +++ b/fs/nfsd/state.h
> >> @@ -362,6 +362,7 @@ struct nfs4_openowner {
> >>
> >> struct nfs4_lockowner {
> >> struct nfs4_stateowner lo_owner; /* must be first element */
> >> + unsigned int lo_hashval;
> >
> > Actually, I have another comment here too. I'm not sure there's a lot
> > of value in tracking the lo_hashval here. Why not just recompute it in
> > nfs4_verify_lock_stateowner?
>
> No, ownerstr_hashval() needs a client which holds the conflicting lock,
> in nfsd4_lock/nfsd4_lockt cannot know the client from vfs_lock_file()
> or vfs_test_lock().
>
Right, that was why I mentioned that you'd need to pass down the
nfs4_client pointer to these functions, and then use that to derive the
"nn".
> >
> > You'd need to pass down a pointer to the nfs4_client in order to do
> > so, but you could do that instead of passing down 'nn' and then
> > just call net_generic on clp->net where you needed it.
>
> I find patch "[PATCH v3 069/114] nfsd: Move the open owner hash table
> into struct nfs4_client" have remove the client argument from
> ownerstr_hashval().
>
> So, the lo_hashval isn't needed after that patch.
> After Bruce merging that patch, I will resend a new patch recomputing
> the hash value instead recording in lo_hashval.
>
Ok. I guess the only question is whether this is a bugfix that needs to
go in before those patches. If so, then we may want to do something
closer to this patch.
It's untested, but it should be OK now that we cache the clp in the
cstate. If this needs to go into 3.16 then we'll need to get that
pointer elsewhere, but that's doable.
-------------------------[snip]------------------------------
[PATCH] NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"
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 for nfsd4_lock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
Signed-off-by: Kinglong Mee <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c400ec17915e..00cd7fd17b0f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4312,18 +4312,29 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
-};
+static bool
+nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfs4_client *clp)
+{
+ struct nfs4_stateowner *local = NULL;
+ struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+ unsigned int hashval = ownerstr_hashval(clp->cl_clientid.cl_id,
+ &lo->lo_owner.so_owner);
+
+ list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
+ if (local == &lo->lo_owner)
+ return true;
+ }
+
+ return false;
+}
static inline void
-nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
+nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
+ struct nfs4_client *clp)
{
- struct nfs4_lockowner *lo;
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner;
- if (fl->fl_lmops == &nfsd_posix_mng_ops) {
- lo = (struct nfs4_lockowner *) fl->fl_owner;
+ if (nfs4_verify_lock_stateowner(lo, clp)) {
deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
lo->lo_owner.so_owner.len, GFP_KERNEL);
if (!deny->ld_owner.data)
@@ -4594,7 +4605,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
file_lock->fl_start = lock->lk_offset;
file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
nfs4_transform_lock_offset(file_lock);
@@ -4617,7 +4627,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
case (EAGAIN): /* conflock holds conflicting lock */
status = nfserr_denied;
dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
- nfs4_set_lock_denied(conflock, &lock->lk_denied);
+ nfs4_set_lock_denied(conflock, &lock->lk_denied,
+ cstate->clp);
break;
case (EDEADLK):
status = nfserr_deadlock;
@@ -4725,7 +4736,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (file_lock->fl_type != F_UNLCK) {
status = nfserr_denied;
- nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
+ nfs4_set_lock_denied(file_lock, &lockt->lt_denied, cstate->clp);
}
out:
nfs4_unlock_state();
@@ -4776,7 +4787,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
file_lock->fl_start = locku->lu_offset;
file_lock->fl_end = last_byte_offset(locku->lu_offset,
--
1.9.3
On Tue, 08 Jul 2014 20:26:03 +0800
Kinglong Mee <[email protected]> wrote:
> On 7/8/2014 19:03, Jeff Layton wrote:
> > On Tue, 08 Jul 2014 11:23:49 +0800
> > Kinglong Mee <[email protected]> wrote:
> >
> >> On 7/8/2014 01:26, Jeff Layton wrote:
> >>> On Mon, 07 Jul 2014 22:12:21 +0800
> >>> Kinglong Mee <[email protected]> wrote:
> >>>
> > -------------------------[snip]------------------------------
> >
> > [PATCH] NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"
> >
> > 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 for nfsd4_lock always be NULL.
> >
> > Also, commit 0996905f93 (lockd: posix_test_lock() should not call locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt always be NULL too.
> >
> > So that, using fl_lmops cannot checking nfsd4 lockowner for it always be NULL.
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
> > 1 file changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index c400ec17915e..00cd7fd17b0f 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4312,18 +4312,29 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> > -};
> > +static bool
> > +nfs4_verify_lock_stateowner(struct nfs4_lockowner *lo, struct nfs4_client *clp)
> > +{
> > + struct nfs4_stateowner *local = NULL;
> > + struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> > + unsigned int hashval = ownerstr_hashval(clp->cl_clientid.cl_id,
> > + &lo->lo_owner.so_owner);
>
> Needs the conflicting client which holds the lock,
> Not the new client which try to locking the file has be locked.
>
> > +
> > + list_for_each_entry(local, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> > + if (local == &lo->lo_owner)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> >
> > static inline void
> > -nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> > +nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny,
> > + struct nfs4_client *clp)
> > {
> > - struct nfs4_lockowner *lo;
> > + struct nfs4_lockowner *lo = (struct nfs4_lockowner *)fl->fl_owner;
> >
> > - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> > - lo = (struct nfs4_lockowner *) fl->fl_owner;
> > + if (nfs4_verify_lock_stateowner(lo, clp)) {
> > deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> > lo->lo_owner.so_owner.len, GFP_KERNEL);
> > if (!deny->ld_owner.data)
> > @@ -4594,7 +4605,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > file_lock->fl_pid = current->tgid;
> > file_lock->fl_file = filp;
> > file_lock->fl_flags = FL_POSIX;
> > - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> > file_lock->fl_start = lock->lk_offset;
> > file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> > nfs4_transform_lock_offset(file_lock);
> > @@ -4617,7 +4627,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > case (EAGAIN): /* conflock holds conflicting lock */
> > status = nfserr_denied;
> > dprintk("NFSD: nfsd4_lock: conflicting lock found!\n");
> > - nfs4_set_lock_denied(conflock, &lock->lk_denied);
> > + nfs4_set_lock_denied(conflock, &lock->lk_denied,
> > + cstate->clp);
> > break;
> > case (EDEADLK):
> > status = nfserr_deadlock;
> > @@ -4725,7 +4736,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >
> > if (file_lock->fl_type != F_UNLCK) {
> > status = nfserr_denied;
> > - nfs4_set_lock_denied(file_lock, &lockt->lt_denied);
> > + nfs4_set_lock_denied(file_lock, &lockt->lt_denied, cstate->clp);
>
> The clp is the client which try to lock the file here,
> but the file have be locked by other client or server.
>
> With this patch, the hashval is wrong.
> *correct* *this patch*
> clid conflicting client new request lock client
> owner from vfs_lock_file() from vfs_lock_file()
> or vfs_test_lock() or vfs_test_lock()
> hashval right hashval bad hashval (never exist)
>
> With this patch, LOCK/LOCKT compound gets client with all zero, and owner with all zero.
>
> thanks,
> Kinglong Mee
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
<facepalm>
Ahh ok, I see what you're saying now. I'll leave it to you to respin
once the rest of the client_mutex removal series is merged.
</facepalm>
--
Jeff Layton <[email protected]>
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
v5: same as the first version
Reviewed-by: Jeff Layton <[email protected]>
Signed-off-by: Kinglong Mee <[email protected]>
---
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
On Tue, Aug 19, 2014 at 04:23:44PM -0400, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
> > ---
> > 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.
On 8/15/2014 19:14, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:07:12 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Kinglong Mee <[email protected]>
---
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
On Sat, Aug 02, 2014 at 07:05:05PM -0400, Jeff Layton wrote:
> On Sat, 02 Aug 2014 22:45:17 +0800
> Kinglong Mee <[email protected]> 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
> > for nfsd4_lock always be NULL.
> >
> > Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> > locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt
> > always be NULL too.
> >
> > So that, nfsd4 lockowner for it always be NULL too.
> >
> > This patch re-coping the fl_lmops to conflock.
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > ---
> > fs/locks.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 717fbc4..cc1219a 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -279,7 +279,7 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> > new->fl_start = fl->fl_start;
> > new->fl_end = fl->fl_end;
> > new->fl_ops = NULL;
> > - new->fl_lmops = NULL;
> > + new->fl_lmops = fl->fl_lmops;
> > }
> > EXPORT_SYMBOL(__locks_copy_lock);
> >
> > @@ -290,7 +290,6 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> > __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);
> > }
>
> This looks sane to me AFAICT.
>
> I'll run a few tests with it, and put it into I'll plan to pick this up
> since I have some other fs/locks.c related patches slated for 3.17.
Looks like your mail and Trond's crossed? I'd need to go remind myself
of the __locks_copy_lock callers, but I'm pretty sure Trond's right....
--b.
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 <[email protected]>
---
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
On 8/12/2014 00:19, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
>> ---
>> 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;
>
>
On Wed, 06 Aug 2014 21:35:40 +0800
Kinglong Mee <[email protected]> wrote:
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 24168ae..07e4b5c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4867,6 +4867,29 @@ 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_lock(struct file_lock *dst, struct file_lock *src)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> + get_lockowner(lo);
> +}
> +
> +static void nfsd4_fl_release_lock(struct file_lock *fl)
> +{
> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> + nfs4_put_stateowner(&lo->lo_owner);
> +}
> +
> +static const struct file_lock_operations nfsd4_fl_lock_ops = {
> + .fl_copy_lock = nfsd4_fl_copy_lock,
> + .fl_release_private = nfsd4_fl_release_lock,
> +};
> +
> static inline void
> nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> {
> @@ -5233,10 +5256,12 @@ 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 | FL_NFSD;
> + file_lock->fl_ops = &nfsd4_fl_lock_ops;
> file_lock->fl_start = lock->lk_offset;
> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> nfs4_transform_lock_offset(file_lock);
> @@ -5399,6 +5424,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);
> @@ -5420,6 +5446,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__);
> @@ -5428,10 +5456,11 @@ 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 | FL_NFSD;
> + file_lock->fl_ops = &nfsd4_fl_lock_ops;
> file_lock->fl_start = locku->lu_offset;
>
> file_lock->fl_end = last_byte_offset(locku->lu_offset,
(Sorry I didn't respond to these before -- I no longer work for Red
Hat, so emails to [email protected] don't get to me).
This looks really wrong. In this case, knfsd is acting as a
(server-side) lock manager, not a filesystem:
const struct file_lock_operations *fl_ops; /* Callbacks for filesystems */
const struct lock_manager_operations *fl_lmops; /* Callbacks for lockmanagers */
So really, taking and putting of lockowner references is really the
purview of the lockmanager. If you had a filesystem that set fl_ops and
was exportable, then this will likely break it.
The fact that the file locking code uses struct file_lock for lock
requests and for tracking the locks themselves makes for a horribly
confusing interface. If I were designing this from scratch I would have
made those two distinct structures. Maybe we should do that anyway
sometime.
What you probably ought want to do is to declare some new fl_lmops for
taking and putting lockowner references and add hooks to call them at
the appropriate places.
--
Jeff Layton <[email protected]>
On Sun, 10 Aug 2014 23:38:25 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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
for nfsd4_lock always be NULL.
Also, commit 0996905f93 (lockd: posix_test_lock() should not call
locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt
always be NULL too.
So that, nfsd4 lockowner for it always be NULL too.
This patch re-coping the fl_lmops to conflock.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/locks.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index 717fbc4..cc1219a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -279,7 +279,7 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
new->fl_start = fl->fl_start;
new->fl_end = fl->fl_end;
new->fl_ops = NULL;
- new->fl_lmops = NULL;
+ new->fl_lmops = fl->fl_lmops;
}
EXPORT_SYMBOL(__locks_copy_lock);
@@ -290,7 +290,6 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
__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);
}
--
1.9.3
On Thu, 14 Aug 2014 20:26:03 +0800
Kinglong Mee <[email protected]> wrote:
> On 8/12/2014 00:19, Jeff Layton wrote:
> > On Sun, 10 Aug 2014 23:38:25 +0800
> > Kinglong Mee <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
On 8/20/2014 04:23, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
>> ---
>> 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
v2: Fix bad using of struct file_lock_operations for handle the owner.
Signed-off-by: Kinglong Mee <[email protected]>
---
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
On Sun, 17 Aug 2014 21:42:21 +0800
Kinglong Mee <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
On Mon, 2014-08-11 at 12:19 -0400, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
---
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
On 8/9/2014 19:08, Jeff Layton wrote:
> On Wed, 06 Aug 2014 21:35:40 +0800
> Kinglong Mee <[email protected]> wrote:
>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 33 +++++++++++++++++++++++++++++++--
>> 1 file changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 24168ae..07e4b5c 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4867,6 +4867,29 @@ 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_lock(struct file_lock *dst, struct file_lock *src)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
>> + get_lockowner(lo);
>> +}
>> +
>> +static void nfsd4_fl_release_lock(struct file_lock *fl)
>> +{
>> + struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
>> + nfs4_put_stateowner(&lo->lo_owner);
>> +}
>> +
>> +static const struct file_lock_operations nfsd4_fl_lock_ops = {
>> + .fl_copy_lock = nfsd4_fl_copy_lock,
>> + .fl_release_private = nfsd4_fl_release_lock,
>> +};
>> +
>> static inline void
>> nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>> {
>> @@ -5233,10 +5256,12 @@ 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 | FL_NFSD;
>> + file_lock->fl_ops = &nfsd4_fl_lock_ops;
>> file_lock->fl_start = lock->lk_offset;
>> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
>> nfs4_transform_lock_offset(file_lock);
>> @@ -5399,6 +5424,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);
>> @@ -5420,6 +5446,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__);
>> @@ -5428,10 +5456,11 @@ 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 | FL_NFSD;
>> + file_lock->fl_ops = &nfsd4_fl_lock_ops;
>> file_lock->fl_start = locku->lu_offset;
>>
>> file_lock->fl_end = last_byte_offset(locku->lu_offset,
>
> (Sorry I didn't respond to these before -- I no longer work for Red
> Hat, so emails to [email protected] don't get to me).
>
> This looks really wrong. In this case, knfsd is acting as a
> (server-side) lock manager, not a filesystem:
>
> const struct file_lock_operations *fl_ops; /* Callbacks for filesystems */
> const struct lock_manager_operations *fl_lmops; /* Callbacks for lockmanagers */
>
> So really, taking and putting of lockowner references is really the
> purview of the lockmanager. If you had a filesystem that set fl_ops and
> was exportable, then this will likely break it.
>
> The fact that the file locking code uses struct file_lock for lock
> requests and for tracking the locks themselves makes for a horribly
> confusing interface. If I were designing this from scratch I would have
> made those two distinct structures. Maybe we should do that anyway
> sometime.
>
> What you probably ought want to do is to declare some new fl_lmops for
> taking and putting lockowner references and add hooks to call them at
> the appropriate places.
Sorry for my fault, a new resolve as v2 has be send.
Please help have a check, thank you very much.
thanks,
Kinglong Mee
On Sat, Aug 2, 2014 at 10:45 AM, Kinglong Mee <[email protected]> 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
> for nfsd4_lock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt
> always be NULL too.
>
> So that, nfsd4 lockowner for it always be NULL too.
>
> This patch re-coping the fl_lmops to conflock.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/locks.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 717fbc4..cc1219a 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -279,7 +279,7 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> new->fl_start = fl->fl_start;
> new->fl_end = fl->fl_end;
> new->fl_ops = NULL;
> - new->fl_lmops = NULL;
> + new->fl_lmops = fl->fl_lmops;
> }
> EXPORT_SYMBOL(__locks_copy_lock);
>
> @@ -290,7 +290,6 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> __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);
> }
> --
No. There is a very good reason why we separate __locks_copy_lock and
locks_copy_private(). We don't want to have anyone calling notifiers
etc for copies of locks.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Tue, 19 Aug 2014 23:18:58 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
On Tue, 19 Aug 2014 23:25:32 +0800
Kinglong Mee <[email protected]> wrote:
> v4: same as v3
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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);
}
-
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;
--
1.9.3
On Fri, 15 Aug 2014 08:07:12 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Kinglong Mee <[email protected]>
---
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
On 8/14/2014 22:00, Jeff Layton wrote:
> On Thu, 14 Aug 2014 20:26:03 +0800
> Kinglong Mee <[email protected]> wrote:
>
>> On 8/12/2014 00:19, Jeff Layton wrote:
>>> On Sun, 10 Aug 2014 23:38:25 +0800
>>> Kinglong Mee <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
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 <[email protected]>
---
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
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 24168ae..07e4b5c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4867,6 +4867,29 @@ 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_lock(struct file_lock *dst, struct file_lock *src)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
+ get_lockowner(lo);
+}
+
+static void nfsd4_fl_release_lock(struct file_lock *fl)
+{
+ struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
+ nfs4_put_stateowner(&lo->lo_owner);
+}
+
+static const struct file_lock_operations nfsd4_fl_lock_ops = {
+ .fl_copy_lock = nfsd4_fl_copy_lock,
+ .fl_release_private = nfsd4_fl_release_lock,
+};
+
static inline void
nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
{
@@ -5233,10 +5256,12 @@ 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 | FL_NFSD;
+ file_lock->fl_ops = &nfsd4_fl_lock_ops;
file_lock->fl_start = lock->lk_offset;
file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
nfs4_transform_lock_offset(file_lock);
@@ -5399,6 +5424,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);
@@ -5420,6 +5446,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__);
@@ -5428,10 +5456,11 @@ 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 | FL_NFSD;
+ file_lock->fl_ops = &nfsd4_fl_lock_ops;
file_lock->fl_start = locku->lu_offset;
file_lock->fl_end = last_byte_offset(locku->lu_offset,
--
1.9.3
v3: new patch isn't exist in v2/v1.
Signed-off-by: Kinglong Mee <[email protected]>
---
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
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 <[email protected]>
---
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
On Sun, 10 Aug 2014 23:43:57 +0800
Kinglong Mee <[email protected]> wrote:
> v2: Fix bad using of struct file_lock_operations for handle the owner.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> 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 <[email protected]>
On 8/12/2014 00:46, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:43:57 +0800
> Kinglong Mee <[email protected]> wrote:
>
>> v2: Fix bad using of struct file_lock_operations for handle the owner.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> 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;
>
>
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
On Tue, 19 Aug 2014 23:24:23 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
Make sure copy the private information by fl_copy_lock()
in struct file_lock_operations, use locks_copy_lock copying
all information for conflock.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/lockd/svclock.c | 2 +-
fs/locks.c | 27 ++++++---------------------
include/linux/fs.h | 6 ------
3 files changed, 7 insertions(+), 28 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..5547b3f 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -266,35 +266,20 @@ 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);
}
-
EXPORT_SYMBOL(locks_copy_lock);
static inline int flock_translate_cmd(int cmd) {
@@ -718,7 +703,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 +906,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 4d40097..0d2c491 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -942,7 +942,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 *);
@@ -1002,11 +1001,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
On Sat, 02 Aug 2014 22:45:17 +0800
Kinglong Mee <[email protected]> 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
> for nfsd4_lock always be NULL.
>
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt
> always be NULL too.
>
> So that, nfsd4 lockowner for it always be NULL too.
>
> This patch re-coping the fl_lmops to conflock.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/locks.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 717fbc4..cc1219a 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -279,7 +279,7 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> new->fl_start = fl->fl_start;
> new->fl_end = fl->fl_end;
> new->fl_ops = NULL;
> - new->fl_lmops = NULL;
> + new->fl_lmops = fl->fl_lmops;
> }
> EXPORT_SYMBOL(__locks_copy_lock);
>
> @@ -290,7 +290,6 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> __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);
> }
This looks sane to me AFAICT.
I'll run a few tests with it, and put it into I'll plan to pick this up
since I have some other fs/locks.c related patches slated for 3.17.
Thanks!
--
Jeff Layton <[email protected]>
On 8/18/2014 19:54, Jeff Layton wrote:
> On Sun, 17 Aug 2014 21:42:21 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
On Tue, 19 Aug 2014 23:26:45 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
v4: same as v3
Signed-off-by: Kinglong Mee <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, 19 Aug 2014 23:21:19 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On 8/9/2014 18:51, Jeff Layton wrote:
> On Wed, 06 Aug 2014 21:33:25 +0800
> Kinglong Mee <[email protected]> wrote:
>
>> Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 15 ++++-----------
>> include/linux/fs.h | 1 +
>> 2 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 2e80a59..24168ae 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4867,17 +4867,12 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
>> -};
>> -
>> static inline void
>> nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
>> {
>> struct nfs4_lockowner *lo;
>>
>> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
>> + if (fl->fl_flags & FL_NFSD) {
>> lo = (struct nfs4_lockowner *) fl->fl_owner;
>> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
>> lo->lo_owner.so_owner.len, GFP_KERNEL);
>> @@ -5241,8 +5236,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_owner = (fl_owner_t)lock_sop;
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> - file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> + file_lock->fl_flags = FL_POSIX | FL_NFSD;
>> file_lock->fl_start = lock->lk_offset;
>> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
>> nfs4_transform_lock_offset(file_lock);
>> @@ -5375,7 +5369,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_NFSD;
>>
>> file_lock->fl_start = lockt->lt_offset;
>> file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
>> @@ -5437,8 +5431,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
>> file_lock->fl_pid = current->tgid;
>> file_lock->fl_file = filp;
>> - file_lock->fl_flags = FL_POSIX;
>> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
>> + file_lock->fl_flags = FL_POSIX | FL_NFSD;
>> file_lock->fl_start = locku->lu_offset;
>>
>> file_lock->fl_end = last_byte_offset(locku->lu_offset,
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e11d60c..4d40097 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -819,6 +819,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_NFSD 2048 /* NFSD holds this lock */
>>
>> /*
>> * Special return value from posix_lock_file() and vfs_lock_file() for
>
> Honestly, I'd prefer not using a fl_flag for this. Why, might you ask?
>
> Currently, there are two alternate lock managers in the kernel (nfsd
> and lockd) but there could (in principle) be an arbitrary number of
> them later. What's so special about knfsd that it warrants its own flag?
>
> I think existing use of an empty lock_manager_operations is a cleaner
> solution than this for identifying locks owned by knfsd.
Thanks for your explication.
I will drop this patch in v2.
thanks,
Kinglong Mee
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 <[email protected]>
---
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
On Fri, 15 Aug 2014 08:02:55 +0800
Kinglong Mee <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
On Tue, 5 Aug 2014 15:14:58 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Sat, Aug 02, 2014 at 07:05:05PM -0400, Jeff Layton wrote:
> > On Sat, 02 Aug 2014 22:45:17 +0800
> > Kinglong Mee <[email protected]> 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
> > > for nfsd4_lock always be NULL.
> > >
> > > Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> > > locks_copy_lock()) caused the fl_lmops of conflock for nfsd4_lockt
> > > always be NULL too.
> > >
> > > So that, nfsd4 lockowner for it always be NULL too.
> > >
> > > This patch re-coping the fl_lmops to conflock.
> > >
> > > Signed-off-by: Kinglong Mee <[email protected]>
> > > ---
> > > fs/locks.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 717fbc4..cc1219a 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -279,7 +279,7 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> > > new->fl_start = fl->fl_start;
> > > new->fl_end = fl->fl_end;
> > > new->fl_ops = NULL;
> > > - new->fl_lmops = NULL;
> > > + new->fl_lmops = fl->fl_lmops;
> > > }
> > > EXPORT_SYMBOL(__locks_copy_lock);
> > >
> > > @@ -290,7 +290,6 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> > > __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);
> > > }
> >
> > This looks sane to me AFAICT.
> >
> > I'll run a few tests with it, and put it into I'll plan to pick this up
> > since I have some other fs/locks.c related patches slated for 3.17.
>
> Looks like your mail and Trond's crossed? I'd need to go remind myself
> of the __locks_copy_lock callers, but I'm pretty sure Trond's right....
>
> --b.
Yeah, I missed his earlier reply and have since dropped this patch. The
potential race that Trond pointed out trumps the minor problem of not
having conflock info, so I'm inclined to wait for a better solution to
that problem.
--
Jeff Layton <[email protected]>
On Wed, 06 Aug 2014 21:33:25 +0800
Kinglong Mee <[email protected]> wrote:
> Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 15 ++++-----------
> include/linux/fs.h | 1 +
> 2 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e80a59..24168ae 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4867,17 +4867,12 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
> -};
> -
> static inline void
> nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
> {
> struct nfs4_lockowner *lo;
>
> - if (fl->fl_lmops == &nfsd_posix_mng_ops) {
> + if (fl->fl_flags & FL_NFSD) {
> lo = (struct nfs4_lockowner *) fl->fl_owner;
> deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
> lo->lo_owner.so_owner.len, GFP_KERNEL);
> @@ -5241,8 +5236,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_owner = (fl_owner_t)lock_sop;
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> - file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> + file_lock->fl_flags = FL_POSIX | FL_NFSD;
> file_lock->fl_start = lock->lk_offset;
> file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> nfs4_transform_lock_offset(file_lock);
> @@ -5375,7 +5369,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_NFSD;
>
> file_lock->fl_start = lockt->lt_offset;
> file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> @@ -5437,8 +5431,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> file_lock->fl_pid = current->tgid;
> file_lock->fl_file = filp;
> - file_lock->fl_flags = FL_POSIX;
> - file_lock->fl_lmops = &nfsd_posix_mng_ops;
> + file_lock->fl_flags = FL_POSIX | FL_NFSD;
> file_lock->fl_start = locku->lu_offset;
>
> file_lock->fl_end = last_byte_offset(locku->lu_offset,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e11d60c..4d40097 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -819,6 +819,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_NFSD 2048 /* NFSD holds this lock */
>
> /*
> * Special return value from posix_lock_file() and vfs_lock_file() for
Honestly, I'd prefer not using a fl_flag for this. Why, might you ask?
Currently, there are two alternate lock managers in the kernel (nfsd
and lockd) but there could (in principle) be an arbitrary number of
them later. What's so special about knfsd that it warrants its own flag?
I think existing use of an empty lock_manager_operations is a cleaner
solution than this for identifying locks owned by knfsd.
--
Jeff Layton <[email protected]>
On Fri, Aug 15, 2014 at 06:57:41AM -0400, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:02:55 +0800
> Kinglong Mee <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Yep, got it, thanks!
--b.
Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.
Signed-off-by: Kinglong Mee <[email protected]>
---
fs/nfsd/nfs4state.c | 15 ++++-----------
include/linux/fs.h | 1 +
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..24168ae 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4867,17 +4867,12 @@ 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 const struct lock_manager_operations nfsd_posix_mng_ops = {
-};
-
static inline void
nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny)
{
struct nfs4_lockowner *lo;
- if (fl->fl_lmops == &nfsd_posix_mng_ops) {
+ if (fl->fl_flags & FL_NFSD) {
lo = (struct nfs4_lockowner *) fl->fl_owner;
deny->ld_owner.data = kmemdup(lo->lo_owner.so_owner.data,
lo->lo_owner.so_owner.len, GFP_KERNEL);
@@ -5241,8 +5236,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_owner = (fl_owner_t)lock_sop;
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
- file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
+ file_lock->fl_flags = FL_POSIX | FL_NFSD;
file_lock->fl_start = lock->lk_offset;
file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
nfs4_transform_lock_offset(file_lock);
@@ -5375,7 +5369,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_NFSD;
file_lock->fl_start = lockt->lt_offset;
file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
@@ -5437,8 +5431,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
file_lock->fl_pid = current->tgid;
file_lock->fl_file = filp;
- file_lock->fl_flags = FL_POSIX;
- file_lock->fl_lmops = &nfsd_posix_mng_ops;
+ file_lock->fl_flags = FL_POSIX | FL_NFSD;
file_lock->fl_start = locku->lu_offset;
file_lock->fl_end = last_byte_offset(locku->lu_offset,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e11d60c..4d40097 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -819,6 +819,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_NFSD 2048 /* NFSD holds this lock */
/*
* Special return value from posix_lock_file() and vfs_lock_file() for
--
1.9.3
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 <[email protected]>
Reviewed-by: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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