Trond--you mentioned noticing some locks_copy_locks() abuses; is this
what you were thinking of?
--b.
commit 07c5abe6899d8ab49368604e64894821d4f60bf9
Author: J. Bruce Fields <[email protected]>
Date: Thu Apr 24 10:08:22 2008 -0400
locks: don't call ->copy_lock methods on return of conflicting locks
The file_lock structure is used both as a heavy-weight representation of
an active lock, with pointers to reference-counted structures, etc., and
as a simple container for parameters that describe a file lock.
The conflicting lock returned from __posix_lock_file is an example of
the latter; so don't call the filesystem or lock manager callbacks when
copying to it. This also saves the need for an unnecessary
locks_init_lock in the nfsv4 server.
Thanks to Trond for pointing out the error.
Signed-off-by: J. Bruce Fields <[email protected]>
Cc: Trond Myklebust <[email protected]>
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 1f122c1..4d81553 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -632,7 +632,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 2e0fa66..e1ea2fe 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -224,7 +224,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
/*
* Initialize a new lock from an existing file_lock structure.
*/
-static void __locks_copy_lock(struct file_lock *new, const 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_pid = fl->fl_pid;
@@ -833,7 +833,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/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 55dfdd7..8799b87 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2712,9 +2712,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* Note: locks.c uses the BKL to protect the inode's lock list.
*/
- /* XXX?: Just to divert the locks_release_private at the start of
- * locks_copy_lock: */
- locks_init_lock(&conflock);
err = vfs_lock_file(filp, cmd, &file_lock, &conflock);
switch (-err) {
case 0: /* success! */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index cc2be2c..6556f2f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -973,6 +973,7 @@ extern int do_sync_mapping_range(struct address_space *mapping, loff_t offset,
/* fs/locks.c */
extern void locks_init_lock(struct file_lock *);
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_flock(struct file *);
extern void posix_test_lock(struct file *, struct file_lock *);
And apologies for that subject line. It was a placeholder I forgot to
replace....
On Thu, Apr 24, 2008 at 04:34:03PM -0400, J. Bruce Fields wrote:
> Trond--you mentioned noticing some locks_copy_locks() abuses; is this
> what you were thinking of?
>
> --b.
>
> commit 07c5abe6899d8ab49368604e64894821d4f60bf9
> Author: J. Bruce Fields <[email protected]>
> Date: Thu Apr 24 10:08:22 2008 -0400
>
> locks: don't call ->copy_lock methods on return of conflicting locks
>
> The file_lock structure is used both as a heavy-weight representation of
> an active lock, with pointers to reference-counted structures, etc., and
> as a simple container for parameters that describe a file lock.
>
> The conflicting lock returned from __posix_lock_file is an example of
> the latter; so don't call the filesystem or lock manager callbacks when
> copying to it. This also saves the need for an unnecessary
> locks_init_lock in the nfsv4 server.
>
> Thanks to Trond for pointing out the error.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> Cc: Trond Myklebust <[email protected]>
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 1f122c1..4d81553 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -632,7 +632,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 2e0fa66..e1ea2fe 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -224,7 +224,7 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
> /*
> * Initialize a new lock from an existing file_lock structure.
> */
> -static void __locks_copy_lock(struct file_lock *new, const 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_pid = fl->fl_pid;
> @@ -833,7 +833,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/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 55dfdd7..8799b87 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2712,9 +2712,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> * Note: locks.c uses the BKL to protect the inode's lock list.
> */
>
> - /* XXX?: Just to divert the locks_release_private at the start of
> - * locks_copy_lock: */
> - locks_init_lock(&conflock);
> err = vfs_lock_file(filp, cmd, &file_lock, &conflock);
> switch (-err) {
> case 0: /* success! */
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index cc2be2c..6556f2f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -973,6 +973,7 @@ extern int do_sync_mapping_range(struct address_space *mapping, loff_t offset,
> /* fs/locks.c */
> extern void locks_init_lock(struct file_lock *);
> 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_flock(struct file *);
> extern void posix_test_lock(struct file *, struct file_lock *);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html