lock_operations

lock_operations

[Matthew Wilcox]

So... switching locks_remove_posix to use the unlock call was Bad, Evil
and Wrong.  But I still really want the underlying filesystem to know
about what's coming since I don't want it to sleep in fl_remove.

Comments on this patch?

 - introduces lock_operations
 - splits old f_op->lock method into l_op->set_lock and ->get_lock
 - moves fl->fl_insert and fl_remove methods to l_op->lock_insert and
   l_op->lock_remove
 - introduces remove_posix() and remove_flock().

I've fixed up NFS to the point where it will compile.  I suspect that it's
possible to simplify NFS from here, but that would only clutter this patch.

I think the `nfsd over a clusterfs' problem is solvable with this
infrastructure.  Introduce a new method that takes whatever information
(a pointer to a struct host, i guess?) the fs needs to construct a
meaningful owner field.  But that's a patch for another day ...

diff -urpNX dontdiff linux-2.5.48/fs/lockd/clntproc.c linux-2.5.48-flock/fs/lockd/clntproc.c
--- linux-2.5.48/fs/lockd/clntproc.c	2002-11-17 23:29:21.000000000 -0500
+++ linux-2.5.48-flock/fs/lockd/clntproc.c	2002-11-19 18:38:39.000000000 -0500
@@ -462,8 +462,6 @@ nlmclnt_lock(struct nlm_rqst *req, struc
 		fl->fl_u.nfs_fl.state = host->h_state;
 		fl->fl_u.nfs_fl.flags |= NFS_LCK_GRANTED;
 		fl->fl_u.nfs_fl.host = host;
-		fl->fl_insert = nlmclnt_insert_lock_callback;
-		fl->fl_remove = nlmclnt_remove_lock_callback;
 	}
 
 	return nlm_stat_to_errno(resp->status);
@@ -689,3 +687,30 @@ nlm_stat_to_errno(u32 status)
 	printk(KERN_NOTICE "lockd: unexpected server status %d\n", status);
 	return -ENOLCK;
 }
+
+static int nlmclnt_set_lock(struct file *filp, struct file_lock *fl)
+{
+	if (fl->fl_flags & FL_SLEEP) {
+		nfs_lock(filp, F_SETLKW, fl);
+	} else {
+		nfs_lock(filp, F_SETLK, fl);
+	}
+}
+
+static int nlmclnt_get_lock(struct file *filp, struct file_lock *fl)
+{
+	nfs_lock(filp, F_GETLK, fl);
+}
+
+static int nlmclnt_remove_posix(struct file *filp, fl_owner_t id)
+{
+	printk("remove posix support not yet implemented\n");
+}
+
+struct lock_operations nfs_client_lops = {
+	.set_lock =	nlmclnt_set_lock,
+	.get_lock =	nlmclnt_get_lock,
+	.remove_posix =	nlmclnt_remove_posix,
+	.lock_insert =	nlmclnt_insert_lock_callback,
+	.lock_remove =	nlmclnt_remove_lock_callback,
+};
diff -urpNX dontdiff linux-2.5.48/fs/locks.c linux-2.5.48-flock/fs/locks.c
--- linux-2.5.48/fs/locks.c	2002-11-17 23:29:56.000000000 -0500
+++ linux-2.5.48-flock/fs/locks.c	2002-11-19 19:31:23.000000000 -0500
@@ -187,8 +187,6 @@ void locks_init_lock(struct file_lock *f
 	fl->fl_type = 0;
 	fl->fl_start = fl->fl_end = 0;
 	fl->fl_notify = NULL;
-	fl->fl_insert = NULL;
-	fl->fl_remove = NULL;
 }
 
 /*
@@ -219,8 +217,6 @@ void locks_copy_lock(struct file_lock *n
 	new->fl_start = fl->fl_start;
 	new->fl_end = fl->fl_end;
 	new->fl_notify = fl->fl_notify;
-	new->fl_insert = fl->fl_insert;
-	new->fl_remove = fl->fl_remove;
 	new->fl_u = fl->fl_u;
 }
 
@@ -312,8 +308,6 @@ static int flock_to_posix_lock(struct fi
 	fl->fl_file = filp;
 	fl->fl_flags = FL_POSIX;
 	fl->fl_notify = NULL;
-	fl->fl_insert = NULL;
-	fl->fl_remove = NULL;
 
 	return assign_type(fl, l->l_type);
 }
@@ -352,8 +346,6 @@ static int flock64_to_posix_lock(struct 
 	fl->fl_file = filp;
 	fl->fl_flags = FL_POSIX;
 	fl->fl_notify = NULL;
-	fl->fl_insert = NULL;
-	fl->fl_remove = NULL;
 
 	switch (l->l_type) {
 	case F_RDLCK:
@@ -388,8 +380,6 @@ static int lease_alloc(struct file *filp
 	fl->fl_start = 0;
 	fl->fl_end = OFFSET_MAX;
 	fl->fl_notify = NULL;
-	fl->fl_insert = NULL;
-	fl->fl_remove = NULL;
 
 	*flp = fl;
 	return 0;
@@ -465,14 +455,17 @@ static void locks_wake_up_blocks(struct 
  */
 static void locks_insert_lock(struct file_lock **pos, struct file_lock *fl)
 {
+	struct lock_operations *l_op = NULL;
 	list_add(&fl->fl_link, &file_lock_list);
 
 	/* insert into file's list */
 	fl->fl_next = *pos;
 	*pos = fl;
 
-	if (fl->fl_insert)
-		fl->fl_insert(fl);
+	if (fl->fl_file->f_op)
+		l_op = fl->fl_file->f_op->lock;
+	if (l_op && l_op->lock_insert)
+		l_op->lock_insert(fl);
 }
 
 /*
@@ -484,6 +477,7 @@ static void locks_insert_lock(struct fil
 static void locks_delete_lock(struct file_lock **thisfl_p)
 {
 	struct file_lock *fl = *thisfl_p;
+	struct lock_operations *l_op = NULL;
 
 	*thisfl_p = fl->fl_next;
 	fl->fl_next = NULL;
@@ -495,8 +489,10 @@ static void locks_delete_lock(struct fil
 		fl->fl_fasync = NULL;
 	}
 
-	if (fl->fl_remove)
-		fl->fl_remove(fl);
+	if (fl->fl_file->f_op)
+		l_op = fl->fl_file->f_op->lock;
+	if (l_op && l_op->lock_remove)
+		l_op->lock_remove(fl);
 
 	locks_wake_up_blocks(fl);
 	locks_free_lock(fl);
@@ -1324,6 +1320,7 @@ asmlinkage long sys_flock(unsigned int f
 int fcntl_getlk(struct file *filp, struct flock *l)
 {
 	struct file_lock *fl, file_lock;
+	struct lock_operations *l_op = NULL;
 	struct flock flock;
 	int error;
 
@@ -1338,15 +1335,13 @@ int fcntl_getlk(struct file *filp, struc
 	if (error)
 		goto out;
 
-	if (filp->f_op && filp->f_op->lock) {
-		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+	if (filp->f_op)
+		l_op = filp->f_op->lock;
+	if (l_op && l_op->get_lock) {
+		error = l_op->get_lock(filp, &file_lock);
 		if (error < 0)
 			goto out;
-		else if (error == LOCK_USE_CLNT)
-		  /* Bypass for NFS with no locking - 2.0.36 compat */
-		  fl = posix_test_lock(filp, &file_lock);
-		else
-		  fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
+		fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
 	} else {
 		fl = posix_test_lock(filp, &file_lock);
 	}
@@ -1380,19 +1375,48 @@ out:
 	return error;
 }
 
+int vfs_setlock_posix(struct file *filp, struct file_lock *fl)
+{
+	struct lock_operations *l_op = NULL;
+	int error;
+
+	error = security_ops->file_lock(filp, fl->fl_type);
+	if (error)
+		return error;
+
+	if (filp->f_op)
+		l_op = filp->f_op->lock;
+	if (l_op && l_op->set_lock)
+		return l_op->set_lock(filp, fl);
+
+	for (;;) {
+		error = posix_lock_file(filp, fl);
+		if ((error != -EAGAIN) || !(fl->fl_flags & FL_SLEEP))
+			break;
+		error = wait_event_interruptible(fl->fl_wait,
+				!fl->fl_next);
+		if (!error)
+			continue;
+
+		lock_kernel();
+		locks_delete_block(fl);
+		unlock_kernel();
+		break;
+	}
+
+	return error;
+}
+
 /* Apply the lock described by l to an open file descriptor.
  * This implements both the F_SETLK and F_SETLKW commands of fcntl().
  */
 int fcntl_setlk(struct file *filp, unsigned int cmd, struct flock *l)
 {
-	struct file_lock *file_lock = locks_alloc_lock(0);
+	struct file_lock file_lock;
 	struct flock flock;
 	struct inode *inode;
 	int error;
 
-	if (file_lock == NULL)
-		return -ENOLCK;
-
 	/*
 	 * This might block, so we do it before checking the inode.
 	 */
@@ -1417,11 +1441,12 @@ int fcntl_setlk(struct file *filp, unsig
 	}
 #endif
 
-	error = flock_to_posix_lock(filp, file_lock, &flock);
+	locks_init_lock(&file_lock);
+	error = flock_to_posix_lock(filp, &file_lock, &flock);
 	if (error)
 		goto out;
 	if (cmd == F_SETLKW) {
-		file_lock->fl_flags |= FL_SLEEP;
+		file_lock.fl_flags |= FL_SLEEP;
 	}
 	
 	error = -EBADF;
@@ -1441,33 +1466,9 @@ int fcntl_setlk(struct file *filp, unsig
 		goto out;
 	}
 
-	error = security_ops->file_lock(filp, file_lock->fl_type);
-	if (error)
-		goto out;
-
-	if (filp->f_op && filp->f_op->lock != NULL) {
-		error = filp->f_op->lock(filp, cmd, file_lock);
-		if (error < 0)
-			goto out;
-	}
-
-	for (;;) {
-		error = posix_lock_file(filp, file_lock);
-		if ((error != -EAGAIN) || (cmd == F_SETLK))
-			break;
-		error = wait_event_interruptible(file_lock->fl_wait,
-				!file_lock->fl_next);
-		if (!error)
-			continue;
-
-		lock_kernel();
-		locks_delete_block(file_lock);
-		unlock_kernel();
-		break;
-	}
+	error = vfs_setlock_posix(filp, &file_lock);
 
  out:
-	locks_free_lock(file_lock);
 	return error;
 }
 
@@ -1478,6 +1479,7 @@ int fcntl_setlk(struct file *filp, unsig
 int fcntl_getlk64(struct file *filp, struct flock64 *l)
 {
 	struct file_lock *fl, file_lock;
+	struct lock_operations *l_op = NULL;
 	struct flock64 flock;
 	int error;
 
@@ -1492,15 +1494,13 @@ int fcntl_getlk64(struct file *filp, str
 	if (error)
 		goto out;
 
-	if (filp->f_op && filp->f_op->lock) {
-		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
+	if (filp->f_op)
+		l_op = filp->f_op->lock;
+	if (l_op && l_op->get_lock) {
+		error = l_op->get_lock(filp, &file_lock);
 		if (error < 0)
 			goto out;
-		else if (error == LOCK_USE_CLNT)
-		  /* Bypass for NFS with no locking - 2.0.36 compat */
-		  fl = posix_test_lock(filp, &file_lock);
-		else
-		  fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
+		fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
 	} else {
 		fl = posix_test_lock(filp, &file_lock);
 	}
@@ -1527,14 +1527,11 @@ out:
  */
 int fcntl_setlk64(struct file *filp, unsigned int cmd, struct flock64 *l)
 {
-	struct file_lock *file_lock = locks_alloc_lock(0);
+	struct file_lock file_lock;
 	struct flock64 flock;
 	struct inode *inode;
 	int error;
 
-	if (file_lock == NULL)
-		return -ENOLCK;
-
 	/*
 	 * This might block, so we do it before checking the inode.
 	 */
@@ -1544,6 +1541,7 @@ int fcntl_setlk64(struct file *filp, uns
 
 	inode = filp->f_dentry->d_inode;
 
+#ifdef CONFIG_MMU
 	/* Don't allow mandatory locks on files that may be memory mapped
 	 * and shared.
 	 */
@@ -1556,12 +1554,13 @@ int fcntl_setlk64(struct file *filp, uns
 			goto out;
 		}
 	}
+#endif
 
-	error = flock64_to_posix_lock(filp, file_lock, &flock);
+	error = flock64_to_posix_lock(filp, &file_lock, &flock);
 	if (error)
 		goto out;
 	if (cmd == F_SETLKW64) {
-		file_lock->fl_flags |= FL_SLEEP;
+		file_lock.fl_flags |= FL_SLEEP;
 	}
 	
 	error = -EBADF;
@@ -1581,33 +1580,9 @@ int fcntl_setlk64(struct file *filp, uns
 		goto out;
 	}
 
-	error = security_ops->file_lock(filp, file_lock->fl_type);
-	if (error)
-		goto out;
-
-	if (filp->f_op && filp->f_op->lock != NULL) {
-		error = filp->f_op->lock(filp, cmd, file_lock);
-		if (error < 0)
-			goto out;
-	}
-
-	for (;;) {
-		error = posix_lock_file(filp, file_lock);
-		if ((error != -EAGAIN) || (cmd == F_SETLK64))
-			break;
-		error = wait_event_interruptible(file_lock->fl_wait,
-				!file_lock->fl_next);
-		if (!error)
-			continue;
-
-		lock_kernel();
-		locks_delete_block(file_lock);
-		unlock_kernel();
-		break;
-	}
+	error = vfs_setlock_posix(filp, &file_lock);
 
-out:
-	locks_free_lock(file_lock);
+ out:
 	return error;
 }
 #endif /* BITS_PER_LONG == 32 */
@@ -1619,30 +1594,34 @@ out:
  */
 void locks_remove_posix(struct file *filp, fl_owner_t owner)
 {
-	struct file_lock lock;
+	struct file_lock *fl, **before;
+	struct lock_operations *l_op = NULL;
+	struct inode *inode = filp->f_dentry->d_inode;
 
 	/*
-	 * If there are no locks held on this file, we don't need to call
-	 * posix_lock_file().  Another process could be setting a lock on this
-	 * file at the same time, but we wouldn't remove that lock anyway.
+	 * If there are no locks held on this file, we don't need to do
+	 * anything.  Another thread could be setting a lock on this
+	 * file at the same time, but it's a race we just won.
 	 */
-	if (!filp->f_dentry->d_inode->i_flock)
+	if (!inode->i_flock)
 		return;
 
-	lock.fl_type = F_UNLCK;
-	lock.fl_flags = FL_POSIX;
-	lock.fl_start = 0;
-	lock.fl_end = OFFSET_MAX;
-	lock.fl_owner = owner;
-	lock.fl_pid = current->tgid;
-	lock.fl_file = filp;
-
-	if (filp->f_op && filp->f_op->lock != NULL) {
-		filp->f_op->lock(filp, F_SETLK, &lock);
-		/* Ignore any error -- we must remove the locks anyway */
+	if (filp->f_op)
+		l_op = filp->f_op->lock;
+	if (l_op && l_op->remove_posix) {
+		l_op->remove_posix(filp, owner);
+	} else {
+		lock_kernel();
+		before = &inode->i_flock;
+		while ((fl = *before) != NULL) {
+			if (IS_POSIX(fl) && fl->fl_owner == owner) {
+				locks_delete_lock(before);
+				continue;
+			}
+			before = &fl->fl_next;
+		}
+		unlock_kernel();
 	}
-
-	posix_lock_file(filp, &lock);
 }
 
 /*
diff -urpNX dontdiff linux-2.5.48/include/linux/fs.h linux-2.5.48-flock/include/linux/fs.h
--- linux-2.5.48/include/linux/fs.h	2002-11-17 23:29:29.000000000 -0500
+++ linux-2.5.48-flock/include/linux/fs.h	2002-11-19 19:24:41.000000000 -0500
@@ -540,8 +540,6 @@ struct file_lock {
 	loff_t fl_end;
 
 	void (*fl_notify)(struct file_lock *);	/* unblock callback */
-	void (*fl_insert)(struct file_lock *);	/* lock insertion callback */
-	void (*fl_remove)(struct file_lock *);	/* lock removal callback */
 
 	struct fasync_struct *	fl_fasync; /* for lease break notifications */
 	unsigned long fl_break_time;	/* for nonblocking lease breaks */
@@ -734,6 +732,42 @@ typedef struct {
 
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *, unsigned long, unsigned long);
 
+/**
+ * struct lock_operations - filesystem hooks for file locking
+ *
+ * This struct is a work in progress.  It is intended to be per-filesystem;
+ * indeed it could be part of f_ops were it not pure bloat for non-network
+ * filesystems.  I suspect lock_insert and lock_remove are now unnecessary,
+ * but need feedback from FS maintainers.
+ * 
+ * @set_lock:
+ *	Attempt to set a new lock.  BKL not held, may sleep.
+ * @get_lock:
+ *	Return any lock which would conflict with the incoming lock.
+ *	No locks held, may sleep.
+ * @remove_posix:
+ *	A process closed a file descriptor.  Any locks on this @filp owned
+ *	by @owner should be removed.  BKL not held, may sleep.
+ * @remove_flock:
+ *	This @filp has been closed.  All locks owned by this process should
+ *	be removed.  BKL not held, may sleep.
+ * @lock_insert:
+ *	Notification that @fl, which was previously blocked, is now being
+ *	inserted.  BKL might not be held.  Must not sleep.
+ * @lock_remove:
+ *	Notification that @fl, which was an active lock, is now being
+ *	removed from the @filp.  BKL might not be held.  Must not sleep.
+ */
+
+struct lock_operations {
+	int (*set_lock) (struct file *filp, struct file_lock *fl);
+	int (*get_lock) (struct file *filp, struct file_lock *fl);
+	void (*remove_posix) (struct file *filp, fl_owner_t owner);
+	void (*remove_flock) (struct file *filp);
+	void (*lock_insert) (struct file_lock *fl);
+	void (*lock_remove) (struct file_lock *fl);
+};
+
 /*
  * NOTE:
  * read, write, poll, fsync, readv, writev can be called
@@ -757,7 +791,7 @@ struct file_operations {
 	int (*fsync) (struct file *, struct dentry *, int datasync);
 	int (*aio_fsync) (struct kiocb *, int datasync);
 	int (*fasync) (int, struct file *, int);
-	int (*lock) (struct file *, int, struct file_lock *);
+	struct lock_operations *lock;
 	ssize_t (*readv) (struct file *, const struct iovec *, unsigned long, loff_t *);
 	ssize_t (*writev) (struct file *, const struct iovec *, unsigned long, loff_t *);
 	ssize_t (*sendfile) (struct file *, loff_t *, size_t, read_actor_t, void *);

-- 
Revolutions do not require corporate support.

Re: lock_operations

[Trond Myklebust]

   > Comments on this patch?

nfs_client_lops does not really belong in fs/lockd/clntproc.c. That
introduces a module dependency between lockd and the NFS client code
that may be inappropriate if the user just wants to run a pure NFS
server.

Cheers,
  Trond

Re: lock_operations

[Matthew Wilcox]

On Wed, Nov 20, 2002 at 06:54:08PM +0100, Trond Myklebust wrote:
> nfs_client_lops does not really belong in fs/lockd/clntproc.c. That
> introduces a module dependency between lockd and the NFS client code
> that may be inappropriate if the user just wants to run a pure NFS
> server.

fair point.  in terms of the architecture though, you see no problems?

-- 
Revolutions do not require corporate support.

Re: lock_operations

[Trond Myklebust]

>>>>> " " == Matthew Wilcox <willy@debian.org> writes:

     > fair point.  in terms of the architecture though, you see no
     > problems?

Not at the moment. I'm a bit curious, though, as to why you chose to
split out get_lock() as a separate callback method, but not
unlock(). Isn't that just an optimization?

Cheers,
  Trond

Re: lock_operations

[Matthew Wilcox]

On Wed, Nov 20, 2002 at 08:04:59PM +0100, Trond Myklebust wrote:
> Not at the moment. I'm a bit curious, though, as to why you chose to
> split out get_lock() as a separate callback method, but not
> unlock(). Isn't that just an optimization?

i see get_lock as a fundamentally different operation to set_lock.  for
one thing it returns a lock which set_lock doesn't.

unlock is the same as setting a lock -- we have to deal with the same
broken posix behaviours of removing overlapping locks that we own.

yeah, they're different NLM calls, but that doesn't seem to be a big problem.

-- 
Revolutions do not require corporate support.