2007-02-03 05:34:15

by J.Bruce Fields

[permalink] [raw]
Subject: [PATCH 2/14] locks: factor out generic/filesystem switch from test_lock

From: J. Bruce Fields <[email protected]> - unquoted

Factor out the code that switches between generic and filesystem-specific lock
methods; eventually we want to call this from lock managers (lockd and nfsd)
too; currently they only call the generic methods.

This patch does that for test_lock.

Note that this hasn't been necessary until recently, because the few
filesystems that define ->lock() (nfs, cifs...) aren't exportable via NFS.
However GFS (and, in the future, other cluster filesystems) need to implement
their own locking to get cluster-coherent locking, and also want to be able to
export locking to NFS (lockd and NFSv4).

So we accomplish this by factoring out code such as this and exporting it for
the use of lockd and nfsd.

Signed-off-by: "J. Bruce Fields" <[email protected]>
---
fs/locks.c | 59 ++++++++++++++++++++++++++-----------------
include/linux/lockd/bind.h | 2 +
2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 4390a95..95246dc 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1611,6 +1611,33 @@ asmlinkage long sys_flock(unsigned int fd, unsigned int cmd)
return error;
}

+/**
+ * vfs_test_lock - test file byte range lock
+ * @filp: The file to test lock for
+ * @fl: The lock to test
+ * @conf: Place to return a copy of the conflicting lock, if found
+ *
+ * Returns -ERRNO on failure. Indicates presence of conflicting lock by
+ * setting conf->fl_type to something other than F_UNLCK.
+ */
+int vfs_test_lock(struct file *filp, struct file_lock *fl, struct file_lock *conf)
+{
+ int error;
+
+ conf->fl_type = F_UNLCK;
+ if (filp->f_op && filp->f_op->lock) {
+ __locks_copy_lock(conf, fl);
+ error = filp->f_op->lock(filp, F_GETLK, conf);
+ if (conf->fl_ops && conf->fl_ops->fl_release_private)
+ conf->fl_ops->fl_release_private(conf);
+ return error;
+ } else {
+ posix_test_lock(filp, fl, conf);
+ return 0;
+ }
+}
+EXPORT_SYMBOL(vfs_test_lock);
+
/* Report the first existing lock that would conflict with l.
* This implements the F_GETLK command of fcntl().
*/
@@ -1631,17 +1658,10 @@ int fcntl_getlk(struct file *filp, struct flock __user *l)
if (error)
goto out;

- if (filp->f_op && filp->f_op->lock) {
- error = filp->f_op->lock(filp, F_GETLK, &file_lock);
- if (file_lock.fl_ops && file_lock.fl_ops->fl_release_private)
- file_lock.fl_ops->fl_release_private(&file_lock);
- if (error < 0)
- goto out;
- else
- fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
- } else {
- fl = (posix_test_lock(filp, &file_lock, &cfl) ? &cfl : NULL);
- }
+ error = vfs_test_lock(filp, &file_lock, &cfl);
+ if (error)
+ goto out;
+ fl = (cfl.fl_type == F_UNLCK ? NULL : &cfl);

flock.l_type = F_UNLCK;
if (fl != NULL) {
@@ -1785,18 +1805,11 @@ int fcntl_getlk64(struct file *filp, struct flock64 __user *l)
if (error)
goto out;

- if (filp->f_op && filp->f_op->lock) {
- error = filp->f_op->lock(filp, F_GETLK, &file_lock);
- if (file_lock.fl_ops && file_lock.fl_ops->fl_release_private)
- file_lock.fl_ops->fl_release_private(&file_lock);
- if (error < 0)
- goto out;
- else
- fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
- } else {
- fl = (posix_test_lock(filp, &file_lock, &cfl) ? &cfl : NULL);
- }
-
+ error = vfs_test_lock(filp, &file_lock, &cfl);
+ if (error)
+ goto out;
+ fl = (cfl.fl_type == F_UNLCK ? NULL : &cfl);
+
flock.l_type = F_UNLCK;
if (fl != NULL) {
flock.l_pid = fl->fl_pid;
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 246de1d..f78b24a 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -38,4 +38,6 @@ extern int nlmclnt_proc(struct inode *, int, struct file_lock *);
extern int lockd_up(int proto);
extern void lockd_down(void);

+extern int vfs_test_lock(struct file *, struct file_lock *, struct file_lock *);
+
#endif /* LINUX_LOCKD_BIND_H */
--
1.5.0.rc1.g72fe


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2007-02-03 08:50:29

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/14] locks: factor out generic/filesystem switch from test_lock

> +int vfs_test_lock(struct file *filp, struct file_lock *fl, struct file_lock *conf)

Please make sure to add linebreaks after at most 80 characters.

> + error = vfs_test_lock(filp, &file_lock, &cfl);
> + if (error)
> + goto out;

> + fl = (cfl.fl_type == F_UNLCK ? NULL : &cfl);
> flock.l_type = F_UNLCK;
> if (fl != NULL) {

This code snippled is more than ugly. fl is only checked for equality
once so you should reformulate that check using the actual type check:

if (cfl.fl_type != F_UNLCK) {

That also allows you to move the

flock.l_type = fl->fl_type;

out of the if statement later on. In fact that copying out should
proably move into posix_lock_to_flock and posix_lock_to_fock64
helpers similar to the flock_to_posix_lock and flock64_to_posix_lock
helpers we have for the other way around.


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-02-04 01:48:44

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH 2/14] locks: factor out generic/filesystem switch from test_lock

On Sat, Feb 03, 2007 at 08:50:25AM +0000, Christoph Hellwig wrote:
> > +int vfs_test_lock(struct file *filp, struct file_lock *fl, struct file_lock *conf)
>
> Please make sure to add linebreaks after at most 80 characters.

OK, done.

>
> > + error = vfs_test_lock(filp, &file_lock, &cfl);
> > + if (error)
> > + goto out;
>
> > + fl = (cfl.fl_type == F_UNLCK ? NULL : &cfl);
> > flock.l_type = F_UNLCK;
> > if (fl != NULL) {
>
> This code snippled is more than ugly. fl is only checked for equality
> once so you should reformulate that check using the actual type check:
>
> if (cfl.fl_type != F_UNLCK) {
>
> That also allows you to move the
>
> flock.l_type = fl->fl_type;
>
> out of the if statement later on.

That's a good idea, thanks; done.

Actually, I wonder if there's any reason we couldn't also just give
posix_test_lock() the same interface as ->lock? (The latter uses the
same file_lock argument for the input and (in the case where it finds
a conflicting lock) the output, where the former uses an extra argument
to pass back the lock.) That'd make this a little simpler too.

> In fact that copying out should proably move into posix_lock_to_flock
> and posix_lock_to_fock64 helpers similar to the flock_to_posix_lock
> and flock64_to_posix_lock helpers we have for the other way around.

OK!--b.

2007-02-04 08:41:10

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 2/14] locks: factor out generic/filesystem switch from test_lock

On Sat, Feb 03, 2007 at 08:48:44PM -0500, J. Bruce Fields wrote:
> Actually, I wonder if there's any reason we couldn't also just give
> posix_test_lock() the same interface as ->lock? (The latter uses the
> same file_lock argument for the input and (in the case where it finds
> a conflicting lock) the output, where the former uses an extra argument
> to pass back the lock.) That'd make this a little simpler too.

yes, giving posix_test_lock the same interface as ->lock would be
even better.