| Message ID | 53DCF97D.3000605@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Sat, 02 Aug 2014 22:45:17 +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 > 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); > } 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!
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@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); > > } > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 5 Aug 2014 15:14:58 -0400 "J. Bruce Fields" <bfields@fieldses.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@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); > > > } > > > > 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.
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); }
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(-)