[PATCH 2/5] POSIX Locking fixes. Take 2...

[PATCH 2/5] POSIX Locking fixes. Take 2...

[Trond Myklebust]

VFS: More extensive fix to locking code. Add file_lock_operations to
deal with copy/release of private data in the file_lock->fl_u field. Add
filesystem hooks for steal_locks(): changing the lockowner is hardly a
supported concept in most file locking protocols.

 fs/locks.c         |   40 ++++++++++++++++++++++++++++++----------
 include/linux/fs.h |    8 ++++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff -u --recursive --new-file --show-c-function linux-2.6.7-01-fix_locks/fs/locks.c linux-2.6.7-02-fix_locks2/fs/locks.c
--- linux-2.6.7-01-fix_locks/fs/locks.c	2004-06-28 21:47:10.000000000 -0400
+++ linux-2.6.7-02-fix_locks2/fs/locks.c	2004-06-29 16:03:14.000000000 -0400
@@ -167,6 +167,11 @@ static inline void locks_free_lock(struc
 	if (!list_empty(&fl->fl_link))
 		panic("Attempting to free lock on active lock list");
 
+	if (fl->fl_ops && fl->fl_ops->fl_release_private) {
+		fl->fl_ops->fl_release_private(fl);
+		fl->fl_ops = NULL;
+	}
+
 	kmem_cache_free(filelock_cache, fl);
 }
 
@@ -186,6 +191,7 @@ void locks_init_lock(struct file_lock *f
 	fl->fl_notify = NULL;
 	fl->fl_insert = NULL;
 	fl->fl_remove = NULL;
+	fl->fl_ops = NULL;
 }
 
 EXPORT_SYMBOL(locks_init_lock);
@@ -220,7 +226,9 @@ void locks_copy_lock(struct file_lock *n
 	new->fl_notify = fl->fl_notify;
 	new->fl_insert = fl->fl_insert;
 	new->fl_remove = fl->fl_remove;
-	new->fl_u = fl->fl_u;
+	new->fl_ops = fl->fl_ops;
+	if (fl->fl_ops && fl->fl_ops->fl_copy_lock)
+		fl->fl_ops->fl_copy_lock(new, fl);
 }
 
 EXPORT_SYMBOL(locks_copy_lock);
@@ -324,6 +332,7 @@ static int flock_to_posix_lock(struct fi
 	fl->fl_notify = NULL;
 	fl->fl_insert = NULL;
 	fl->fl_remove = NULL;
+	fl->fl_ops = NULL;
 
 	return assign_type(fl, l->l_type);
 }
@@ -364,6 +373,7 @@ static int flock64_to_posix_lock(struct 
 	fl->fl_notify = NULL;
 	fl->fl_insert = NULL;
 	fl->fl_remove = NULL;
+	fl->fl_ops = NULL;
 
 	switch (l->l_type) {
 	case F_RDLCK:
@@ -400,6 +410,7 @@ static int lease_alloc(struct file *filp
 	fl->fl_notify = NULL;
 	fl->fl_insert = NULL;
 	fl->fl_remove = NULL;
+	fl->fl_ops = NULL;
 
 	*flp = fl;
 	return 0;
@@ -419,10 +430,9 @@ static inline int locks_overlap(struct f
 static inline int
 posix_same_owner(struct file_lock *fl1, struct file_lock *fl2)
 {
-	/* FIXME: Replace this sort of thing with struct file_lock_operations */
-	if ((fl1->fl_type | fl2->fl_type) & FL_LOCKD)
-		return fl1->fl_owner == fl2->fl_owner &&
-			fl1->fl_pid == fl2->fl_pid;
+	if (fl1->fl_ops && fl1->fl_ops->fl_compare_owner)
+		return fl2->fl_ops == fl1->fl_ops &&
+			fl1->fl_ops->fl_compare_owner(fl1, fl2);
 	return fl1->fl_owner == fl2->fl_owner;
 }
 
@@ -981,6 +991,8 @@ int locks_mandatory_area(int read_write,
 		break;
 	}
 
+	if (fl.fl_ops && fl.fl_ops->fl_release_private)
+		fl.fl_ops->fl_release_private(&fl);
 	return error;
 }
 
@@ -1415,7 +1427,6 @@ int fcntl_getlk(struct file *filp, struc
 	error = -EFAULT;
 	if (!copy_to_user(l, &flock, sizeof(flock)))
 		error = 0;
-  
 out:
 	return error;
 }
@@ -1665,6 +1676,7 @@ void locks_remove_posix(struct file *fil
 	lock.fl_owner = owner;
 	lock.fl_pid = current->tgid;
 	lock.fl_file = filp;
+	lock.fl_ops = NULL;
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		filp->f_op->lock(filp, F_SETLK, &lock);
@@ -1684,6 +1696,8 @@ void locks_remove_posix(struct file *fil
 		before = &fl->fl_next;
 	}
 	unlock_kernel();
+	if (lock.fl_ops && lock.fl_ops->fl_release_private)
+		lock.fl_ops->fl_release_private(&lock);
 }
 
 EXPORT_SYMBOL(locks_remove_posix);
@@ -1978,12 +1992,18 @@ EXPORT_SYMBOL(lock_may_write);
 static inline void __steal_locks(struct file *file, fl_owner_t from)
 {
 	struct inode *inode = file->f_dentry->d_inode;
-	struct file_lock *fl = inode->i_flock;
+	struct file_lock *fl;
 
-	while (fl) {
-		if (fl->fl_file == file && fl->fl_owner == from)
+restart:
+	for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) {
+		if (fl->fl_file == file && fl->fl_owner == from) {
+			if (fl->fl_ops && fl->fl_ops->fl_steal_locks) {
+				fl->fl_ops->fl_steal_locks(fl, from);
+				/* Some filesystems may just drop the lock */
+				goto restart;
+			}
 			fl->fl_owner = current->files;
-		fl = fl->fl_next;
+		}
 	}
 }
 
diff -u --recursive --new-file --show-c-function linux-2.6.7-01-fix_locks/include/linux/fs.h linux-2.6.7-02-fix_locks2/include/linux/fs.h
--- linux-2.6.7-01-fix_locks/include/linux/fs.h	2004-06-28 21:45:04.000000000 -0400
+++ linux-2.6.7-02-fix_locks2/include/linux/fs.h	2004-06-28 21:47:13.000000000 -0400
@@ -622,6 +622,13 @@ extern void close_private_file(struct fi
  */
 typedef struct files_struct *fl_owner_t;
 
+struct file_lock_operations {
+	int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
+	void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
+	void (*fl_release_private)(struct file_lock *);
+	void (*fl_steal_locks)(struct file_lock *, fl_owner_t);
+};
+
 /* that will die - we need it for nfs_lock_info */
 #include <linux/nfs_fs_i.h>
 
@@ -645,6 +652,7 @@ struct file_lock {
 	struct fasync_struct *	fl_fasync; /* for lease break notifications */
 	unsigned long fl_break_time;	/* for nonblocking lease breaks */
 
+	struct file_lock_operations *fl_ops;	/* Callbacks for lockmanagers */
 	union {
 		struct nfs_lock_info	nfs_fl;
 	} fl_u;

Re: [PATCH 2/5] POSIX Locking fixes. Take 2...

[Matthew Wilcox]

On Tue, Jun 29, 2004 at 08:29:09PM -0400, Trond Myklebust wrote:
> VFS: More extensive fix to locking code. Add file_lock_operations to
> deal with copy/release of private data in the file_lock->fl_u field. Add
> filesystem hooks for steal_locks(): changing the lockowner is hardly a
> supported concept in most file locking protocols.

The thing that got me most confused when I was trying to rip fl_notify,
fl_insert and fl_remove out of the existing struct file_lock was that one
of them is "owned" by the host fs, and two are owned by the exporting fs.
Or was it the other way around... anyway, which is this new f_l_ops for,
and can fl_notify or fl_insert/fl_remove move into it?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH 2/5] POSIX Locking fixes. Take 2...

[Trond Myklebust]

På ty , 29/06/2004 klokka 20:54, skreiv Matthew Wilcox:

> The thing that got me most confused when I was trying to rip fl_notify,
> fl_insert and fl_remove out of the existing struct file_lock was that one
> of them is "owned" by the host fs, and two are owned by the exporting fs.
> Or was it the other way around... anyway, which is this new f_l_ops for,
> and can fl_notify or fl_insert/fl_remove move into it?

fl_ops is a hook to allow the VFS to call back the real lock manager
whoever that may be.

        As far as NFS is concerned, the VFS is just a local scoreboard
        for keeping track of locks that we may need to clean up on file
        close() or reclaim in case of a server reboot (afaik this is
        pretty much all CIFS and GFS care about too - I've no idea about
        the needs of Lustre and others).

        As far as lockd is concerned, the VFS representation of the lock
        has its usual local function, but lockd has its own private
        representation of the lockowner (it has a remote host + a remote
        pid). It does not care about the clean up on file close() stuff.
        In addition, it needs special notification in the case where one
        lock is blocking on another lock. If this confuses you, then
        blame it on the fact that the blocking code uses the exact same
        struct file_lock to represent the blocker.

If you prefer, we might split fl_ops + fl_notify/... into 2 structs:

struct file_lock_operations {
	void (*fl_insert)(struct file_lock *);
	void (*fl_remove)(struct file_lock *);
	void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
	void (*fl_release_private)(struct file_lock *);
	void (*fl_steal_locks)(struct file_lock *, fl_owner_t);
};

struct lock_manager_operations {
	int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
	void (*fl_notify)(struct file_lock *);
};

That would in effect allow us to keep the lockd and the filesystem stuff
separate: one in file_lock->fl_ops, the other in file_lock->fl_lmops.

Cheers,
  Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html