[PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[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
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 <kinglongmee@gmail.com>
---
 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

Re: [PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[Trond Myklebust]

On Sat, Aug 2, 2014 at 10:45 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks)
> using fl_lmops field in file_lock for checking nfsd4 lockowner.
>
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods
> on return of conflicting locks) causes the fl_lmops of conflock
> 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 <kinglongmee@gmail.com>
> ---
>  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

trond.myklebust@primarydata.com

Re: [PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[Jeff Layton]

On Sat, 02 Aug 2014 22:45:17 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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 <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[J. Bruce Fields]

On Sat, Aug 02, 2014 at 07:05:05PM -0400, Jeff Layton wrote:
> On Sat, 02 Aug 2014 22:45:17 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs/locks.c: Copy fl_lmops to conflock for nfsd using

[Jeff Layton]

On Tue, 5 Aug 2014 15:14:58 -0400
"J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> 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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  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 <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] NFSD: New FL_NFSD for marking file_lock belongs to NFSD

[Kinglong Mee]

Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3 RFC] fs/locks.c: Copy all infomation for conflock

[Kinglong Mee]

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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] NFSD: Increase the reference of lockowner when coping file_lock

[Jeff Layton]

On Wed, 06 Aug 2014 21:35:40 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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 jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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 <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

On 8/9/2014 19:08, Jeff Layton wrote:
> On Wed, 06 Aug 2014 21:35:40 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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 jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] NFSD: New FL_NFSD for marking file_lock belongs to NFSD

[Jeff Layton]

On Wed, 06 Aug 2014 21:33:25 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  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 <jlayton@primarydata.com>

Re: [PATCH 1/3] NFSD: New FL_NFSD for marking file_lock belongs to NFSD

[Kinglong Mee]

On 8/9/2014 18:51, Jeff Layton wrote:
> On Wed, 06 Aug 2014 21:33:25 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Using fl_flags instead of fl_lmops for marking file_lock belongs to NFSD.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[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.

v2: Only change the order from 3/3 to 1/3 now.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3 v2] fs/locks.c: New ops in file_lock_operations for copying/releasing owner

[Kinglong Mee]

NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.

v2: A new patch isn't exist in v1.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

v2: Fix bad using of struct file_lock_operations for handle the owner.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock

[Jeff Layton]

On Sun, 10 Aug 2014 23:43:57 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> v2: Fix bad using of struct file_lock_operations for handle the owner.
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e80a59..24a8d91 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4867,9 +4867,33 @@ nfs4_transform_lock_offset(struct file_lock *lock)
>  		lock->fl_end = OFFSET_MAX;
>  }
>  
> +static inline struct nfs4_lockowner *get_lockowner(struct nfs4_lockowner *lo)
> +{
> +	atomic_inc(&lo->lo_owner.so_count);
> +	return lo;
> +}
> +

The other places that increment the so_count on a lockowner should
probably be changed to use get_lockowner.

Or what may be better is to make a nfs4_get_stateowner call and change
all of the places that bump the so_count to use it. Then you could just
do something like this in the right spot below:

    file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(lock_sop));

...
> +static void nfsd4_fl_copy_owner(struct file_lock *dst, struct file_lock *src)
> +{
> +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> +	dst->fl_owner = (fl_owner_t) get_lockowner(lo);
> +}
> +
> +static void nfsd4_fl_release_owner(struct file_lock *fl)
> +{
> +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> +
> +	if (lo) {
> +		nfs4_put_stateowner(&lo->lo_owner);
> +		fl->fl_owner = NULL;
> +	}
> +}
> +
>  /* Hack!: For now, we're defining this just so we can use a pointer to it
>   * as a unique cookie to identify our (NFSv4's) posix locks. */

The above comment should be removed now.

>  static const struct lock_manager_operations nfsd_posix_mng_ops  = {
> +	.lm_copy_owner = nfsd4_fl_copy_owner,
> +	.lm_release_owner = nfsd4_fl_release_owner,
>  };
>  
>  static inline void
> @@ -5238,7 +5262,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_openmode;
>  		goto out;
>  	}
> -	file_lock->fl_owner = (fl_owner_t)lock_sop;
> +
> +	file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
>  	file_lock->fl_pid = current->tgid;
>  	file_lock->fl_file = filp;
>  	file_lock->fl_flags = FL_POSIX;
> @@ -5405,6 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_ol_stateid *stp;
>  	struct file *filp = NULL;
>  	struct file_lock *file_lock = NULL;
> +	struct nfs4_lockowner *lock_sop = NULL;
>  	__be32 status;
>  	int err;
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> @@ -5426,6 +5452,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_lock_range;
>  		goto put_stateid;
>  	}
> +
> +	lock_sop = lockowner(stp->st_stateowner);
>  	file_lock = locks_alloc_lock();
>  	if (!file_lock) {
>  		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> @@ -5434,7 +5462,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  	locks_init_lock(file_lock);
>  	file_lock->fl_type = F_UNLCK;
> -	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> +	file_lock->fl_owner = (fl_owner_t)get_lockowner(lock_sop);
>  	file_lock->fl_pid = current->tgid;
>  	file_lock->fl_file = filp;
>  	file_lock->fl_flags = FL_POSIX;


-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 v2] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

On 8/12/2014 00:46, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:43:57 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> v2: Fix bad using of struct file_lock_operations for handle the owner.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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;
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[Jeff Layton]

On Sun, 10 Aug 2014 23:38:25 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
> 
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> 
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> 
> v2: Only change the order from 3/3 to 1/3 now.
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/lockd/svclock.c |  2 +-
>  fs/locks.c         | 25 ++++++-------------------
>  include/linux/fs.h |  6 ------
>  3 files changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index ab798a8..e1f209c 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
>  		block->b_flags |= B_TIMED_OUT;
>  	if (conf) {
>  		if (block->b_fl)
> -			__locks_copy_lock(block->b_fl, conf);
> +			locks_copy_lock(block->b_fl, conf);
>  	}
>  }
>  
> diff --git a/fs/locks.c b/fs/locks.c
> index 717fbc4..91b0f03 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  		new->fl_lmops = fl->fl_lmops;
>  }
>  
> -/*
> - * Initialize a new lock from an existing file_lock structure.
> - */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  {
> +	locks_release_private(new);
> +
>  	new->fl_owner = fl->fl_owner;
>  	new->fl_pid = fl->fl_pid;
> -	new->fl_file = NULL;
> +	new->fl_file = fl->fl_file;
>  	new->fl_flags = fl->fl_flags;
>  	new->fl_type = fl->fl_type;
>  	new->fl_start = fl->fl_start;
>  	new->fl_end = fl->fl_end;
>  	new->fl_ops = NULL;
>  	new->fl_lmops = NULL;
> -}
> -EXPORT_SYMBOL(__locks_copy_lock);
> -
> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> -	locks_release_private(new);
> -
> -	__locks_copy_lock(new, fl);
> -	new->fl_file = fl->fl_file;
> -	new->fl_ops = fl->fl_ops;
> -	new->fl_lmops = fl->fl_lmops;
>  
>  	locks_copy_private(new, fl);
>  }

(cc'ing Joe Perches)

Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
is that you now need to ensure that any conflock structures are
properly initialized before passing them to locks_copy_lock.

The nfsv4 server code currently doesn't do that and it will need to be
fixed to do so or that will be a regression.

For the NLM code, Joe Perches has proposed a patch to remove the
conflock parameter from lm_grant since the callers always pass in NULL
anyway. You may want to pull in his patch and rebase yours on top of it
since it'll remove that __locks_copy_lock call altogether.

Joe, is Andrew merging that patch or do I need to pull it into the
locks tree?

> -
>  EXPORT_SYMBOL(locks_copy_lock);
>  
>  static inline int flock_translate_cmd(int cmd) {
> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  			break;
>  	}
>  	if (cfl) {
> -		__locks_copy_lock(fl, cfl);
> +		locks_copy_lock(fl, cfl);
>  		if (cfl->fl_nspid)
>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
>  	} else
> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>  			if (!posix_locks_conflict(request, fl))
>  				continue;
>  			if (conflock)
> -				__locks_copy_lock(conflock, fl);
> +				locks_copy_lock(conflock, fl);
>  			error = -EAGAIN;
>  			if (!(request->fl_flags & FL_SLEEP))
>  				goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e11d60c..ced023d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
>  extern void locks_init_lock(struct file_lock *);
>  extern struct file_lock * locks_alloc_lock(void);
>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>  extern void locks_remove_posix(struct file *, fl_owner_t);
>  extern void locks_remove_file(struct file *);
>  extern void locks_release_private(struct file_lock *);
> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>  	return;
>  }
>  
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> -	return;
> -}
> -
>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  {
>  	return;


-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[Joe Perches]

On Mon, 2014-08-11 at 12:19 -0400, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> > fl_lmops field in file_lock for checking nfsd4 lockowner.
> > 
> > But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> > of conflicting locks) causes the fl_lmops of conflock always be NULL.
> > 
> > Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> > locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> > 
> > v2: Only change the order from 3/3 to 1/3 now.
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  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.


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[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 <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>
>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>
>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>
>>> v2: Only change the order from 3/3 to 1/3 now.
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[Kinglong Mee]

On 8/12/2014 00:19, Jeff Layton wrote:
> On Sun, 10 Aug 2014 23:38:25 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>
>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>
>> v2: Only change the order from 3/3 to 1/3 now.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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;
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[Jeff Layton]

On Thu, 14 Aug 2014 20:26:03 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> On 8/12/2014 00:19, Jeff Layton wrote:
> > On Sun, 10 Aug 2014 23:38:25 +0800
> > Kinglong Mee <kinglongmee@gmail.com> wrote:
> > 
> >> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> >> fl_lmops field in file_lock for checking nfsd4 lockowner.
> >>
> >> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> >> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> >>
> >> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> >> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> >>
> >> v2: Only change the order from 3/3 to 1/3 now.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/lockd/svclock.c |  2 +-
> >>  fs/locks.c         | 25 ++++++-------------------
> >>  include/linux/fs.h |  6 ------
> >>  3 files changed, 7 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> >> index ab798a8..e1f209c 100644
> >> --- a/fs/lockd/svclock.c
> >> +++ b/fs/lockd/svclock.c
> >> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
> >>  		block->b_flags |= B_TIMED_OUT;
> >>  	if (conf) {
> >>  		if (block->b_fl)
> >> -			__locks_copy_lock(block->b_fl, conf);
> >> +			locks_copy_lock(block->b_fl, conf);
> >>  	}
> >>  }
> >>  
> >> diff --git a/fs/locks.c b/fs/locks.c
> >> index 717fbc4..91b0f03 100644
> >> --- a/fs/locks.c
> >> +++ b/fs/locks.c
> >> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> >>  		new->fl_lmops = fl->fl_lmops;
> >>  }
> >>  
> >> -/*
> >> - * Initialize a new lock from an existing file_lock structure.
> >> - */
> >> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> >> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>  {
> >> +	locks_release_private(new);
> >> +
> >>  	new->fl_owner = fl->fl_owner;
> >>  	new->fl_pid = fl->fl_pid;
> >> -	new->fl_file = NULL;
> >> +	new->fl_file = fl->fl_file;
> >>  	new->fl_flags = fl->fl_flags;
> >>  	new->fl_type = fl->fl_type;
> >>  	new->fl_start = fl->fl_start;
> >>  	new->fl_end = fl->fl_end;
> >>  	new->fl_ops = NULL;
> >>  	new->fl_lmops = NULL;
> >> -}
> >> -EXPORT_SYMBOL(__locks_copy_lock);
> >> -
> >> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> -{
> >> -	locks_release_private(new);
> >> -
> >> -	__locks_copy_lock(new, fl);
> >> -	new->fl_file = fl->fl_file;
> >> -	new->fl_ops = fl->fl_ops;
> >> -	new->fl_lmops = fl->fl_lmops;
> >>  
> >>  	locks_copy_private(new, fl);
> >>  }
> > 
> > (cc'ing Joe Perches)
> > 
> > Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
> > is that you now need to ensure that any conflock structures are
> > properly initialized before passing them to locks_copy_lock.
> > 
> > The nfsv4 server code currently doesn't do that and it will need to be
> > fixed to do so or that will be a regression.
> 
> I don't think so.
> locks_alloc_lock() has initialize the file_lock struct,
> the same as locks_init_lock().
> 
> I will clean the duplicate initialize for file_lock in nfs4state.c in v3.
> 

Ahh, you're correct. Yes, please just remove that instead. You might
also want to look for other places in the kernel that call
locks_init_lock unnecessarily. We might as well get rid of all of
them while we're looking.

> > For the NLM code, Joe Perches has proposed a patch to remove the
> > conflock parameter from lm_grant since the callers always pass in NULL
> > anyway. You may want to pull in his patch and rebase yours on top of it
> > since it'll remove that __locks_copy_lock call altogether.
> > 
> > Joe, is Andrew merging that patch or do I need to pull it into the
> > locks tree?
> 
> I will update this patch based on that patch and your new patch for locks.c.
> 
> thanks,
> Kinglong Mee
> 

Thanks. I wiggled Joe's patch on top of my current set of locking
patches and will plan to merge it for v3.18 unless there are any
objections.

> > 
> >> -
> >>  EXPORT_SYMBOL(locks_copy_lock);
> >>  
> >>  static inline int flock_translate_cmd(int cmd) {
> >> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> >>  			break;
> >>  	}
> >>  	if (cfl) {
> >> -		__locks_copy_lock(fl, cfl);
> >> +		locks_copy_lock(fl, cfl);
> >>  		if (cfl->fl_nspid)
> >>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
> >>  	} else
> >> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> >>  			if (!posix_locks_conflict(request, fl))
> >>  				continue;
> >>  			if (conflock)
> >> -				__locks_copy_lock(conflock, fl);
> >> +				locks_copy_lock(conflock, fl);
> >>  			error = -EAGAIN;
> >>  			if (!(request->fl_flags & FL_SLEEP))
> >>  				goto out;
> >> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >> index e11d60c..ced023d 100644
> >> --- a/include/linux/fs.h
> >> +++ b/include/linux/fs.h
> >> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
> >>  extern void locks_init_lock(struct file_lock *);
> >>  extern struct file_lock * locks_alloc_lock(void);
> >>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> >> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> >>  extern void locks_remove_posix(struct file *, fl_owner_t);
> >>  extern void locks_remove_file(struct file *);
> >>  extern void locks_release_private(struct file_lock *);
> >> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> >>  	return;
> >>  }
> >>  
> >> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >> -{
> >> -	return;
> >> -}
> >> -
> >>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>  {
> >>  	return;
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH 1/3 v2] fs/locks.c: Copy all information for conflock

[Kinglong Mee]

On 8/14/2014 22:00, Jeff Layton wrote:
> On Thu, 14 Aug 2014 20:26:03 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> On 8/12/2014 00:19, Jeff Layton wrote:
>>> On Sun, 10 Aug 2014 23:38:25 +0800
>>> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>
>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>
>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>
>>>> v2: Only change the order from 3/3 to 1/3 now.
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  fs/lockd/svclock.c |  2 +-
>>>>  fs/locks.c         | 25 ++++++-------------------
>>>>  include/linux/fs.h |  6 ------
>>>>  3 files changed, 7 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>>>> index ab798a8..e1f209c 100644
>>>> --- a/fs/lockd/svclock.c
>>>> +++ b/fs/lockd/svclock.c
>>>> @@ -677,7 +677,7 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
>>>>  		block->b_flags |= B_TIMED_OUT;
>>>>  	if (conf) {
>>>>  		if (block->b_fl)
>>>> -			__locks_copy_lock(block->b_fl, conf);
>>>> +			locks_copy_lock(block->b_fl, conf);
>>>>  	}
>>>>  }
>>>>  
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index 717fbc4..91b0f03 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -266,35 +266,22 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>>  		new->fl_lmops = fl->fl_lmops;
>>>>  }
>>>>  
>>>> -/*
>>>> - * Initialize a new lock from an existing file_lock structure.
>>>> - */
>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>  {
>>>> +	locks_release_private(new);
>>>> +
>>>>  	new->fl_owner = fl->fl_owner;
>>>>  	new->fl_pid = fl->fl_pid;
>>>> -	new->fl_file = NULL;
>>>> +	new->fl_file = fl->fl_file;
>>>>  	new->fl_flags = fl->fl_flags;
>>>>  	new->fl_type = fl->fl_type;
>>>>  	new->fl_start = fl->fl_start;
>>>>  	new->fl_end = fl->fl_end;
>>>>  	new->fl_ops = NULL;
>>>>  	new->fl_lmops = NULL;
>>>> -}
>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>> -
>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> -	locks_release_private(new);
>>>> -
>>>> -	__locks_copy_lock(new, fl);
>>>> -	new->fl_file = fl->fl_file;
>>>> -	new->fl_ops = fl->fl_ops;
>>>> -	new->fl_lmops = fl->fl_lmops;
>>>>  
>>>>  	locks_copy_private(new, fl);
>>>>  }
>>>
>>> (cc'ing Joe Perches)
>>>
>>> Ok, so you're basically just reverting 1a747ee0cc11a19. The catch there
>>> is that you now need to ensure that any conflock structures are
>>> properly initialized before passing them to locks_copy_lock.
>>>
>>> The nfsv4 server code currently doesn't do that and it will need to be
>>> fixed to do so or that will be a regression.
>>
>> I don't think so.
>> locks_alloc_lock() has initialize the file_lock struct,
>> the same as locks_init_lock().
>>
>> I will clean the duplicate initialize for file_lock in nfs4state.c in v3.
>>
> 
> Ahh, you're correct. Yes, please just remove that instead. You might
> also want to look for other places in the kernel that call
> locks_init_lock unnecessarily. We might as well get rid of all of
> them while we're looking.

OK, I will review those codes where calling locks_init_lock().

> 
>>> For the NLM code, Joe Perches has proposed a patch to remove the
>>> conflock parameter from lm_grant since the callers always pass in NULL
>>> anyway. You may want to pull in his patch and rebase yours on top of it
>>> since it'll remove that __locks_copy_lock call altogether.
>>>
>>> Joe, is Andrew merging that patch or do I need to pull it into the
>>> locks tree?
>>
>> I will update this patch based on that patch and your new patch for locks.c.
>>
>> thanks,
>> Kinglong Mee
>>
> 
> Thanks. I wiggled Joe's patch on top of my current set of locking
> patches and will plan to merge it for v3.18 unless there are any
> objections.

I saw your patch, thank you very much.

thanks,
Kinglong Mee

> 
>>>
>>>> -
>>>>  EXPORT_SYMBOL(locks_copy_lock);
>>>>  
>>>>  static inline int flock_translate_cmd(int cmd) {
>>>> @@ -718,7 +705,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>>  			break;
>>>>  	}
>>>>  	if (cfl) {
>>>> -		__locks_copy_lock(fl, cfl);
>>>> +		locks_copy_lock(fl, cfl);
>>>>  		if (cfl->fl_nspid)
>>>>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>>  	} else
>>>> @@ -921,7 +908,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>>  			if (!posix_locks_conflict(request, fl))
>>>>  				continue;
>>>>  			if (conflock)
>>>> -				__locks_copy_lock(conflock, fl);
>>>> +				locks_copy_lock(conflock, fl);
>>>>  			error = -EAGAIN;
>>>>  			if (!(request->fl_flags & FL_SLEEP))
>>>>  				goto out;
>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>> index e11d60c..ced023d 100644
>>>> --- a/include/linux/fs.h
>>>> +++ b/include/linux/fs.h
>>>> @@ -941,7 +941,6 @@ void locks_free_lock(struct file_lock *fl);
>>>>  extern void locks_init_lock(struct file_lock *);
>>>>  extern struct file_lock * locks_alloc_lock(void);
>>>>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>>  extern void locks_remove_posix(struct file *, fl_owner_t);
>>>>  extern void locks_remove_file(struct file *);
>>>>  extern void locks_release_private(struct file_lock *);
>>>> @@ -1001,11 +1000,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>>  	return;
>>>>  }
>>>>  
>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>> -{
>>>> -	return;
>>>> -}
>>>> -
>>>>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>  {
>>>>  	return;
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock

[Kinglong Mee]

locks_alloc_lock() has initialize struct file_lock,
don't need re-initialize it by locks_init_lock().

v3: A new patch isn't exit in v2/v1.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock

[Jeff Layton]

On Fri, 15 Aug 2014 08:02:55 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> locks_alloc_lock() has initialize struct file_lock,
> don't need re-initialize it by locks_init_lock().
> 
> v3: A new patch isn't exit in v2/v1.
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/nfsd/nfs4state.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e80a59..98edf97 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
>  	fl = locks_alloc_lock();
>  	if (!fl)
>  		return NULL;
> -	locks_init_lock(fl);
>  	fl->fl_lmops = &nfsd_lease_mng_ops;
>  	fl->fl_flags = FL_DELEG;
>  	fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
> @@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  
>  	fp = lock_stp->st_stid.sc_file;
> -	locks_init_lock(file_lock);
>  	switch (lock->lk_type) {
>  		case NFS4_READ_LT:
>  		case NFS4_READW_LT:
> @@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_jukebox;
>  		goto out;
>  	}
> -	locks_init_lock(file_lock);
> +
>  	switch (lockt->lt_type) {
>  		case NFS4_READ_LT:
>  		case NFS4_READW_LT:
> @@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_jukebox;
>  		goto fput;
>  	}
> -	locks_init_lock(file_lock);
> +
>  	file_lock->fl_type = F_UNLCK;
>  	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
>  	file_lock->fl_pid = current->tgid;

Bruce, since this is an nfsd patch, do you mind picking this one up?

Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5 v3] NFSD: Remove duplicate initialization of file_lock

[J. Bruce Fields]

On Fri, Aug 15, 2014 at 06:57:41AM -0400, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:02:55 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > locks_alloc_lock() has initialize struct file_lock,
> > don't need re-initialize it by locks_init_lock().
> > 
> > v3: A new patch isn't exit in v2/v1.
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  fs/nfsd/nfs4state.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 2e80a59..98edf97 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
> >  	fl = locks_alloc_lock();
> >  	if (!fl)
> >  		return NULL;
> > -	locks_init_lock(fl);
> >  	fl->fl_lmops = &nfsd_lease_mng_ops;
> >  	fl->fl_flags = FL_DELEG;
> >  	fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
> > @@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	}
> >  
> >  	fp = lock_stp->st_stid.sc_file;
> > -	locks_init_lock(file_lock);
> >  	switch (lock->lk_type) {
> >  		case NFS4_READ_LT:
> >  		case NFS4_READW_LT:
> > @@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  		status = nfserr_jukebox;
> >  		goto out;
> >  	}
> > -	locks_init_lock(file_lock);
> > +
> >  	switch (lockt->lt_type) {
> >  		case NFS4_READ_LT:
> >  		case NFS4_READW_LT:
> > @@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  		status = nfserr_jukebox;
> >  		goto fput;
> >  	}
> > -	locks_init_lock(file_lock);
> > +
> >  	file_lock->fl_type = F_UNLCK;
> >  	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> >  	file_lock->fl_pid = current->tgid;
> 
> Bruce, since this is an nfsd patch, do you mind picking this one up?
> 
> Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>

Yep, got it, thanks!

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5 v3] locks: New ops in file_lock_operations for copy/release owner

[Kinglong Mee]

NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.

v3: same as v2, isn't exist in v1.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5 v3] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner.

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner

[Kinglong Mee]

NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.

v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/locks.c         | 12 ++++++++++--
 include/linux/fs.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index cb66fb0..08342e0 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
 			fl->fl_ops->fl_release_private(fl);
 		fl->fl_ops = NULL;
 	}
-	fl->fl_lmops = NULL;
 
+	if (fl->fl_lmops) {
+		if (fl->fl_lmops->lm_put_owner)
+			fl->fl_lmops->lm_put_owner(fl);
+		fl->fl_lmops = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(locks_release_private);
 
@@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
 			fl->fl_ops->fl_copy_lock(new, fl);
 		new->fl_ops = fl->fl_ops;
 	}
-	if (fl->fl_lmops)
+
+	if (fl->fl_lmops) {
+		if (fl->fl_lmops->lm_get_owner)
+			fl->fl_lmops->lm_get_owner(new, fl);
 		new->fl_lmops = fl->fl_lmops;
+	}
 }
 
 /*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 908af4f..6829340 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -868,6 +868,8 @@ struct file_lock_operations {
 struct lock_manager_operations {
 	int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
 	unsigned long (*lm_owner_key)(struct file_lock *);
+	void (*lm_get_owner)(struct file_lock *, struct file_lock *);
+	void (*lm_put_owner)(struct file_lock *);
 	void (*lm_notify)(struct file_lock *);	/* unblock callback */
 	int (*lm_grant)(struct file_lock *, int);
 	void (*lm_break)(struct file_lock *);
-- 
1.9.3

Re: [PATCH 2/6 v4] locks: New ops in file_lock_operations for get/put owner

[Jeff Layton]

On Tue, 19 Aug 2014 23:18:58 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> NFSD or other lockmanager may increase the owner's reference,
> so adds two new options for copying and releasing owner.
> 
> v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/locks.c         | 12 ++++++++++--
>  include/linux/fs.h |  2 ++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index cb66fb0..08342e0 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -230,8 +230,12 @@ void locks_release_private(struct file_lock *fl)
>  			fl->fl_ops->fl_release_private(fl);
>  		fl->fl_ops = NULL;
>  	}
> -	fl->fl_lmops = NULL;
>  
> +	if (fl->fl_lmops) {
> +		if (fl->fl_lmops->lm_put_owner)
> +			fl->fl_lmops->lm_put_owner(fl);
> +		fl->fl_lmops = NULL;
> +	}
>  }
>  EXPORT_SYMBOL_GPL(locks_release_private);
>  
> @@ -274,8 +278,12 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  			fl->fl_ops->fl_copy_lock(new, fl);
>  		new->fl_ops = fl->fl_ops;
>  	}
> -	if (fl->fl_lmops)
> +
> +	if (fl->fl_lmops) {
> +		if (fl->fl_lmops->lm_get_owner)
> +			fl->fl_lmops->lm_get_owner(new, fl);
>  		new->fl_lmops = fl->fl_lmops;
> +	}
>  }
>  
>  /*
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 908af4f..6829340 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -868,6 +868,8 @@ struct file_lock_operations {
>  struct lock_manager_operations {
>  	int (*lm_compare_owner)(struct file_lock *, struct file_lock *);
>  	unsigned long (*lm_owner_key)(struct file_lock *);
> +	void (*lm_get_owner)(struct file_lock *, struct file_lock *);
> +	void (*lm_put_owner)(struct file_lock *);
>  	void (*lm_notify)(struct file_lock *);	/* unblock callback */
>  	int (*lm_grant)(struct file_lock *, int);
>  	void (*lm_break)(struct file_lock *);

Looks good. I'd probably move patch #3 before this one but that's just
a minor nit.

Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock()

[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 <kinglongmee@gmail.com>
---
 fs/locks.c         | 10 +++++-----
 include/linux/fs.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 08342e0..c376561 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -289,7 +289,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
 /*
  * Initialize a new lock from an existing file_lock structure.
  */
-void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
+void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
 {
 	new->fl_owner = fl->fl_owner;
 	new->fl_pid = fl->fl_pid;
@@ -301,14 +301,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
 	new->fl_ops = NULL;
 	new->fl_lmops = NULL;
 }
-EXPORT_SYMBOL(__locks_copy_lock);
+EXPORT_SYMBOL(locks_copy_conflock);
 
 void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
 {
 	/* "new" must be a freshly-initialized lock */
 	WARN_ON_ONCE(new->fl_ops);
 
-	__locks_copy_lock(new, fl);
+	locks_copy_conflock(new, fl);
 	new->fl_file = fl->fl_file;
 	new->fl_ops = fl->fl_ops;
 	new->fl_lmops = fl->fl_lmops;
@@ -743,7 +743,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
 			break;
 	}
 	if (cfl) {
-		__locks_copy_lock(fl, cfl);
+		locks_copy_conflock(fl, cfl);
 		if (cfl->fl_nspid)
 			fl->fl_pid = pid_vnr(cfl->fl_nspid);
 	} else
@@ -949,7 +949,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
 			if (!posix_locks_conflict(request, fl))
 				continue;
 			if (conflock)
-				__locks_copy_lock(conflock, fl);
+				locks_copy_conflock(conflock, fl);
 			error = -EAGAIN;
 			if (!(request->fl_flags & FL_SLEEP))
 				goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6829340..3b07ce2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -968,7 +968,7 @@ void locks_free_lock(struct file_lock *fl);
 extern void locks_init_lock(struct file_lock *);
 extern struct file_lock * locks_alloc_lock(void);
 extern void locks_copy_lock(struct file_lock *, struct file_lock *);
-extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
+extern void locks_copy_conflock(struct file_lock *, struct file_lock *);
 extern void locks_remove_posix(struct file *, fl_owner_t);
 extern void locks_remove_file(struct file *);
 extern void locks_release_private(struct file_lock *);
@@ -1028,7 +1028,7 @@ static inline void locks_init_lock(struct file_lock *fl)
 	return;
 }
 
-static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
+static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
 {
 	return;
 }
-- 
1.9.3

Re: [PATCH 3/6 v4] locks: Rename __locks_copy_lock() to locks_copy_conflock()

[Jeff Layton]

On Tue, 19 Aug 2014 23:21:19 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> Jeff advice, " Right now __locks_copy_lock is only used to copy 
> conflocks. It would be good to rename that to something more 
> distinct (i.e.locks_copy_conflock), to make it clear that we're
> generating a conflock there."
> 
> v4: a new patch only rename
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/locks.c         | 10 +++++-----
>  include/linux/fs.h |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 08342e0..c376561 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -289,7 +289,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  /*
>   * Initialize a new lock from an existing file_lock structure.
>   */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
>  {
>  	new->fl_owner = fl->fl_owner;
>  	new->fl_pid = fl->fl_pid;
> @@ -301,14 +301,14 @@ void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>  	new->fl_ops = NULL;
>  	new->fl_lmops = NULL;
>  }
> -EXPORT_SYMBOL(__locks_copy_lock);
> +EXPORT_SYMBOL(locks_copy_conflock);
>  
>  void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  {
>  	/* "new" must be a freshly-initialized lock */
>  	WARN_ON_ONCE(new->fl_ops);
>  
> -	__locks_copy_lock(new, fl);
> +	locks_copy_conflock(new, fl);
>  	new->fl_file = fl->fl_file;
>  	new->fl_ops = fl->fl_ops;
>  	new->fl_lmops = fl->fl_lmops;
> @@ -743,7 +743,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  			break;
>  	}
>  	if (cfl) {
> -		__locks_copy_lock(fl, cfl);
> +		locks_copy_conflock(fl, cfl);
>  		if (cfl->fl_nspid)
>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
>  	} else
> @@ -949,7 +949,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>  			if (!posix_locks_conflict(request, fl))
>  				continue;
>  			if (conflock)
> -				__locks_copy_lock(conflock, fl);
> +				locks_copy_conflock(conflock, fl);
>  			error = -EAGAIN;
>  			if (!(request->fl_flags & FL_SLEEP))
>  				goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6829340..3b07ce2 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -968,7 +968,7 @@ void locks_free_lock(struct file_lock *fl);
>  extern void locks_init_lock(struct file_lock *);
>  extern struct file_lock * locks_alloc_lock(void);
>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> +extern void locks_copy_conflock(struct file_lock *, struct file_lock *);
>  extern void locks_remove_posix(struct file *, fl_owner_t);
>  extern void locks_remove_file(struct file *);
>  extern void locks_release_private(struct file_lock *);
> @@ -1028,7 +1028,7 @@ static inline void locks_init_lock(struct file_lock *fl)
>  	return;
>  }
>  
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> +static inline void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
>  {
>  	return;
>  }

Reviewed-by: Jeff Layton <jlayton@primarydata.com>

[PATCH 1/6 v4] NFSD: Remove the duplicate initialize of file_lock

[Kinglong Mee]

locks_alloc_lock() has initialize the struct file_lock,
so, don't need re-initialize it by locks_init_lock().

v4: same as v3, only cleanup

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock()

[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."

v4: only copy fl_lmops for conflock, don't copy fl_ops

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6 v4] locks: Copy fl_lmops information for conflock in, locks_copy_conflock()

[Jeff Layton]

On Tue, 19 Aug 2014 23:24:23 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
> 
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> 
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> 
> Make sure copy the private information by fl_copy_lock() in struct
> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
> 
> Jeff advice, "Set fl_lmops on conflocks, but don't set fl_ops.
> fl_ops are superfluous, since they are callbacks into the filesystem.
> There should be no need to bother the filesystem at all with info
> in a conflock. But, lock _ownership_ matters for conflocks and that's
> indicated by the fl_lmops. So you really do want to copy the fl_lmops
> for conflocks I think."
> 
> v4: only copy fl_lmops for conflock, don't copy fl_ops
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/locks.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index c376561..0ee775d 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl)
>  
>  EXPORT_SYMBOL(locks_init_lock);
>  
> -static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> -{
> -	if (fl->fl_ops) {
> -		if (fl->fl_ops->fl_copy_lock)
> -			fl->fl_ops->fl_copy_lock(new, fl);
> -		new->fl_ops = fl->fl_ops;
> -	}
> -
> -	if (fl->fl_lmops) {
> -		if (fl->fl_lmops->lm_get_owner)
> -			fl->fl_lmops->lm_get_owner(new, fl);
> -		new->fl_lmops = fl->fl_lmops;
> -	}
> -}
> -
>  /*
>   * Initialize a new lock from an existing file_lock structure.
>   */
> @@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
>  	new->fl_type = fl->fl_type;
>  	new->fl_start = fl->fl_start;
>  	new->fl_end = fl->fl_end;
> +	new->fl_lmops = fl->fl_lmops;
>  	new->fl_ops = NULL;
> -	new->fl_lmops = NULL;
> +
> +	if (fl->fl_lmops) {
> +		if (fl->fl_lmops->lm_get_owner)
> +			fl->fl_lmops->lm_get_owner(new, fl);
> +	}
>  }
>  EXPORT_SYMBOL(locks_copy_conflock);
>  
> @@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  	WARN_ON_ONCE(new->fl_ops);
>  
>  	locks_copy_conflock(new, fl);
> +
>  	new->fl_file = fl->fl_file;
>  	new->fl_ops = fl->fl_ops;
> -	new->fl_lmops = fl->fl_lmops;
>  
> -	locks_copy_private(new, fl);
> +	if (fl->fl_ops) {
> +		if (fl->fl_ops->fl_copy_lock)
> +			fl->fl_ops->fl_copy_lock(new, fl);
> +	}
>  }
>  
>  EXPORT_SYMBOL(locks_copy_lock);
> @@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
>  	if (file_lock.fl_type != F_UNLCK) {
>  		error = posix_lock_to_flock(&flock, &file_lock);
>  		if (error)
> -			goto out;
> +			goto rel_priv;
>  	}
>  	error = -EFAULT;
>  	if (!copy_to_user(l, &flock, sizeof(flock)))
>  		error = 0;
> +rel_priv:
> +	locks_release_private(&file_lock);
>  out:
>  	return error;
>  }
> @@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
>  	error = -EFAULT;
>  	if (!copy_to_user(l, &flock, sizeof(flock)))
>  		error = 0;
> -  
> +
> +	locks_release_private(&file_lock);
>  out:
>  	return error;
>  }

Looks good for the most part. I think there is one small piece missing
though.

There is a vfs_test_lock call in nlmsvc_testlock. The conflock in that
case is embedded inside a nlm_lock and locks_release_private is not
called on it (AFAICT). You're not planning to set these new lmops for
lockd, but someone could in the future.

I think it would be good to go ahead and plumb in a call to
locks_release_private there as well to help ensure that no one will
break this in the future. It'll be a no-op for the time being of course.

-- 
Jeff Layton <jlayton@primarydata.com>

[PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference

[Kinglong Mee]

v4: same as v3

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 98edf97..e087a71 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
 	spin_unlock(&nn->client_lock);
 }
 
+static inline struct nfs4_stateowner *
+nfs4_get_stateowner(struct nfs4_stateowner *sop)
+{
+	atomic_inc(&sop->so_count);
+	return sop;
+}
+
 static int
 same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
 {
@@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
 			    so_strhash) {
 		if (!so->so_is_open_owner)
 			continue;
-		if (same_owner_str(so, &open->op_owner)) {
-			atomic_inc(&so->so_count);
-			return openowner(so);
-		}
+		if (same_owner_str(so, &open->op_owner))
+			return openowner(nfs4_get_stateowner(so));
 	}
 	return NULL;
 }
@@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
 	}
 	while (!list_empty(&clp->cl_openowners)) {
 		oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
-		atomic_inc(&oo->oo_owner.so_count);
+		nfs4_get_stateowner(&oo->oo_owner);
 		release_openowner(oo);
 	}
 	nfsd4_shutdown_callback(clp);
@@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
 {
 	if (!nfsd4_has_session(cstate)) {
 		mutex_lock(&so->so_replay.rp_mutex);
-		cstate->replay_owner = so;
-		atomic_inc(&so->so_count);
+		cstate->replay_owner = nfs4_get_stateowner(so);
 	}
 }
 
@@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
 	atomic_inc(&stp->st_stid.sc_count);
 	stp->st_stid.sc_type = NFS4_OPEN_STID;
 	INIT_LIST_HEAD(&stp->st_locks);
-	stp->st_stateowner = &oo->oo_owner;
-	atomic_inc(&stp->st_stateowner->so_count);
+	stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
 	get_nfs4_file(fp);
 	stp->st_stid.sc_file = fp;
 	stp->st_access_bmap = 0;
@@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
 			    so_strhash) {
 		if (so->so_is_open_owner)
 			continue;
-		if (!same_owner_str(so, owner))
-			continue;
-		atomic_inc(&so->so_count);
-		return lockowner(so);
+		if (same_owner_str(so, owner))
+			return lockowner(nfs4_get_stateowner(so));
 	}
 	return NULL;
 }
@@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
 
 	atomic_inc(&stp->st_stid.sc_count);
 	stp->st_stid.sc_type = NFS4_LOCK_STID;
-	stp->st_stateowner = &lo->lo_owner;
-	atomic_inc(&lo->lo_owner.so_count);
+	stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
 	get_nfs4_file(fp);
 	stp->st_stid.sc_file = fp;
 	stp->st_stid.sc_free = nfs4_free_lock_stateid;
@@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 			}
 		}
 
-		atomic_inc(&sop->so_count);
+		nfs4_get_stateowner(sop);
 		break;
 	}
 	spin_unlock(&clp->cl_lock);
-- 
1.9.3

Re: [PATCH 5/6 v4] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference

[Jeff Layton]

On Tue, 19 Aug 2014 23:25:32 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> v4: same as v3
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 98edf97..e087a71 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
>  	spin_unlock(&nn->client_lock);
>  }
>  
> +static inline struct nfs4_stateowner *
> +nfs4_get_stateowner(struct nfs4_stateowner *sop)
> +{
> +	atomic_inc(&sop->so_count);
> +	return sop;
> +}
> +
>  static int
>  same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
>  {
> @@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
>  			    so_strhash) {
>  		if (!so->so_is_open_owner)
>  			continue;
> -		if (same_owner_str(so, &open->op_owner)) {
> -			atomic_inc(&so->so_count);
> -			return openowner(so);
> -		}
> +		if (same_owner_str(so, &open->op_owner))
> +			return openowner(nfs4_get_stateowner(so));
>  	}
>  	return NULL;
>  }
> @@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
>  	}
>  	while (!list_empty(&clp->cl_openowners)) {
>  		oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
> -		atomic_inc(&oo->oo_owner.so_count);
> +		nfs4_get_stateowner(&oo->oo_owner);
>  		release_openowner(oo);
>  	}
>  	nfsd4_shutdown_callback(clp);
> @@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
>  {
>  	if (!nfsd4_has_session(cstate)) {
>  		mutex_lock(&so->so_replay.rp_mutex);
> -		cstate->replay_owner = so;
> -		atomic_inc(&so->so_count);
> +		cstate->replay_owner = nfs4_get_stateowner(so);
>  	}
>  }
>  
> @@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>  	atomic_inc(&stp->st_stid.sc_count);
>  	stp->st_stid.sc_type = NFS4_OPEN_STID;
>  	INIT_LIST_HEAD(&stp->st_locks);
> -	stp->st_stateowner = &oo->oo_owner;
> -	atomic_inc(&stp->st_stateowner->so_count);
> +	stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
>  	get_nfs4_file(fp);
>  	stp->st_stid.sc_file = fp;
>  	stp->st_access_bmap = 0;
> @@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
>  			    so_strhash) {
>  		if (so->so_is_open_owner)
>  			continue;
> -		if (!same_owner_str(so, owner))
> -			continue;
> -		atomic_inc(&so->so_count);
> -		return lockowner(so);
> +		if (same_owner_str(so, owner))
> +			return lockowner(nfs4_get_stateowner(so));
>  	}
>  	return NULL;
>  }
> @@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>  
>  	atomic_inc(&stp->st_stid.sc_count);
>  	stp->st_stid.sc_type = NFS4_LOCK_STID;
> -	stp->st_stateowner = &lo->lo_owner;
> -	atomic_inc(&lo->lo_owner.so_count);
> +	stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
>  	get_nfs4_file(fp);
>  	stp->st_stid.sc_file = fp;
>  	stp->st_stid.sc_free = nfs4_free_lock_stateid;
> @@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  			}
>  		}
>  
> -		atomic_inc(&sop->so_count);
> +		nfs4_get_stateowner(sop);
>  		break;
>  	}
>  	spin_unlock(&clp->cl_lock);

Reviewed-by: Jeff Layton <jlayton@primarydata.com>

[PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

v4: same as v3, no change
v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e087a71..7161111 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
 		lock->fl_end = OFFSET_MAX;
 }
 
-/* Hack!: For now, we're defining this just so we can use a pointer to it
- * as a unique cookie to identify our (NFSv4's) posix locks. */
+static inline struct nfs4_lockowner *
+nfs4_get_lockowner(struct nfs4_lockowner *lo)
+{
+	return lockowner(nfs4_get_stateowner(&lo->lo_owner));
+}
+
+static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
+{
+	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
+	dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
+}
+
+static void nfsd4_fl_put_owner(struct file_lock *fl)
+{
+	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
+
+	if (lo) {
+		nfs4_put_stateowner(&lo->lo_owner);
+		fl->fl_owner = NULL;
+	}
+}
+
 static const struct lock_manager_operations nfsd_posix_mng_ops  = {
+	.lm_get_owner = nfsd4_fl_get_owner,
+	.lm_put_owner = nfsd4_fl_put_owner,
 };
 
 static inline void
@@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = nfserr_openmode;
 		goto out;
 	}
-	file_lock->fl_owner = (fl_owner_t)lock_sop;
+
+	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
 	file_lock->fl_pid = current->tgid;
 	file_lock->fl_file = filp;
 	file_lock->fl_flags = FL_POSIX;
@@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfs4_ol_stateid *stp;
 	struct file *filp = NULL;
 	struct file_lock *file_lock = NULL;
+	struct nfs4_lockowner *lock_sop = NULL;
 	__be32 status;
 	int err;
 	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = nfserr_lock_range;
 		goto put_stateid;
 	}
+
+	lock_sop = lockowner(stp->st_stateowner);
 	file_lock = locks_alloc_lock();
 	if (!file_lock) {
 		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
@@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	}
 
 	file_lock->fl_type = F_UNLCK;
-	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
+	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
 	file_lock->fl_pid = current->tgid;
 	file_lock->fl_file = filp;
 	file_lock->fl_flags = FL_POSIX;
-- 
1.9.3

Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock

[Jeff Layton]

On Tue, 19 Aug 2014 23:26:45 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> v4: same as v3, no change
> v3: Update based on Jeff's comments
> v2: Fix bad using of struct file_lock_operations for handle the owner.
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e087a71..7161111 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
>  		lock->fl_end = OFFSET_MAX;
>  }
>  
> -/* Hack!: For now, we're defining this just so we can use a pointer to it
> - * as a unique cookie to identify our (NFSv4's) posix locks. */
> +static inline struct nfs4_lockowner *
> +nfs4_get_lockowner(struct nfs4_lockowner *lo)
> +{
> +	return lockowner(nfs4_get_stateowner(&lo->lo_owner));
> +}
> +

I'd probably not bother with this inline function. Just open code that
into the callers.

> +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
> +{
> +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> +	dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
> +}
> +
> +static void nfsd4_fl_put_owner(struct file_lock *fl)
> +{
> +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> +
> +	if (lo) {
> +		nfs4_put_stateowner(&lo->lo_owner);
> +		fl->fl_owner = NULL;
> +	}
> +}
> +
>  static const struct lock_manager_operations nfsd_posix_mng_ops  = {
> +	.lm_get_owner = nfsd4_fl_get_owner,
> +	.lm_put_owner = nfsd4_fl_put_owner,
>  };
>  
>  static inline void
> @@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_openmode;
>  		goto out;
>  	}
> -	file_lock->fl_owner = (fl_owner_t)lock_sop;
> +
> +	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
>  	file_lock->fl_pid = current->tgid;
>  	file_lock->fl_file = filp;
>  	file_lock->fl_flags = FL_POSIX;
> @@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_ol_stateid *stp;
>  	struct file *filp = NULL;
>  	struct file_lock *file_lock = NULL;
> +	struct nfs4_lockowner *lock_sop = NULL;

nit: Probably no need to initialize lock_sop to NULL. Even better, I'd
just drop that and change the fl_owner assignment below.

>  	__be32 status;
>  	int err;
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> @@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_lock_range;
>  		goto put_stateid;
>  	}
> +
> +	lock_sop = lockowner(stp->st_stateowner);
>  	file_lock = locks_alloc_lock();
>  	if (!file_lock) {
>  		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> @@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  
>  	file_lock->fl_type = F_UNLCK;
> -	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> +	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);

I'd do this instead and not bother with a nfs4_get_lockowner at all...

	file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));

>  	file_lock->fl_pid = current->tgid;
>  	file_lock->fl_file = filp;
>  	file_lock->fl_flags = FL_POSIX;

But those are minor nits. This looks fine otherwise.

Bruce, if it's OK by you, I'll just take the whole series once Kinglong
respins. It does touch some nfsd code, but it hopefully shouldn't cause
much in the way of conflicts with anything you have queued for v3.18.

Acked-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock

[J. Bruce Fields]

On Tue, Aug 19, 2014 at 04:23:44PM -0400, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
> > v4: same as v3, no change
> > v3: Update based on Jeff's comments
> > v2: Fix bad using of struct file_lock_operations for handle the owner.
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> >  fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e087a71..7161111 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4869,9 +4869,31 @@ nfs4_transform_lock_offset(struct file_lock *lock)
> >  		lock->fl_end = OFFSET_MAX;
> >  }
> >  
> > -/* Hack!: For now, we're defining this just so we can use a pointer to it
> > - * as a unique cookie to identify our (NFSv4's) posix locks. */
> > +static inline struct nfs4_lockowner *
> > +nfs4_get_lockowner(struct nfs4_lockowner *lo)
> > +{
> > +	return lockowner(nfs4_get_stateowner(&lo->lo_owner));
> > +}
> > +
> 
> I'd probably not bother with this inline function. Just open code that
> into the callers.
> 
> > +static void nfsd4_fl_get_owner(struct file_lock *dst, struct file_lock *src)
> > +{
> > +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) src->fl_owner;
> > +	dst->fl_owner = (fl_owner_t) nfs4_get_lockowner(lo);
> > +}
> > +
> > +static void nfsd4_fl_put_owner(struct file_lock *fl)
> > +{
> > +	struct nfs4_lockowner *lo = (struct nfs4_lockowner *) fl->fl_owner;
> > +
> > +	if (lo) {
> > +		nfs4_put_stateowner(&lo->lo_owner);
> > +		fl->fl_owner = NULL;
> > +	}
> > +}
> > +
> >  static const struct lock_manager_operations nfsd_posix_mng_ops  = {
> > +	.lm_get_owner = nfsd4_fl_get_owner,
> > +	.lm_put_owner = nfsd4_fl_put_owner,
> >  };
> >  
> >  static inline void
> > @@ -5236,7 +5258,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  		status = nfserr_openmode;
> >  		goto out;
> >  	}
> > -	file_lock->fl_owner = (fl_owner_t)lock_sop;
> > +
> > +	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
> >  	file_lock->fl_pid = current->tgid;
> >  	file_lock->fl_file = filp;
> >  	file_lock->fl_flags = FL_POSIX;
> > @@ -5403,6 +5426,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	struct nfs4_ol_stateid *stp;
> >  	struct file *filp = NULL;
> >  	struct file_lock *file_lock = NULL;
> > +	struct nfs4_lockowner *lock_sop = NULL;
> 
> nit: Probably no need to initialize lock_sop to NULL. Even better, I'd
> just drop that and change the fl_owner assignment below.
> 
> >  	__be32 status;
> >  	int err;
> >  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > @@ -5424,6 +5448,8 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  		status = nfserr_lock_range;
> >  		goto put_stateid;
> >  	}
> > +
> > +	lock_sop = lockowner(stp->st_stateowner);
> >  	file_lock = locks_alloc_lock();
> >  	if (!file_lock) {
> >  		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> > @@ -5432,7 +5458,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	}
> >  
> >  	file_lock->fl_type = F_UNLCK;
> > -	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
> > +	file_lock->fl_owner = (fl_owner_t) nfs4_get_lockowner(lock_sop);
> 
> I'd do this instead and not bother with a nfs4_get_lockowner at all...
> 
> 	file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner));
> 
> >  	file_lock->fl_pid = current->tgid;
> >  	file_lock->fl_file = filp;
> >  	file_lock->fl_flags = FL_POSIX;
> 
> But those are minor nits. This looks fine otherwise.
> 
> Bruce, if it's OK by you, I'll just take the whole series once Kinglong
> respins. It does touch some nfsd code, but it hopefully shouldn't cause
> much in the way of conflicts with anything you have queued for v3.18.

That's fine by me.

--b.

Re: [PATCH 6/6 v4] NFSD: Increase the reference of lockowner when coping file_lock

[Kinglong Mee]

On 8/20/2014 04:23, Jeff Layton wrote:
> On Tue, 19 Aug 2014 23:26:45 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> v4: same as v3, no change
>> v3: Update based on Jeff's comments
>> v2: Fix bad using of struct file_lock_operations for handle the owner.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6 v5] locks: Rename __locks_copy_lock() to locks_copy_conflock()

[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."

v5: change order from 3/6 to 2/6
v4: new patch only renaming function name

Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6 v5] locks: New ops in file_lock_operations for get/put owner

[Kinglong Mee]

NFSD or other lockmanager may increase the owner's reference,
so adds two new options for copying and releasing owner.

v5: change order from 2/6 to 3/6
v4: rename lm_copy_owner/lm_release_owner to lm_get_owner/lm_put_owner

Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6 v5] NFSD: New helper nfs4_get_stateowner() for atomic_inc sop reference

[Kinglong Mee]

v5: same as the first version

Reviewed-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6 v5] NFSD: Get reference of lockowner when coping file_lock

[Kinglong Mee]

v5: using nfs4_get_stateowner() instead of an inline function
v3: Update based on Jeff's comments
v2: Fix bad using of struct file_lock_operations for handle the owner

Acked-by: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6 v5] NFSD: Remove the duplicate initialize of file_lock

[Kinglong Mee]

locks_alloc_lock() has initialize the struct file_lock,
so, don't need re-initialize it by locks_init_lock().

v5: same the first version, only cleanup

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
Reviewed-by: Jeff Layton <jlayton@primarydata.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e80a59..98edf97 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3759,7 +3759,6 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
 	fl = locks_alloc_lock();
 	if (!fl)
 		return NULL;
-	locks_init_lock(fl);
 	fl->fl_lmops = &nfsd_lease_mng_ops;
 	fl->fl_flags = FL_DELEG;
 	fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
@@ -5210,7 +5209,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	}
 
 	fp = lock_stp->st_stid.sc_file;
-	locks_init_lock(file_lock);
 	switch (lock->lk_type) {
 		case NFS4_READ_LT:
 		case NFS4_READW_LT:
@@ -5354,7 +5352,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = nfserr_jukebox;
 		goto out;
 	}
-	locks_init_lock(file_lock);
+
 	switch (lockt->lt_type) {
 		case NFS4_READ_LT:
 		case NFS4_READW_LT:
@@ -5432,7 +5430,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = nfserr_jukebox;
 		goto fput;
 	}
-	locks_init_lock(file_lock);
+
 	file_lock->fl_type = F_UNLCK;
 	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
 	file_lock->fl_pid = current->tgid;
-- 
1.9.3

[PATCH 4/6 v5] locks: Copy fl_lmops information for conflock in locks_copy_conflock()

[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 <kinglongmee@gmail.com>
---
 fs/lockd/svclock.c |  1 +
 fs/locks.c         | 36 ++++++++++++++++--------------------
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 796e63b..2e1b292 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -570,6 +570,7 @@ nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 	conflock->fl.fl_type = lock->fl.fl_type;
 	conflock->fl.fl_start = lock->fl.fl_start;
 	conflock->fl.fl_end = lock->fl.fl_end;
+	locks_release_private(&lock->fl);
 	ret = nlm_lck_denied;
 out:
 	if (block)
diff --git a/fs/locks.c b/fs/locks.c
index c376561..0ee775d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -271,21 +271,6 @@ void locks_init_lock(struct file_lock *fl)
 
 EXPORT_SYMBOL(locks_init_lock);
 
-static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
-{
-	if (fl->fl_ops) {
-		if (fl->fl_ops->fl_copy_lock)
-			fl->fl_ops->fl_copy_lock(new, fl);
-		new->fl_ops = fl->fl_ops;
-	}
-
-	if (fl->fl_lmops) {
-		if (fl->fl_lmops->lm_get_owner)
-			fl->fl_lmops->lm_get_owner(new, fl);
-		new->fl_lmops = fl->fl_lmops;
-	}
-}
-
 /*
  * Initialize a new lock from an existing file_lock structure.
  */
@@ -298,8 +283,13 @@ void locks_copy_conflock(struct file_lock *new, struct file_lock *fl)
 	new->fl_type = fl->fl_type;
 	new->fl_start = fl->fl_start;
 	new->fl_end = fl->fl_end;
+	new->fl_lmops = fl->fl_lmops;
 	new->fl_ops = NULL;
-	new->fl_lmops = NULL;
+
+	if (fl->fl_lmops) {
+		if (fl->fl_lmops->lm_get_owner)
+			fl->fl_lmops->lm_get_owner(new, fl);
+	}
 }
 EXPORT_SYMBOL(locks_copy_conflock);
 
@@ -309,11 +299,14 @@ void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
 	WARN_ON_ONCE(new->fl_ops);
 
 	locks_copy_conflock(new, fl);
+
 	new->fl_file = fl->fl_file;
 	new->fl_ops = fl->fl_ops;
-	new->fl_lmops = fl->fl_lmops;
 
-	locks_copy_private(new, fl);
+	if (fl->fl_ops) {
+		if (fl->fl_ops->fl_copy_lock)
+			fl->fl_ops->fl_copy_lock(new, fl);
+	}
 }
 
 EXPORT_SYMBOL(locks_copy_lock);
@@ -1989,11 +1982,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock __user *l)
 	if (file_lock.fl_type != F_UNLCK) {
 		error = posix_lock_to_flock(&flock, &file_lock);
 		if (error)
-			goto out;
+			goto rel_priv;
 	}
 	error = -EFAULT;
 	if (!copy_to_user(l, &flock, sizeof(flock)))
 		error = 0;
+rel_priv:
+	locks_release_private(&file_lock);
 out:
 	return error;
 }
@@ -2214,7 +2209,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 __user *l)
 	error = -EFAULT;
 	if (!copy_to_user(l, &flock, sizeof(flock)))
 		error = 0;
-  
+
+	locks_release_private(&file_lock);
 out:
 	return error;
 }
-- 
1.9.3

[PATCH 2/5 v3] locks: Copy all infomation for conflock

[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().

v3: Update based on Joe and Jeff's patch.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/locks.c         | 24 +++++++-----------------
 include/linux/fs.h |  6 ------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index cb66fb0..fe52abb 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
 /*
  * Initialize a new lock from an existing file_lock structure.
  */
-void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
+void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
 {
+	/* "new" must be a freshly-initialized lock */
+	WARN_ON_ONCE(new->fl_ops);
+
 	new->fl_owner = fl->fl_owner;
 	new->fl_pid = fl->fl_pid;
-	new->fl_file = NULL;
+	new->fl_file = fl->fl_file;
 	new->fl_flags = fl->fl_flags;
 	new->fl_type = fl->fl_type;
 	new->fl_start = fl->fl_start;
 	new->fl_end = fl->fl_end;
 	new->fl_ops = NULL;
 	new->fl_lmops = NULL;
-}
-EXPORT_SYMBOL(__locks_copy_lock);
-
-void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
-{
-	/* "new" must be a freshly-initialized lock */
-	WARN_ON_ONCE(new->fl_ops);
-
-	__locks_copy_lock(new, fl);
-	new->fl_file = fl->fl_file;
-	new->fl_ops = fl->fl_ops;
-	new->fl_lmops = fl->fl_lmops;
 
 	locks_copy_private(new, fl);
 }
-
 EXPORT_SYMBOL(locks_copy_lock);
 
 static inline int flock_translate_cmd(int cmd) {
@@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
 			break;
 	}
 	if (cfl) {
-		__locks_copy_lock(fl, cfl);
+		locks_copy_lock(fl, cfl);
 		if (cfl->fl_nspid)
 			fl->fl_pid = pid_vnr(cfl->fl_nspid);
 	} else
@@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
 			if (!posix_locks_conflict(request, fl))
 				continue;
 			if (conflock)
-				__locks_copy_lock(conflock, fl);
+				locks_copy_lock(conflock, fl);
 			error = -EAGAIN;
 			if (!(request->fl_flags & FL_SLEEP))
 				goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 908af4f..a383a30 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
 extern void locks_init_lock(struct file_lock *);
 extern struct file_lock * locks_alloc_lock(void);
 extern void locks_copy_lock(struct file_lock *, struct file_lock *);
-extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
 extern void locks_remove_posix(struct file *, fl_owner_t);
 extern void locks_remove_file(struct file *);
 extern void locks_release_private(struct file_lock *);
@@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
 	return;
 }
 
-static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
-{
-	return;
-}
-
 static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
 {
 	return;
-- 
1.9.3

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Jeff Layton]

On Fri, 15 Aug 2014 08:07:12 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> fl_lmops field in file_lock for checking nfsd4 lockowner.
> 
> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> 
> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> 
> Make sure copy the private information by fl_copy_lock() in struct
> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
> 
> v3: Update based on Joe and Jeff's patch.
> 
> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/locks.c         | 24 +++++++-----------------
>  include/linux/fs.h |  6 ------
>  2 files changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index cb66fb0..fe52abb 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  /*
>   * Initialize a new lock from an existing file_lock structure.
>   */
> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  {
> +	/* "new" must be a freshly-initialized lock */
> +	WARN_ON_ONCE(new->fl_ops);
> +
>  	new->fl_owner = fl->fl_owner;
>  	new->fl_pid = fl->fl_pid;
> -	new->fl_file = NULL;
> +	new->fl_file = fl->fl_file;
>  	new->fl_flags = fl->fl_flags;
>  	new->fl_type = fl->fl_type;
>  	new->fl_start = fl->fl_start;
>  	new->fl_end = fl->fl_end;
>  	new->fl_ops = NULL;
>  	new->fl_lmops = NULL;
> -}
> -EXPORT_SYMBOL(__locks_copy_lock);
> -
> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> -	/* "new" must be a freshly-initialized lock */
> -	WARN_ON_ONCE(new->fl_ops);
> -
> -	__locks_copy_lock(new, fl);
> -	new->fl_file = fl->fl_file;
> -	new->fl_ops = fl->fl_ops;
> -	new->fl_lmops = fl->fl_lmops;
>  
>  	locks_copy_private(new, fl);
>  }
> -
>  EXPORT_SYMBOL(locks_copy_lock);
>  
>  static inline int flock_translate_cmd(int cmd) {
> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  			break;
>  	}
>  	if (cfl) {
> -		__locks_copy_lock(fl, cfl);
> +		locks_copy_lock(fl, cfl);
>  		if (cfl->fl_nspid)
>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
>  	} else
> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>  			if (!posix_locks_conflict(request, fl))
>  				continue;
>  			if (conflock)
> -				__locks_copy_lock(conflock, fl);
> +				locks_copy_lock(conflock, fl);
>  			error = -EAGAIN;
>  			if (!(request->fl_flags & FL_SLEEP))
>  				goto out;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 908af4f..a383a30 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>  extern void locks_init_lock(struct file_lock *);
>  extern struct file_lock * locks_alloc_lock(void);
>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>  extern void locks_remove_posix(struct file *, fl_owner_t);
>  extern void locks_remove_file(struct file *);
>  extern void locks_release_private(struct file_lock *);
> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>  	return;
>  }
>  
> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> -{
> -	return;
> -}
> -
>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>  {
>  	return;

I'm not sure this is really what you want to do. Calling fl_copy_lock
for a conflock looks relatively harmless for nfs and nlm. AFS though
seems to add the lock to a list associated with the inode. That seems a
little suspicious for a conflock and could be problematic. It may be
best to avoid dealing with fl_ops for a conflock.

Also in the case of fcntl_getlk, the struct file_lock lives on the
stack, and locks_release_private is never called on it. You'll need to
audit all of the current callers of __locks_copy_lock to ensure that
any resources you end up taking references on when copying the conflock
are eventually released.

-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Kinglong Mee]

On 8/15/2014 19:14, Jeff Layton wrote:
> On Fri, 15 Aug 2014 08:07:12 +0800
> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>
>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>
>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>
>> Make sure copy the private information by fl_copy_lock() in struct
>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>
>> v3: Update based on Joe and Jeff's patch.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Kinglong Mee]

On 8/15/2014 22:33, Kinglong Mee wrote:
> On 8/15/2014 19:14, Jeff Layton wrote:
>> On Fri, 15 Aug 2014 08:07:12 +0800
>> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>
>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>
>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>
>>> Make sure copy the private information by fl_copy_lock() in struct
>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>
>>> v3: Update based on Joe and Jeff's patch.
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Kinglong Mee]

On 8/16/2014 21:35, Kinglong Mee wrote:
> On 8/15/2014 22:33, Kinglong Mee wrote:
>> On 8/15/2014 19:14, Jeff Layton wrote:
>>> On Fri, 15 Aug 2014 08:07:12 +0800
>>> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>
>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>
>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>
>>>> Make sure copy the private information by fl_copy_lock() in struct
>>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>>
>>>> v3: Update based on Joe and Jeff's patch.
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Jeff Layton]

On Sun, 17 Aug 2014 21:42:21 +0800
Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 8/16/2014 21:35, Kinglong Mee wrote:
> > On 8/15/2014 22:33, Kinglong Mee wrote:
> >> On 8/15/2014 19:14, Jeff Layton wrote:
> >>> On Fri, 15 Aug 2014 08:07:12 +0800
> >>> Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>>
> >>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
> >>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
> >>>>
> >>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
> >>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
> >>>>
> >>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
> >>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
> >>>>
> >>>> Make sure copy the private information by fl_copy_lock() in struct
> >>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
> >>>>
> >>>> v3: Update based on Joe and Jeff's patch.
> >>>>
> >>>> Signed-off-by: Kinglong Mee <kinglongmee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>> ---
> >>>>  fs/locks.c         | 24 +++++++-----------------
> >>>>  include/linux/fs.h |  6 ------
> >>>>  2 files changed, 7 insertions(+), 23 deletions(-)
> >>>>
> >>>> diff --git a/fs/locks.c b/fs/locks.c
> >>>> index cb66fb0..fe52abb 100644
> >>>> --- a/fs/locks.c
> >>>> +++ b/fs/locks.c
> >>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> >>>>  /*
> >>>>   * Initialize a new lock from an existing file_lock structure.
> >>>>   */
> >>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
> >>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>>  {
> >>>> +	/* "new" must be a freshly-initialized lock */
> >>>> +	WARN_ON_ONCE(new->fl_ops);
> >>>> +
> >>>>  	new->fl_owner = fl->fl_owner;
> >>>>  	new->fl_pid = fl->fl_pid;
> >>>> -	new->fl_file = NULL;
> >>>> +	new->fl_file = fl->fl_file;
> >>>>  	new->fl_flags = fl->fl_flags;
> >>>>  	new->fl_type = fl->fl_type;
> >>>>  	new->fl_start = fl->fl_start;
> >>>>  	new->fl_end = fl->fl_end;
> >>>>  	new->fl_ops = NULL;
> >>>>  	new->fl_lmops = NULL;
> >>>> -}
> >>>> -EXPORT_SYMBOL(__locks_copy_lock);
> >>>> -
> >>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> -{
> >>>> -	/* "new" must be a freshly-initialized lock */
> >>>> -	WARN_ON_ONCE(new->fl_ops);
> >>>> -
> >>>> -	__locks_copy_lock(new, fl);
> >>>> -	new->fl_file = fl->fl_file;
> >>>> -	new->fl_ops = fl->fl_ops;
> >>>> -	new->fl_lmops = fl->fl_lmops;
> >>>>  
> >>>>  	locks_copy_private(new, fl);
> >>>>  }
> >>>> -
> >>>>  EXPORT_SYMBOL(locks_copy_lock);
> >>>>  
> >>>>  static inline int flock_translate_cmd(int cmd) {
> >>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
> >>>>  			break;
> >>>>  	}
> >>>>  	if (cfl) {
> >>>> -		__locks_copy_lock(fl, cfl);
> >>>> +		locks_copy_lock(fl, cfl);
> >>>>  		if (cfl->fl_nspid)
> >>>>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
> >>>>  	} else
> >>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
> >>>>  			if (!posix_locks_conflict(request, fl))
> >>>>  				continue;
> >>>>  			if (conflock)
> >>>> -				__locks_copy_lock(conflock, fl);
> >>>> +				locks_copy_lock(conflock, fl);
> >>>>  			error = -EAGAIN;
> >>>>  			if (!(request->fl_flags & FL_SLEEP))
> >>>>  				goto out;
> >>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >>>> index 908af4f..a383a30 100644
> >>>> --- a/include/linux/fs.h
> >>>> +++ b/include/linux/fs.h
> >>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
> >>>>  extern void locks_init_lock(struct file_lock *);
> >>>>  extern struct file_lock * locks_alloc_lock(void);
> >>>>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
> >>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
> >>>>  extern void locks_remove_posix(struct file *, fl_owner_t);
> >>>>  extern void locks_remove_file(struct file *);
> >>>>  extern void locks_release_private(struct file_lock *);
> >>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
> >>>>  	return;
> >>>>  }
> >>>>  
> >>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>> -{
> >>>> -	return;
> >>>> -}
> >>>> -
> >>>>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> >>>>  {
> >>>>  	return;
> >>>
> >>> I'm not sure this is really what you want to do. Calling fl_copy_lock
> >>> for a conflock looks relatively harmless for nfs and nlm. AFS though
> >>> seems to add the lock to a list associated with the inode. That seems a
> >>> little suspicious for a conflock and could be problematic. It may be
> >>> best to avoid dealing with fl_ops for a conflock.
> >>>
> >>> Also in the case of fcntl_getlk, the struct file_lock lives on the
> >>> stack, and locks_release_private is never called on it. You'll need to
> >>> audit all of the current callers of __locks_copy_lock to ensure that
> >>> any resources you end up taking references on when copying the conflock
> >>> are eventually released.
> >>
> >> Sorry for my no further think about it.
> >> I will check that again next day.
> > 
> > I think we should not change the logical of coping lock,
> > leave fl_ops and fl_lmops as private data as right now.

Why not? I think we'd benefit from making conflock creation a more
distinct operation.

> > 
> > I have plan to,
> > 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
> >    I think it should be a private data, am I right?
> > 2. call locks_copy_private() coping private data specifically.
> >    a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
> >       to point whether coping private data.
> >    b. hack the conflock's fl_flags to do the same thing as a,
> >       adds FL_NEED_PRIV fl_flags only valid for conflock.
> > 
> > I don't think 2.a is a nice resolve, because it changes the interface
> > and many caller don't care the private data (I think contains fl_owner)
> > for conflock except nfsd.
> > 
> > So, I'd like *2.b*. A draft is appended as following,
> 
> I'm so sorry for the first draft, there is a bug of it.
> Please using the new draft.
> 
> thanks,
> Kinglong Mee
> 

Personally, I'm not a fan of the approach below. I don't think we need
a new flag for this and it doesn't do anything to solve the problem
where the lockowner could go away while you're operating on it in a
conflock.

I think we need several smaller changes:

1. Right now __locks_copy_lock is only used to copy conflocks. It would
be good to rename that to something more distinct (i.e.
locks_copy_conflock), to make it clear that we're generating a conflock
there.

2. Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are
superfluous, since they are callbacks into the filesystem. There should
be no need to bother the filesystem at all with info in a conflock.
But, lock _ownership_ matters for conflocks and that's indicated by the
fl_lmops. So you really do want to copy the fl_lmops for conflocks I
think.

3. Add the operations you added before to fl_lmops to copy and release
the owner (maybe even rename them lm_get_owner/lm_put_owner?), and
ensure that the places that copy conflocks call those operations
appropriately.

That should be all that's required here.

> ------------------snip---------------------------------------------------------
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index be1858e..128e34f 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -279,6 +279,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>  		new->fl_ops = fl->fl_ops;
>  	}
>  
> +	new->fl_owner = fl->fl_owner;
>  	if (fl->fl_lmops) {
>  		if (fl->fl_lmops->lm_copy_owner)
>  			fl->fl_lmops->lm_copy_owner(new, fl);
> @@ -291,7 +292,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>   */
>  void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>  {
> -	new->fl_owner = fl->fl_owner;
> +	new->fl_owner = NULL;
>  	new->fl_pid = fl->fl_pid;
>  	new->fl_file = NULL;
>  	new->fl_flags = fl->fl_flags;
> @@ -734,6 +735,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  {
>  	struct file_lock *cfl;
>  	struct inode *inode = file_inode(filp);
> +	bool need_priv = !!(fl->fl_flags & FL_NEED_PRIV);
>  
>  	spin_lock(&inode->i_lock);
>  	for (cfl = file_inode(filp)->i_flock; cfl; cfl = cfl->fl_next) {
> @@ -746,6 +748,8 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>  		__locks_copy_lock(fl, cfl);
>  		if (cfl->fl_nspid)
>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
> +		if (need_priv)
> +			locks_copy_private(fl, cfl);
>  	} else
>  		fl->fl_type = F_UNLCK;
>  	spin_unlock(&inode->i_lock);
> @@ -919,7 +923,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>  	struct file_lock *right = NULL;
>  	struct file_lock **before;
>  	int error;
> -	bool added = false;
> +	bool added = false, need_priv = false;
>  	LIST_HEAD(dispose);
>  
>  	/*
> @@ -948,8 +952,12 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>  				continue;
>  			if (!posix_locks_conflict(request, fl))
>  				continue;
> -			if (conflock)
> +			if (conflock) {
> +				need_priv = !!(conflock->fl_flags & FL_NEED_PRIV);
>  				__locks_copy_lock(conflock, fl);
> +				if (need_priv)
> +					locks_copy_private(conflock, fl);
> +			}
>  			error = -EAGAIN;
>  			if (!(request->fl_flags & FL_SLEEP))
>  				goto out;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 5076497..3db43f9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5275,6 +5275,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	}
>  
> +	conflock->fl_flags = FL_NEED_PRIV;
>  	err = vfs_lock_file(filp, F_SETLK, file_lock, conflock);
>  	switch (-err) {
>  	case 0: /* success! */
> @@ -5396,7 +5397,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (lo)
>  		file_lock->fl_owner = (fl_owner_t)lo;
>  	file_lock->fl_pid = current->tgid;
> -	file_lock->fl_flags = FL_POSIX;
> +	file_lock->fl_flags = FL_POSIX | FL_NEED_PRIV;
>  
>  	file_lock->fl_start = lockt->lt_offset;
>  	file_lock->fl_end = last_byte_offset(lockt->lt_offset, lockt->lt_length);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 56f2acd..f257d0d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f)
>  #define FL_DOWNGRADE_PENDING	256 /* Lease is being downgraded */
>  #define FL_UNLOCK_PENDING	512 /* Lease is being broken */
>  #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
> +#define FL_NEED_PRIV	2048	/* Need copy private data from conflock */
>  
>  /*
>   * Special return value from posix_lock_file() and vfs_lock_file() for
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5 v3] locks: Copy all infomation for conflock

[Kinglong Mee]

On 8/18/2014 19:54, Jeff Layton wrote:
> On Sun, 17 Aug 2014 21:42:21 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
>> On 8/16/2014 21:35, Kinglong Mee wrote:
>>> On 8/15/2014 22:33, Kinglong Mee wrote:
>>>> On 8/15/2014 19:14, Jeff Layton wrote:
>>>>> On Fri, 15 Aug 2014 08:07:12 +0800
>>>>> Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>>>
>>>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>>>
>>>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>>>
>>>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>>>
>>>>>> Make sure copy the private information by fl_copy_lock() in struct
>>>>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>>>>
>>>>>> v3: Update based on Joe and Jeff's patch.
>>>>>>
>>>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>>>> ---
>>>>>>  fs/locks.c         | 24 +++++++-----------------
>>>>>>  include/linux/fs.h |  6 ------
>>>>>>  2 files changed, 7 insertions(+), 23 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>> index cb66fb0..fe52abb 100644
>>>>>> --- a/fs/locks.c
>>>>>> +++ b/fs/locks.c
>>>>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>>>>  /*
>>>>>>   * Initialize a new lock from an existing file_lock structure.
>>>>>>   */
>>>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>>  {
>>>>>> +	/* "new" must be a freshly-initialized lock */
>>>>>> +	WARN_ON_ONCE(new->fl_ops);
>>>>>> +
>>>>>>  	new->fl_owner = fl->fl_owner;
>>>>>>  	new->fl_pid = fl->fl_pid;
>>>>>> -	new->fl_file = NULL;
>>>>>> +	new->fl_file = fl->fl_file;
>>>>>>  	new->fl_flags = fl->fl_flags;
>>>>>>  	new->fl_type = fl->fl_type;
>>>>>>  	new->fl_start = fl->fl_start;
>>>>>>  	new->fl_end = fl->fl_end;
>>>>>>  	new->fl_ops = NULL;
>>>>>>  	new->fl_lmops = NULL;
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>>>> -
>>>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> -	/* "new" must be a freshly-initialized lock */
>>>>>> -	WARN_ON_ONCE(new->fl_ops);
>>>>>> -
>>>>>> -	__locks_copy_lock(new, fl);
>>>>>> -	new->fl_file = fl->fl_file;
>>>>>> -	new->fl_ops = fl->fl_ops;
>>>>>> -	new->fl_lmops = fl->fl_lmops;
>>>>>>  
>>>>>>  	locks_copy_private(new, fl);
>>>>>>  }
>>>>>> -
>>>>>>  EXPORT_SYMBOL(locks_copy_lock);
>>>>>>  
>>>>>>  static inline int flock_translate_cmd(int cmd) {
>>>>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>>>>  			break;
>>>>>>  	}
>>>>>>  	if (cfl) {
>>>>>> -		__locks_copy_lock(fl, cfl);
>>>>>> +		locks_copy_lock(fl, cfl);
>>>>>>  		if (cfl->fl_nspid)
>>>>>>  			fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>>>>  	} else
>>>>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>>>>  			if (!posix_locks_conflict(request, fl))
>>>>>>  				continue;
>>>>>>  			if (conflock)
>>>>>> -				__locks_copy_lock(conflock, fl);
>>>>>> +				locks_copy_lock(conflock, fl);
>>>>>>  			error = -EAGAIN;
>>>>>>  			if (!(request->fl_flags & FL_SLEEP))
>>>>>>  				goto out;
>>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>>>> index 908af4f..a383a30 100644
>>>>>> --- a/include/linux/fs.h
>>>>>> +++ b/include/linux/fs.h
>>>>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>>>>>>  extern void locks_init_lock(struct file_lock *);
>>>>>>  extern struct file_lock * locks_alloc_lock(void);
>>>>>>  extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>>>>  extern void locks_remove_posix(struct file *, fl_owner_t);
>>>>>>  extern void locks_remove_file(struct file *);
>>>>>>  extern void locks_release_private(struct file_lock *);
>>>>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>>>>  	return;
>>>>>>  }
>>>>>>  
>>>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> -	return;
>>>>>> -}
>>>>>> -
>>>>>>  static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>>  {
>>>>>>  	return;
>>>>>
>>>>> I'm not sure this is really what you want to do. Calling fl_copy_lock
>>>>> for a conflock looks relatively harmless for nfs and nlm. AFS though
>>>>> seems to add the lock to a list associated with the inode. That seems a
>>>>> little suspicious for a conflock and could be problematic. It may be
>>>>> best to avoid dealing with fl_ops for a conflock.
>>>>>
>>>>> Also in the case of fcntl_getlk, the struct file_lock lives on the
>>>>> stack, and locks_release_private is never called on it. You'll need to
>>>>> audit all of the current callers of __locks_copy_lock to ensure that
>>>>> any resources you end up taking references on when copying the conflock
>>>>> are eventually released.
>>>>
>>>> Sorry for my no further think about it.
>>>> I will check that again next day.
>>>
>>> I think we should not change the logical of coping lock,
>>> leave fl_ops and fl_lmops as private data as right now.
> 
> Why not? I think we'd benefit from making conflock creation a more
> distinct operation.
> 
>>>
>>> I have plan to,
>>> 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
>>>    I think it should be a private data, am I right?
>>> 2. call locks_copy_private() coping private data specifically.
>>>    a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
>>>       to point whether coping private data.
>>>    b. hack the conflock's fl_flags to do the same thing as a,
>>>       adds FL_NEED_PRIV fl_flags only valid for conflock.
>>>
>>> I don't think 2.a is a nice resolve, because it changes the interface
>>> and many caller don't care the private data (I think contains fl_owner)
>>> for conflock except nfsd.
>>>
>>> So, I'd like *2.b*. A draft is appended as following,
>>
>> I'm so sorry for the first draft, there is a bug of it.
>> Please using the new draft.
>>
>> thanks,
>> Kinglong Mee
>>
> 
> Personally, I'm not a fan of the approach below. I don't think we need
> a new flag for this and it doesn't do anything to solve the problem
> where the lockowner could go away while you're operating on it in a
> conflock.

I just want fix the bug with minimal code change.
But, I known my fault now.

> 
> I think we need several smaller changes:
> 
> 1. Right now __locks_copy_lock is only used to copy conflocks. It would
> be good to rename that to something more distinct (i.e.
> locks_copy_conflock), to make it clear that we're generating a conflock
> there.
> 
> 2. Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are
> superfluous, since they are callbacks into the filesystem. There should
> be no need to bother the filesystem at all with info in a conflock.
> But, lock _ownership_ matters for conflocks and that's indicated by the
> fl_lmops. So you really do want to copy the fl_lmops for conflocks I
> think.
> 
> 3. Add the operations you added before to fl_lmops to copy and release
> the owner (maybe even rename them lm_get_owner/lm_put_owner?), and
> ensure that the places that copy conflocks call those operations
> appropriately.
> 
> That should be all that's required here.

I think I know what you are advice.
I will send an update patch as v4.

thanks,
Kinglong Mee

[PATCH 4/5 v3] NFSD: New helper nfs4_get_stateowner() for atomic_inc reference

[Kinglong Mee]

v3: new patch isn't exist in v2/v1.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 98edf97..e087a71 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -216,6 +216,13 @@ static void nfsd4_put_session(struct nfsd4_session *ses)
 	spin_unlock(&nn->client_lock);
 }
 
+static inline struct nfs4_stateowner *
+nfs4_get_stateowner(struct nfs4_stateowner *sop)
+{
+	atomic_inc(&sop->so_count);
+	return sop;
+}
+
 static int
 same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner)
 {
@@ -235,10 +242,8 @@ find_openstateowner_str_locked(unsigned int hashval, struct nfsd4_open *open,
 			    so_strhash) {
 		if (!so->so_is_open_owner)
 			continue;
-		if (same_owner_str(so, &open->op_owner)) {
-			atomic_inc(&so->so_count);
-			return openowner(so);
-		}
+		if (same_owner_str(so, &open->op_owner))
+			return openowner(nfs4_get_stateowner(so));
 	}
 	return NULL;
 }
@@ -1644,7 +1649,7 @@ __destroy_client(struct nfs4_client *clp)
 	}
 	while (!list_empty(&clp->cl_openowners)) {
 		oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
-		atomic_inc(&oo->oo_owner.so_count);
+		nfs4_get_stateowner(&oo->oo_owner);
 		release_openowner(oo);
 	}
 	nfsd4_shutdown_callback(clp);
@@ -3125,8 +3130,7 @@ static void nfsd4_cstate_assign_replay(struct nfsd4_compound_state *cstate,
 {
 	if (!nfsd4_has_session(cstate)) {
 		mutex_lock(&so->so_replay.rp_mutex);
-		cstate->replay_owner = so;
-		atomic_inc(&so->so_count);
+		cstate->replay_owner = nfs4_get_stateowner(so);
 	}
 }
 
@@ -3225,8 +3229,7 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
 	atomic_inc(&stp->st_stid.sc_count);
 	stp->st_stid.sc_type = NFS4_OPEN_STID;
 	INIT_LIST_HEAD(&stp->st_locks);
-	stp->st_stateowner = &oo->oo_owner;
-	atomic_inc(&stp->st_stateowner->so_count);
+	stp->st_stateowner = nfs4_get_stateowner(&oo->oo_owner);
 	get_nfs4_file(fp);
 	stp->st_stid.sc_file = fp;
 	stp->st_access_bmap = 0;
@@ -4914,10 +4917,8 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
 			    so_strhash) {
 		if (so->so_is_open_owner)
 			continue;
-		if (!same_owner_str(so, owner))
-			continue;
-		atomic_inc(&so->so_count);
-		return lockowner(so);
+		if (same_owner_str(so, owner))
+			return lockowner(nfs4_get_stateowner(so));
 	}
 	return NULL;
 }
@@ -4996,8 +4997,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
 
 	atomic_inc(&stp->st_stid.sc_count);
 	stp->st_stid.sc_type = NFS4_LOCK_STID;
-	stp->st_stateowner = &lo->lo_owner;
-	atomic_inc(&lo->lo_owner.so_count);
+	stp->st_stateowner = nfs4_get_stateowner(&lo->lo_owner);
 	get_nfs4_file(fp);
 	stp->st_stid.sc_file = fp;
 	stp->st_stid.sc_free = nfs4_free_lock_stateid;
@@ -5539,7 +5539,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 			}
 		}
 
-		atomic_inc(&sop->so_count);
+		nfs4_get_stateowner(sop);
 		break;
 	}
 	spin_unlock(&clp->cl_lock);
-- 
1.9.3