Messages in this thread | | | Date | Thu, 4 Jul 2002 00:10:04 -0700 | From | Greg KH <> | Subject | Re: [PATCH] remove BKL from driverfs |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |