Multithreaded locks.c

Gentlemen,

Here's a patch which fully multithreads the file locking code.

The functionality is unchanged, however I wonder if we actually have got it right. Does anyone know what this part of the flock(2) manpage means?

A single file may not simultaneously have both shared and exclusive locks.

I'm not able to generate any code which triggers this error. The implementation actually handles this case by detecting when a flock transits from LOCK_EX to LOCK_SH and waking up any LOCK_EX waiters. The BSD manpage isn't very helpful either. I think this should be an error. If it were, locks.c could become significantly simpler.

Commentary:

- This patch includes changes which were covered in Trond's recent patch, vis:

- Drop semaphore when we call fl_notify hooks. This is to allow the notification routines to call posix_unblock_lock().

(I didn't do this - it would have introduced a lot of races.
Instead, posix_unlock_block() requires that the per-inode lock be held, and doesn't do the lock itself)

- In locks_wake_up_blocks(), drop semaphore when we're asking the waiter waiter to unblock, and reorder loop to protect against the waiter disappearing beneath us while we're not holding the semaphore.

- locks_delete_lock() should not call file->f_op->lock(). The latter notifies the *server*, hence calling it from routines like posix_lock_file() while merging/unmerging locks in our internal representation of the locks is a bug.

- A new function locks_unlock_delete() takes care of the special case when we're freeing all locks upon close() and drops the semaphore before notifying the server.

- The global semaphore has gone. In its place are per-inode spinlocks and a global spinlock to protect the two global lists.

- Fixed the deadlock where nlmsvc_notify_blocked() calls posix_unblock_lock() with the global semaphore held.

- There was a bug in nlmsvc_notify_blocked() - it can return with the BKL held.

- The locking rules are:

fl_notify(), fl_insert() and fl_remove() are called with the per-inode spinlock held. They may not sleep (they don't at present). If it's a requirement that these be able to sleep then we have a medium-sized mess on our hands.

file_operations.lock() is always called with no locks held. It may sleep.

- I introduced a funky naming convention where functions which require that the per-inode spinlock be held end in `_ilr'. This helped a lot in getting the locking right.

@@ -405,6 +405,7 @@
 	struct super_block	*i_sb;
 	wait_queue_head_t	i_wait;
 	struct file_lock	*i_flock;
+	spinlock_t		i_flock_lock;
 	struct address_space	*i_mapping;
 	struct address_space	i_data;	
 	struct dquot		*i_dquot[MAXQUOTAS];

@@ -546,7 +547,7 @@
 #endif
 
 extern struct list_head file_lock_list;
-extern struct semaphore file_lock_sem;
+extern spinlock_t file_locks_lock;
 
 #include <linux/fcntl.h>
 

@@ -168,10 +168,10 @@
 	 * reclaim is in progress */
 	lock_kernel();
 	lockd_up();
-	down(&file_lock_sem);
 
 	/* First, reclaim all locks that have been granted previously. */
 restart:
+	spin_lock(&file_locks_lock);
 	tmp = file_lock_list.next;
 	while (tmp != &file_lock_list) {
 		struct file_lock *fl = list_entry(tmp, struct file_lock, fl_link);

@@ -181,12 +181,13 @@
 				fl->fl_u.nfs_fl.state != host->h_state &&
 				(fl->fl_u.nfs_fl.flags & NFS_LCK_GRANTED)) {
 			fl->fl_u.nfs_fl.flags &= ~ NFS_LCK_GRANTED;
+			spin_unlock(&file_locks_lock);
 			nlmclnt_reclaim(host, fl);
 			goto restart;
 		}
 		tmp = tmp->next;
 	}
-	up(&file_lock_sem);
+	spin_unlock(&file_locks_lock);
 
 	host->h_reclaiming = 0;
 	wake_up(&host->h_gracewait);

@@ -404,6 +404,11 @@
 	return 0;
 }
 
+/*
+ * VFS callback for file lock insertion.
+ *
+ * This function is not allowed to sleep.
+ */
 static
 void nlmclnt_insert_lock_callback(struct file_lock *fl)
 {

@@ -411,6 +416,12 @@
 	nlm_get_host(fl->fl_u.nfs_fl.host);
 	unlock_kernel();
 }
+
+/*
+ * VFS callback for file lock removal.
+ *
+ * This function is not allowed to sleep.
+ */
 static
 void nlmclnt_remove_lock_callback(struct file_lock *fl)
 {

@@ -99,6 +99,7 @@
 		sema_init(&inode->i_sem, 1);
 		sema_init(&inode->i_zombie, 1);
 		spin_lock_init(&inode->i_data.i_shared_lock);
+		spin_lock_init(&inode->i_flock_lock);
 	}
 }
 

@@ -215,7 +215,7 @@
 EXPORT_SYMBOL(page_hash_bits);
 EXPORT_SYMBOL(page_hash_table);
 EXPORT_SYMBOL(file_lock_list);
-EXPORT_SYMBOL(file_lock_sem);
+EXPORT_SYMBOL(file_locks_lock);
 EXPORT_SYMBOL(locks_init_lock);
 EXPORT_SYMBOL(locks_copy_lock);
 EXPORT_SYMBOL(posix_lock_file);

@@ -460,6 +460,8 @@
  *
  * 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.
+ *
+ * This function is not allowed to sleep.
  */
 static void
 nlmsvc_notify_blocked(struct file_lock *fl)

@@ -473,6 +475,7 @@
 		if (nlm_compare_locks(&block->b_call.a_args.lock.fl, fl)) {
 			nlmsvc_insert_block(block, 0);
 			svc_wake_up(block->b_daemon);
+			unlock_kernel();
 			return;
 		}
 	}

@@ -159,10 +159,10 @@
 	void (*fl_remove)(struct file_lock *);	/* lock removal callback */
 
 locking rules:
-		BKL	may block
-fl_notify:	yes	no
-fl_insert:	yes	maybe
-fl_remove:	yes	maybe
+		BKL	file_locks_lock   inode.i_flock_lock   may block
+fl_notify:	no	     no               yes              no
+fl_insert:	no	     no               yes              no
+fl_remove:	no	     no               yes              no
 	Currently only NLM provides instances of this class. None of the
 them block. If you have out-of-tree instances - please, show up. Locking
 in that area will change.

@@ -230,7 +230,7 @@
 release:	no
 fsync:		yes	(see below)
 fasync:		yes	(see below)
-lock:		yes
+lock:		no
 readv:		no
 writev:		no
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

AFAIK its saying LOCK_EX is exclusive and blocks shared locks and vice versa. Its a standard reader-writer lock setup. LOCK_EX is writer, LOCK_SH is reader.

Alan

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Thanks, Alan. You're right, and this is indeed how the current implementation behaves. The DG/UX manpage makes a bit more sense here.

One interesting factoid: if a process currently has an exclusive lock, and it changes that to an exclusive lock (ie: no change), this has the effect of dropping the lock and reclaiming it.

The DG/UX dg_flock() departs from BSD in that "Upgrading a lock will be an atomic operation. That is, a lock is not released to upgrade that lock". Linux doesn't do that - the lock is effectively dropped and reclaimed when we go from LOCK_SH to LOCK_EX.

Even the DG/UX manpage doesn't say what happens when you sidegrade the lock. LOCK_EX->LOCK_EX :)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Suggested code:

         printk("Don't do that\n"); return -EKNUCKLEHEAD;

---------------------------------------------------------
Victor Yodaiken 
Finite State Machine Labs: The RTLinux Company.
 www.fsmlabs.com  www.rtlinux.com

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/