[PATCH] remove BKL from driverfs

[PATCH] remove BKL from driverfs

[Dave Hansen]

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

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?

-- 
Dave Hansen
haveblue@us.ibm.com

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

--- linux-2.5.24-clean/fs/driverfs/inode.c	Thu Jun 20 15:53:45 2002
+++ linux/fs/driverfs/inode.c	Wed Jul  3 23:18:23 2002
@@ -146,20 +146,16 @@
 static int driverfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 {
 	int res;
-	lock_kernel();
 	dentry->d_op = &driverfs_dentry_dir_ops;
  	res = driverfs_mknod(dir, dentry, mode | S_IFDIR, 0);
-	unlock_kernel();
 	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;
  	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;
 	}
@@ -353,8 +349,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 +368,7 @@
 	default:
 		break;
 	}
-	unlock_kernel();
+	up(&inode->i_sem);
 	return retval;
 }
 

Re: [PATCH] remove BKL from driverfs

[Greg KH]

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

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

This means what?

> 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?


> --- linux-2.5.24-clean/fs/driverfs/inode.c	Thu Jun 20 15:53:45 2002
> +++ linux/fs/driverfs/inode.c	Wed Jul  3 23:18:23 2002
> @@ -146,20 +146,16 @@
>  static int driverfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
>  {
>  	int res;
> -	lock_kernel();
>  	dentry->d_op = &driverfs_dentry_dir_ops;
>   	res = driverfs_mknod(dir, dentry, mode | S_IFDIR, 0);
> -	unlock_kernel();
>  	return res;
>  }

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


>  
>  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;
>  	}
> @@ -353,8 +349,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;

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

thanks,

greg k-h

Re: [PATCH] remove BKL from driverfs

[Dave Hansen]

[-- 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;
 }
 

Re: [PATCH] remove BKL from driverfs

[Greg KH]

On Thu, Jul 04, 2002 at 12:26:04AM -0700, Dave Hansen wrote:
> 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?

Yes he did.  Bitkeeper is your friend for finding out these kinds of
things :)

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

So go pick on init/main.c, not us poor little ram based filesystems...

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

Heh, since you've seen my talk twice, there is a real reason for the
tabs instead of spaces.  It's not just me being annoying.  Well ok, I'm
probably being annoying, but you should know better by now :)

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

thanks,

greg k-h

Re: [PATCH] remove BKL from driverfs

[Dave Hansen]

CC'ing Al for comments...

Greg KH wrote:
> 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.

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

OK, I agree a simple spinlock is not the way to go because I now see 
the sleepable operations in there.  But, I don't think 
driverfs_get_inode() needs any more locking.  The inode that it 
references is freshly allocated and my only concern would be about 
access from the inode_in_use list.  Maybe down()ing i_sem will provide 
a bit more protection, but unless the access through inode_in_use is 
already a problem, i_sem isn't needed.  Any thoughts, Al?

-- 
Dave Hansen
haveblue@us.ibm.com

Re: [PATCH] remove BKL from driverfs

[Alexander Viro]

On Thu, 4 Jul 2002, Dave Hansen wrote:

> CC'ing Al for comments...
> 
> Greg KH wrote:
> > 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.

Re: [PATCH] remove BKL from driverfs

[Dave Hansen]

Alexander Viro wrote:
> 
> On Thu, 4 Jul 2002, Dave Hansen wrote:
> 
>>CC'ing Al for comments...
>>
>>Greg KH wrote:
>>
>>>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.
> 

So, can we remove the spread in this case?

-- 
Dave Hansen
haveblue@us.ibm.com

Re: [PATCH] remove BKL from driverfs

[Patrick Mochel]

On Wed, 3 Jul 2002, 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).

I'm sorry to hear about your distress. Hopefully you've had a chance to 
talk to someone about it and calm down a bit. 

> 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?

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. 

The other replacement happens in _lseek. That looks fine, though I 
think that entire function can be replaced with one of the generic lseek 
functions...

Patch applied. Thanks,

	-pat

Re: [PATCH] remove BKL from driverfs

[Patrick Mochel]

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

	-pat

Re: [PATCH] remove BKL from driverfs

[Dave Hansen]

Patrick Mochel wrote:
>>>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.

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

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

I agree.  It looks like vfs_unlink() provides all of the protection 
that is needed.  No BKL, no more i_sem uses.  I'm asking Al Viro about 
driverfs_get_inode().  It looks like it will be safe and correct to 
use i_sem there, but stay tuned, it may not be necessary at all.

-- 
Dave Hansen
haveblue@us.ibm.com

Re: [PATCH] remove BKL from driverfs

[Greg KH]

On Sun, Jul 07, 2002 at 05:41:02PM -0700, Dave Hansen wrote:
> 
> I need to get some USB devices that work in Linux!

I can easily fix that :)

greg k-h