[PATCH] autofs4: locking rework

From: Ian Kent <raven@themaw.net>

Remove BKL from autofs4 module and add spinlock to serialise access to the
automount daemon communication waitq.

Locking requirements are different in 2.6 and so I'm seeking comments and
suggestions on this.  I have taken a rather heavy handed approach to this in
the patch.  For example, the VFS operations that directly change the
filesystem, such as autofs4_mkdir etc, hold the inode semaphore on entry so
the BKL has been removed.  I can't see why two locking mechanisms are needed.
 Rather than add locking all over the place, I'm looking for justification
it's needed, as I don't see it myself.

fs/autofs4/autofs_i.h

@@ -82,7 +82,7 @@ struct autofs_wait_queue {
 	char *name;
 	/* This is for status reporting upon return */
 	int status;
-	int wait_ctr;
+	atomic_t wait_ctr;
 };
 
 #define AUTOFS_SBI_MAGIC 0x6d4a556d

fs/autofs4/root.c

@@ -255,7 +255,6 @@ static struct dentry *autofs4_root_lookup(struct inode *dir, struct dentry *dent
 
 	sbi = autofs4_sbi(dir->i_sb);
 
-	lock_kernel();
 	oz_mode = autofs4_oz_mode(sbi);
 	DPRINTK(("autofs4_lookup: pid = %u, pgrp = %u, catatonic = %d, oz_mode = %d\n",
 		 current->pid, process_group(current), sbi->catatonic, oz_mode));
@@ -297,12 +296,10 @@ static struct dentry *autofs4_root_lookup(struct inode *dir, struct dentry *dent
 			if (sigismember (sigset, SIGKILL) ||
 			    sigismember (sigset, SIGQUIT) ||
 			    sigismember (sigset, SIGINT)) {
-			    unlock_kernel();
 			    return ERR_PTR(-ERESTARTNOINTR);
 			}
 		}
 	}
-	unlock_kernel();
 
 	/*
 	 * If this dentry is unhashed, then we shouldn't honour this
@@ -328,24 +325,18 @@ static int autofs4_dir_symlink(struct inode *dir,
 	DPRINTK(("autofs4_dir_symlink: %s <- %.*s\n", symname,
 		 dentry->d_name.len, dentry->d_name.name));
 
-	lock_kernel();
-	if (!autofs4_oz_mode(sbi)) {
-		unlock_kernel();
+	if (!autofs4_oz_mode(sbi))
 		return -EACCES;
-	}
 
 	ino = autofs4_init_ino(ino, sbi, S_IFLNK | 0555);
-	if (ino == NULL) {
-		unlock_kernel();
+	if (ino == NULL)
 		return -ENOSPC;
-	}
 
 	ino->size = strlen(symname);
 	ino->u.symlink = cp = kmalloc(ino->size + 1, GFP_KERNEL);
 
 	if (cp == NULL) {
 		kfree(ino);
-		unlock_kernel();
 		return -ENOSPC;
 	}
 
@@ -365,7 +356,6 @@ static int autofs4_dir_symlink(struct inode *dir,
 
 	dir->i_mtime = CURRENT_TIME;
 
-	unlock_kernel();
 	return 0;
 }
 
@@ -390,11 +380,8 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
 	
 	/* This allows root to remove symlinks */
-	lock_kernel();
-	if ( !autofs4_oz_mode(sbi) && !capable(CAP_SYS_ADMIN) ) {
-		unlock_kernel();
+	if ( !autofs4_oz_mode(sbi) && !capable(CAP_SYS_ADMIN) )
 		return -EACCES;
-	}
 
 	dput(ino->dentry);
 
@@ -405,8 +392,6 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
 
 	d_drop(dentry);
 
-	unlock_kernel();
-	
 	return 0;
 }
 
@@ -415,16 +400,12 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
 	struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
 	
-	lock_kernel();
-	if (!autofs4_oz_mode(sbi)) {
-		unlock_kernel();
+	if (!autofs4_oz_mode(sbi))
 		return -EACCES;
-	}
 
 	spin_lock(&dcache_lock);
 	if (!list_empty(&dentry->d_subdirs)) {
 		spin_unlock(&dcache_lock);
-		unlock_kernel();
 		return -ENOTEMPTY;
 	}
 	__d_drop(dentry);
@@ -438,7 +419,6 @@ static int autofs4_dir_rmdir(struct inode *dir, struct dentry *dentry)
 	if (dir->i_nlink)
 		dir->i_nlink--;
 
-	unlock_kernel();
 	return 0;
 }
 
@@ -450,20 +430,15 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	struct autofs_info *ino = autofs4_dentry_ino(dentry);
 	struct inode *inode;
 
-	lock_kernel();
-	if ( !autofs4_oz_mode(sbi) ) {
-		unlock_kernel();
+	if ( !autofs4_oz_mode(sbi) )
 		return -EACCES;
-	}
 
 	DPRINTK(("autofs4_dir_mkdir: dentry %p, creating %.*s\n",
 		 dentry, dentry->d_name.len, dentry->d_name.name));
 
 	ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
-	if (ino == NULL) {
-		unlock_kernel();
+	if (ino == NULL)
 		return -ENOSPC;
-	}
 
 	inode = autofs4_get_inode(dir->i_sb, ino);
 	d_instantiate(dentry, inode);
@@ -479,7 +454,6 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	dir->i_nlink++;
 	dir->i_mtime = CURRENT_TIME;
 
-	unlock_kernel();
 	return 0;
 }
 

fs/autofs4/waitq.c

@@ -16,6 +16,8 @@
 #include <linux/file.h>
 #include "autofs_i.h"
 
+static spinlock_t waitq_lock = SPIN_LOCK_UNLOCKED;
+
 /* We make this a static variable rather than a part of the superblock; it
    is better if we don't reassign numbers easily even across filesystems */
 static autofs_wqt_t autofs4_next_wait_queue = 1;
@@ -37,7 +39,7 @@ void autofs4_catatonic_mode(struct autofs_sb_info *sbi)
 		wq->status = -ENOENT; /* Magic is gone - report failure */
 		kfree(wq->name);
 		wq->name = NULL;
-		wake_up(&wq->queue);
+		wake_up_interruptible(&wq->queue);
 		wq = nwq;
 	}
 	if (sbi->pipe) {
@@ -138,12 +140,14 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct qstr *name,
 	if ( name->len > NAME_MAX )
 		return -ENOENT;
 
+	spin_lock(&waitq_lock);
 	for ( wq = sbi->queues ; wq ; wq = wq->next ) {
 		if ( wq->hash == name->hash &&
 		     wq->len == name->len &&
 		     wq->name && !memcmp(wq->name,name->name,name->len) )
 			break;
 	}
+	spin_unlock(&waitq_lock);
 	
 	if ( !wq ) {
 		/* Create a new wait queue */
@@ -156,21 +160,24 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct qstr *name,
 			kfree(wq);
 			return -ENOMEM;
 		}
+
+		spin_lock(&waitq_lock);
 		wq->wait_queue_token = autofs4_next_wait_queue;
 		if (++autofs4_next_wait_queue == 0)
 			autofs4_next_wait_queue = 1;
+		wq->next = sbi->queues;
+		sbi->queues = wq;
+		spin_unlock(&waitq_lock);
 		init_waitqueue_head(&wq->queue);
 		wq->hash = name->hash;
 		wq->len = name->len;
 		wq->status = -EINTR; /* Status return if interrupted */
 		memcpy(wq->name, name->name, name->len);
-		wq->next = sbi->queues;
-		sbi->queues = wq;
 
 		DPRINTK(("autofs4_wait: new wait id = 0x%08lx, name = %.*s, nfy=%d\n",
 			 (unsigned long) wq->wait_queue_token, wq->len, wq->name, notify));
 		/* autofs4_notify_daemon() may block */
-		wq->wait_ctr = 2;
+		atomic_set(&wq->wait_ctr, 2);
 		if (notify != NFY_NONE) {
 			autofs4_notify_daemon(sbi,wq, 
 					notify == NFY_MOUNT ?
@@ -178,7 +185,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct qstr *name,
 						  autofs_ptype_expire_multi);
 		}
 	} else {
-		wq->wait_ctr++;
+		atomic_inc(&wq->wait_ctr);
 		DPRINTK(("autofs4_wait: existing wait id = 0x%08lx, name = %.*s, nfy=%d\n",
 			 (unsigned long) wq->wait_queue_token, wq->len, wq->name, notify));
 	}
@@ -205,7 +212,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct qstr *name,
 		recalc_sigpending();
 		spin_unlock_irqrestore(&current->sighand->siglock, irqflags);
 
-		interruptible_sleep_on(&wq->queue);
+		wait_event_interruptible(wq->queue, wq->name == NULL);
 
 		spin_lock_irqsave(&current->sighand->siglock, irqflags);
 		current->blocked = oldset;
@@ -217,7 +224,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct qstr *name,
 
 	status = wq->status;
 
-	if (--wq->wait_ctr == 0)	/* Are we the last process to need status? */
+	if (atomic_dec_and_test(&wq->wait_ctr))	/* Are we the last process to need status? */
 		kfree(wq);
 
 	return status;
@@ -228,23 +235,28 @@ int autofs4_wait_release(struct autofs_sb_info *sbi, autofs_wqt_t wait_queue_tok
 {
 	struct autofs_wait_queue *wq, **wql;
 
+	spin_lock(&waitq_lock);
 	for ( wql = &sbi->queues ; (wq = *wql) ; wql = &wq->next ) {
 		if ( wq->wait_queue_token == wait_queue_token )
 			break;
 	}
-	if ( !wq )
+
+	if ( !wq ) {
+		spin_unlock(&waitq_lock);
 		return -EINVAL;
+	}
 
 	*wql = wq->next;	/* Unlink from chain */
+	spin_unlock(&waitq_lock);
 	kfree(wq->name);
 	wq->name = NULL;	/* Do not wait on this queue */
 
 	wq->status = status;
 
-	if (--wq->wait_ctr == 0)	/* Is anyone still waiting for this guy? */
+	if (atomic_dec_and_test(&wq->wait_ctr))	/* Is anyone still waiting for this guy? */
 		kfree(wq);
 	else
-		wake_up(&wq->queue);
+		wake_up_interruptible(&wq->queue);
 
 	return 0;
 }