Re: nfs locking for cluster filesystems

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Apr 17, 2007 at 11:05:19AM -0700, Andrew Morton wrote:
> > On Tue, 17 Apr 2007 13:55:33 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > I've got an updated series addressing Christoph's comments (except that
> > it's still using FL_CANCEL instead of a ->cancel() file method).  I was
> > arguing with myself about what would be the least obnoxious way to send
> > the update:
> > 
> > 	- resend all 17?  (Seems like mild overkill.)
> > 	- resend just the last 10?  (The first 7 are identical to what's
> > 	  in -mm.)
> > 	- Send in an incremental patch?  (But I really wouldn't want
> > 	  them submitted to the kernel that way.)
> 
> Yes, there's no perfect approach here, if the changes are too large to
> permit the preferred approach of doing an incremental patch against each
> main patch.

The changes are small, but they touch several of the patches.  And I
reordered the patches a little too, just to add to the confusion.

> > 	- Ask you to fetch from the server-cluster-locking-api branch of
> > 		git://linux-nfs.org/~bfields/linux.git
> 
> Well if we're all now happy with the level of review then a git tree is OK
> by me.  I'll suck that tree down later in the week, will let you know if
> anything goes wrong.

Thanks, that's obviously convenient for me.  Christoph or Trond or
anyone, if you'd rather have this in email, let me know.

I've appended the total diff between two versions just in case anyone's
curious.

--b.

diff --git a/fs/gfs2/locking/dlm/plock.c b/fs/gfs2/locking/dlm/plock.c
index 1160cde..5207e4e 100644
--- a/fs/gfs2/locking/dlm/plock.c
+++ b/fs/gfs2/locking/dlm/plock.c
@@ -89,8 +89,8 @@ int gdlm_plock(void *lockspace, struct lm_lockname *name,
 	op->info.start		= fl->fl_start;
 	op->info.end		= fl->fl_end;
 	op->info.owner		= (__u64)(long) fl->fl_owner;
-	if (fl->fl_lmops && fl->fl_lmops->fl_notify) {
-		xop->callback	= fl->fl_lmops->fl_notify;
+	if (fl->fl_lmops && fl->fl_lmops->fl_grant) {
+		xop->callback	= fl->fl_lmops->fl_grant;
 		/* might need to make a copy */
 		xop->fl		= fl;
 		xop->file	= file;
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index a4828f2..4606651 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -266,8 +266,7 @@ static void nlmsvc_free_block(struct kref *kref)
 	nlmsvc_freegrantargs(block->b_call);
 	nlm_release_call(block->b_call);
 	nlm_release_file(block->b_file);
-	if (block->b_fl)
-		kfree(block->b_fl);
+	kfree(block->b_fl);
 	kfree(block);
 }
 
@@ -596,7 +595,7 @@ nlmsvc_cancel_blocked(struct nlm_file *file, struct nlm_lock *lock)
 
 /*
  * This is a callback from the filesystem for VFS file lock requests.
- * It will be used if fl_notify is defined and the filesystem can not
+ * It will be used if fl_grant is defined and the filesystem can not
  * respond to the request immediately.
  * For GETLK request it will copy the reply to the nlm_block.
  * For SETLK or SETLKW request it will get the local posix lock.
@@ -619,21 +618,12 @@ nlmsvc_update_deferred_block(struct nlm_block *block, struct file_lock *conf,
 	}
 }
 
-/*
- * Unblock a blocked lock request. This is a callback invoked from the
- * VFS layer when a lock on which we blocked is removed.
- *
- * This function doesn't grant the blocked lock instantly, but rather moves
- * the block to the head of nlm_blocked where it can be picked up by lockd.
- */
-static int
-nlmsvc_notify_blocked(struct file_lock *fl, struct file_lock *conf, int result)
+static int nlmsvc_grant_deferred(struct file_lock *fl, struct file_lock *conf,
+					int result)
 {
-	struct nlm_block	*block;
+	struct nlm_block *block;
 	int rc = -ENOENT;
 
-	dprintk("lockd: nlmsvc_notify_blocked lock %p conf %p result %d\n",
-							fl, conf, result);
 	lock_kernel();
 	list_for_each_entry(block, &nlm_blocked, b_list) {
 		if (nlm_compare_locks(&block->b_call->a_args.lock.fl, fl)) {
@@ -655,12 +645,35 @@ nlmsvc_notify_blocked(struct file_lock *fl, struct file_lock *conf, int result)
 		}
 	}
 	unlock_kernel();
-
 	if (rc == -ENOENT)
-		printk(KERN_WARNING "lockd: notification for unknown block!\n");
+		printk(KERN_WARNING "lockd: grant for unknown block\n");
 	return rc;
 }
 
+/*
+ * Unblock a blocked lock request. This is a callback invoked from the
+ * VFS layer when a lock on which we blocked is removed.
+ *
+ * This function doesn't grant the blocked lock instantly, but rather moves
+ * the block to the head of nlm_blocked where it can be picked up by lockd.
+ */
+static void
+nlmsvc_notify_blocked(struct file_lock *fl)
+{
+	struct nlm_block	*block;
+
+	dprintk("lockd: VFS unblock notification for block %p\n", fl);
+	list_for_each_entry(block, &nlm_blocked, b_list) {
+		if (nlm_compare_locks(&block->b_call->a_args.lock.fl, fl)) {
+			nlmsvc_insert_block(block, 0);
+			svc_wake_up(block->b_daemon);
+			return;
+		}
+	}
+
+	printk(KERN_WARNING "lockd: notification for unknown block!\n");
+}
+
 static int nlmsvc_same_owner(struct file_lock *fl1, struct file_lock *fl2)
 {
 	return fl1->fl_owner == fl2->fl_owner && fl1->fl_pid == fl2->fl_pid;
@@ -669,6 +682,7 @@ static int nlmsvc_same_owner(struct file_lock *fl1, struct file_lock *fl2)
 struct lock_manager_operations nlmsvc_lock_operations = {
 	.fl_compare_owner = nlmsvc_same_owner,
 	.fl_notify = nlmsvc_notify_blocked,
+	.fl_grant = nlmsvc_grant_deferred,
 };
 
 /*
diff --git a/fs/locks.c b/fs/locks.c
index 68ae4d4..13a3e02 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -544,7 +544,7 @@ static void locks_wake_up_blocks(struct file_lock *blocker)
 				struct file_lock, fl_block);
 		__locks_delete_block(waiter);
 		if (waiter->fl_lmops && waiter->fl_lmops->fl_notify)
-			waiter->fl_lmops->fl_notify(waiter, NULL, -EAGAIN);
+			waiter->fl_lmops->fl_notify(waiter);
 		else
 			wake_up(&waiter->fl_wait);
 	}
@@ -1696,10 +1696,10 @@ out:
  * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
  * locks, the ->lock() interface may return asynchronously, before the lock has
  * been granted or denied by the underlying filesystem, if (and only if)
- * fl_notify is set. Callers expecting ->lock() to return asynchronously
+ * fl_grant is set. Callers expecting ->lock() to return asynchronously
  * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
  * the request is for a blocking lock. When ->lock() does return asynchronously,
- * it must return -EINPROGRESS, and call ->fl_notify() when the lock
+ * it must return -EINPROGRESS, and call ->fl_grant() when the lock
  * request completes.
  * If the request is for non-blocking lock the file system should return
  * -EINPROGRESS then try to get the lock and call the callback routine with
@@ -1709,7 +1709,7 @@ out:
  * grants a lock so the VFS can find out which locks are locally held and do
  * the correct lock cleanup when required.
  * The underlying filesystem must not drop the kernel lock or call
- * ->fl_notify() before returning to the caller with a -EINPROGRESS
+ * ->fl_grant() before returning to the caller with a -EINPROGRESS
  * return code.
  */
 int vfs_lock_file(struct file *filp, unsigned int cmd, struct file_lock *fl, struct file_lock *conf)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c58690d..b22991d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -784,7 +784,8 @@ struct file_lock_operations {
 
 struct lock_manager_operations {
 	int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
-	int (*fl_notify)(struct file_lock *, struct file_lock *, int);
+	void (*fl_notify)(struct file_lock *);	/* unblock callback */
+	int (*fl_grant)(struct file_lock *, struct file_lock *, int);
 	void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
 	void (*fl_release_private)(struct file_lock *);
 	void (*fl_break)(struct file_lock *);