Re: [PATCH] remove BKL from driverfs

[Dave Hansen <haveblue@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

Greg KH wrote:
> On Wed, Jul 03, 2002 at 11:26:27PM -0700, Dave Hansen wrote:
>>I saw your talk about driverfs at OLS and it got my attention.  When 
>>my BKL debugging patch showed some use of the BKL in driverfs, I was 
>>very dissapointed (you can blame Greg if you want).
> 
> Blame me?  Al Viro pushed that BKL into the file, not I :)

But you're so much closer :)  Did he push the mknod stuff too?

>>text from dmesg after BKL debugging patch:
>>release of recursive BKL hold, depth: 1
>>[ 0]main:492
>>[ 1]inode:149
> 
> This means what?

BKL was acquired at main.c:492 and current->lock_depth was 0
then
BKL was acquired at inode.c:149 and current->lock_depth was 1

>>I see no reason to hold the BKL in your situation.  I replaced it with 
>>i_sem in some places and just plain removed it in others.  I believe 
>>that you get all of the protection that you need from dcache_lock in 
>>the dentry insert and activate.  Can you prove me wrong?
> 
> I see no reason to really care :)
> Can you prove that driverfs (or pcihpfs or usbfs) accesses are on a
> critical path that removing the BKL usage here actually helps?

Nope.  I'm pretty sure that it isn't in a critical path anywhere, nor 
are there any performance benefits.  It is simply an annoying use that 
is relatively easy to remove.  It's kinda like using spaces instead of 
tabs; most people won't notice, but some people really care :)

> I think that driverfs_mknod() needs some kind of protection now that you
> have removed it.

Do you just want to make sure it isn't called concurrently, or is 
there some other BKL-protected area that you're concerned about. 
driverfs_mknod() doesn't appear to be doing anything sneaky like 
sleeping or calling itself, so I think a simple spinlock will work 
just fine.

> Um, you used spaces, please use tabs like the rest of the file, and how
> Documentation/CodingStyle mandates.

Arg.  I saw your talk twice so I really don't have an excuse.  fix 
attached.

-- 
Dave Hansen
haveblue@us.ibm.com

[-- Attachment #2: driverfs-bkl_remove-2.5.24-1.patch --]
[-- Type: text/plain, Size: 1668 bytes --]

--- linux-2.5.24-clean/fs/driverfs/inode.c	Thu Jun 20 15:53:45 2002
+++ linux/fs/driverfs/inode.c	Thu Jul  4 00:22:54 2002
@@ -128,6 +128,7 @@
 	return inode;
 }
 
+static spinlock_t driverfs_mknod_lock = SPIN_LOCK_UNLOCKED;
 static int driverfs_mknod(struct inode *dir, struct dentry *dentry, int mode, int dev)
 {
 	struct inode *inode = driverfs_get_inode(dir->i_sb, mode, dev);
@@ -146,20 +147,20 @@
 static int driverfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 {
 	int res;
-	lock_kernel();
 	dentry->d_op = &driverfs_dentry_dir_ops;
+	spin_lock(&driverfs_mknod_lock);
  	res = driverfs_mknod(dir, dentry, mode | S_IFDIR, 0);
-	unlock_kernel();
+	spin_unlock(&driverfs_mknod_lock);
 	return res;
 }
 
 static int driverfs_create(struct inode *dir, struct dentry *dentry, int mode)
 {
 	int res;
-	lock_kernel();
 	dentry->d_op = &driverfs_dentry_file_ops;
+	spin_lock(&driverfs_mknod_lock);
  	res = driverfs_mknod(dir, dentry, mode | S_IFREG, 0);
-	unlock_kernel();
+	spin_unlock(&driverfs_mknod_lock);
 	return res;
 }
 
@@ -211,9 +212,9 @@
 	if (driverfs_empty(dentry)) {
 		struct inode *inode = dentry->d_inode;
 
-		lock_kernel();
+		down(&inode->i_sem);
 		inode->i_nlink--;
-		unlock_kernel();
+		up(&inode->i_sem);
 		dput(dentry);
 		error = 0;
 	}
@@ -353,8 +354,9 @@
 driverfs_file_lseek(struct file *file, loff_t offset, int orig)
 {
 	loff_t retval = -EINVAL;
+        struct inode *inode = file->f_dentry->d_inode->i_mapping->host;
 
-	lock_kernel();
+	down(&inode->i_sem);	
 	switch(orig) {
 	case 0:
 		if (offset > 0) {
@@ -371,7 +373,7 @@
 	default:
 		break;
 	}
-	unlock_kernel();
+	up(&inode->i_sem);
 	return retval;
 }