remove BKL from driverfs

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).

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

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?

fs/driverfs/inode.c +2 -4

 static int driverfs_create(struct inode *dir, struct dentry *dentry, int mode)
 {
 	int res;
-	lock_kernel();
 	dentry->d_op = &driverfs_dentry_file_ops;
  	res = driverfs_mknod(dir, dentry, mode | S_IFREG, 0);
-	unlock_kernel();
 	return res;
 }
 

@@ -211,9 +207,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;
 	}

Quoted 1 line · Dave Hansen

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 :)

Quoted 4 lines

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

This means what?

Quoted 1 line

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?

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

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

Quoted 6 lines · Greg KH

Quoted 3 lines · Dave Hansen

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?

Quoted 7 lines

Quoted 5 lines

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

Quoted 5 lines

Quoted 4 lines

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?

Quoted 3 lines

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 :)

Quoted 2 lines

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.

bleah, a proliferation of a zillion little spinlocks all across the kernel is not my idea of fun :(

I don't know if a simple spinlock can help us here. Look at driverfs_get_inode() and follow that into the vfs layer. Make sure all of that is race safe (and isn't currently relying on the BKL.) I'll defer to Al Viro's opinion about this, as I don't quite know all of the side effects going on at this moment in time.

CC'ing Al for comments...

Quoted 2 lines · Greg KH

bleah, a proliferation of a zillion little spinlocks all across the kernel is not my idea of fun :(

A zillion locks each with a single purpose is a lot more fun than 1 lock with a zillion different uses.

Quoted 8 lines · Dave Hansen

CC'ing Al for comments...

Quoted 2 lines · Greg KH

bleah, a proliferation of a zillion little spinlocks all across the kernel is not my idea of fun :(

A zillion locks each with a single purpose is a lot more fun than 1 lock with a zillion different uses.

Wrong. It's fun if you are into taking a turd and turning it into a thin film spread over all available surfaces. The former has a chance to be removed. The latter is hopeless.

"Zillion little spinlocks" means that kernel is scaled into oblivion. Literally. If you want to play with resulting body - feel free, but I like it less kinky.

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).

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

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?

Quoted 5 lines

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?

No, and I'm not about to try very hard. It appears that the place you removed it should be fine. In driverfs_unlink, you replace it with i_sem. ramfs, which driverfs mimmicks, doesn't hold any lock during unlink. It seems it could be removed altogether.

Quoted 3 lines

Quoted 1 line

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?

No, and I'm not about to try very hard. It appears that the place you removed it should be fine. In driverfs_unlink, you replace it with i_sem. ramfs, which driverfs mimmicks, doesn't hold any lock during unlink. It seems it could be removed altogether.

Actually, taking i_sem is completely wrong. Look at vfs_unlink() in fs/namei.c:

        down(&dentry->d_inode->i_sem);
        if (d_mountpoint(dentry))
                error = -EBUSY;
        else {
                error = dir->i_op->unlink(dir, dentry);
                if (!error)
                        d_delete(dentry);
        }
        up(&dentry->d_inode->i_sem);

Then, in driverfs_unlink:

	struct inode *inode = dentry->d_inode;

	down(&inode->i_sem);    

You didn't test this on file removal did you? A good way to verify that you have most of your bases covered is to plug/unplug a USB device a few times. I learned that one from Greg, and it's caught several bugs.

Anyway, I say that the lock can be removed altogether. Ditto for mknod as well.

I need to get some USB devices that work in Linux!