--- 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;
}
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
--- 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;
}
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
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
[email protected]
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.
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
[email protected]
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
> > 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
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
[email protected]
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