Remaining BKL users, what to do

Remaining BKL users, what to do

[Arnd Bergmann]

The big kernel lock is gone from almost all code in linux-next, this is
the status of what I think will happen to the remaining users:

drivers/gpu/drm/i810/{i810,i830}_dma.c:
	Fixable, but needs someone with the hardware to test. Can probably be
	marked CONFIG_BROKEN_ON_SMP if nobody cares.

drivers/media/video (V4L):
	Mauro is working on it, some drivers get moved to staging while the
	others get fixed. An easy workaround would be possible by adding
	per-driver mutexes, but Mauro wants to it properly by locking all
	the right places.

fs/adfs:
	Probably not hard to fix, but needs someone to test it.
	adfs has only seen janitorial fixes for the last 5 years.
	Do we know of any users?

fs/autofs:
	Pretty much dead, replaced by autofs4. I'd suggest moving this
	to drivers/staging in 2.6.37 and letting it die there.

fs/coda:
	Coda seems to have an active community, but not all of their
	code is actually part of linux (pioctl!), while the last official
	release is missing many of the cleanups that were don in Linux.
	Not sure what to do, if someone is interested, the best way might
	be a fresh start with a merger of the mainline linux and the
	coda.cs.cmu.edu	codebase in drivers/staging.
	Just removing the BKL without the Coda community seems like a heap
	of pointless work.

fs/freevxfs:
	Uses the BKL in readdir and lookup, should be easy to fix. Christoph?

fs/hpfs:
	Looks fixable, if anyone cares. Maybe it's time for retirement in
	drivers/staging though. The web page only has a Link to the
	linux-2.2 version.

fs/lockd:
	Trond writes that he has someone working on BKL removal here.

fs/locks.c:
	Patch is under discussion, blocked by work on fs/lockd currently.

fs/ncpfs:
	Should be fixable if Petr still cares about it. Otherwise suggest
	moving to drivers/staging if there are no users left.

fs/qnx4:
	Should be easy to fix, there are only a few places in the code that
	use the BKL. Anders?

fs/smbfs:
	Last I heard this was considered obsolete. Should be move it to
	drivers/staging now?

fs/udf:
	Not completely trivial, but probably necessary to fix. Project web
	site is dead, I hope that Jan Kara can be motivated to fix it though.

fs/ufs:
	Evgeniy Dushistov is maintaining this, I hope he can take care of
	getting rid of the BKL in UFS.

kernel/trace/blktrace.c:
	Should be easy. Ingo? Steven?

net/appletalk:
net/ipx/af_ipx.c:
net/irda/af_irda.c:
	Can probably be saved from retirement in drivers/staging if the
	maintainers still care.
	
net/x25:
	Andrew Hendry has started working on it.

This is all that's left now. I still need to submit a few patches for
simple file system changes, but it seems we're getting closer to finally
killing it for good.

	Arnd

Re: Remaining BKL users, what to do

[Steven Rostedt]

On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
> The big kernel lock is gone from almost all code in linux-next, this is
> the status of what I think will happen to the remaining users:

> kernel/trace/blktrace.c:
> 	Should be easy. Ingo? Steven?
> 

Jens,

Git blame shows this to be your code (copied from block/blktrace.c from
years past).

Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)

-- Steve

Re: Remaining BKL users, what to do

[Jens Axboe]

On 2010-09-16 16:49, Steven Rostedt wrote:
> On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
>> The big kernel lock is gone from almost all code in linux-next, this is
>> the status of what I think will happen to the remaining users:
> 
>> kernel/trace/blktrace.c:
>> 	Should be easy. Ingo? Steven?
>>
> 
> Jens,
> 
> Git blame shows this to be your code (copied from block/blktrace.c from
> years past).
> 
> Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)

It isn't, it can be removed.

-- 
Jens Axboe


Confidentiality Notice: This e-mail message, its contents and any attachments to it are confidential to the intended recipient, and may contain information that is privileged and/or exempt from disclosure under applicable law. If you are not the intended recipient, please immediately notify the sender and destroy the original e-mail message and any attachments (and any copies that may have been made) from your system or otherwise. Any unauthorized use, copying, disclosure or distribution of this information is strictly prohibited.

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Thursday 16 September 2010 20:32:36 Jens Axboe wrote:
> On 2010-09-16 16:49, Steven Rostedt wrote:
> > Git blame shows this to be your code (copied from block/blktrace.c from
> > years past).
> > 
> > Is the lock_kernel() needed here? (although Arnd did add it in 62c2a7d9)
> 
> It isn't, it can be removed.

Ok, I queued up this patch now. Thanks!

	Arnd
---
Subject: [PATCH] blktrace: remove the big kernel lock

According to Jens, this code does not need the BKL at all,
it is sufficiently serialized by bd_mutex.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 959f8d6..5328e87 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -23,7 +23,6 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
-#include <linux/smp_lock.h>
 #include <linux/time.h>
 #include <linux/uaccess.h>
 
@@ -639,7 +638,6 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	if (!q)
 		return -ENXIO;
 
-	lock_kernel();
 	mutex_lock(&bdev->bd_mutex);
 
 	switch (cmd) {
@@ -667,7 +665,6 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	}
 
 	mutex_unlock(&bdev->bd_mutex);
-	unlock_kernel();
 	return ret;
 }
 
@@ -1652,10 +1649,9 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 	struct block_device *bdev;
 	ssize_t ret = -ENXIO;
 
-	lock_kernel();
 	bdev = bdget(part_devt(p));
 	if (bdev == NULL)
-		goto out_unlock_kernel;
+		goto out;
 
 	q = blk_trace_get_queue(bdev);
 	if (q == NULL)
@@ -1683,8 +1679,7 @@ out_unlock_bdev:
 	mutex_unlock(&bdev->bd_mutex);
 out_bdput:
 	bdput(bdev);
-out_unlock_kernel:
-	unlock_kernel();
+out:
 	return ret;
 }
 
@@ -1714,11 +1709,10 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 
 	ret = -ENXIO;
 
-	lock_kernel();
 	p = dev_to_part(dev);
 	bdev = bdget(part_devt(p));
 	if (bdev == NULL)
-		goto out_unlock_kernel;
+		goto out;
 
 	q = blk_trace_get_queue(bdev);
 	if (q == NULL)
@@ -1753,8 +1747,6 @@ out_unlock_bdev:
 	mutex_unlock(&bdev->bd_mutex);
 out_bdput:
 	bdput(bdev);
-out_unlock_kernel:
-	unlock_kernel();
 out:
 	return ret ? ret : count;
 }

Re: Remaining BKL users, what to do

[Jan Kara]

On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
> The big kernel lock is gone from almost all code in linux-next, this is
> the status of what I think will happen to the remaining users:
...
> fs/ncpfs:
> 	Should be fixable if Petr still cares about it. Otherwise suggest
> 	moving to drivers/staging if there are no users left.
  I think some people still use this...

> fs/udf:
> 	Not completely trivial, but probably necessary to fix. Project web
> 	site is dead, I hope that Jan Kara can be motivated to fix it though.
  Yeah, I can have a look at it.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Remaining BKL users, what to do

[Anton Altaparmakov]

Hi,

On 16 Sep 2010, at 16:04, Jan Kara wrote:
> On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
>> The big kernel lock is gone from almost all code in linux-next, this is
>> the status of what I think will happen to the remaining users:
> ...
>> fs/ncpfs:
>> 	Should be fixable if Petr still cares about it. Otherwise suggest
>> 	moving to drivers/staging if there are no users left.
>  I think some people still use this...

Yes, indeed.  Netware is still alive (unfortunately!) and ncpfs is used in a lot of Universities here in the UK at least (we use it about a thousand workstations and servers here at Cambridge University!).

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Thursday 16 September 2010, Anton Altaparmakov wrote:
> On 16 Sep 2010, at 16:04, Jan Kara wrote:
> > On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
> >> The big kernel lock is gone from almost all code in linux-next, this is
> >> the status of what I think will happen to the remaining users:
> > ...
> >> fs/ncpfs:
> >>      Should be fixable if Petr still cares about it. Otherwise suggest
> >>      moving to drivers/staging if there are no users left.
> >  I think some people still use this...
> 
> Yes, indeed.  Netware is still alive (unfortunately!) and ncpfs is used in a lot of 
> Universities here in the UK at least (we use it about a thousand workstations and
> servers here at Cambridge University!).

Ok, that means at least when someone gets around to fix it, there will be
people that can test the patches.

If you know someone who would like to help on this, it would be nice to try
out the patch below, unless someone can come up with a better solution.
My naïve understanding of the code tells me that simply using the super block
lock there may work. In fact it makes locking stricter, so if it still works
with that patch, there are probably no subtle regressions.
The patch applies to current linux-next of my bkl/vfs series.

	Arnd

---
ncpfs: replace BKL with lock_super

This mindlessly changes every instance of lock_kernel in ncpfs to
lock_super. I haven't tested this, it may work or may break horribly.
Please test with CONFIG_LOCKDEP enabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 9578cbe..303338d 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -19,7 +19,6 @@
 #include <linux/mm.h>
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
-#include <linux/smp_lock.h>
 
 #include <linux/ncp_fs.h>
 
@@ -339,9 +338,10 @@ static int
 ncp_lookup_validate(struct dentry * dentry, struct nameidata *nd)
 {
 	int res;
-	lock_kernel();
+	struct super_block *sb = dentry->d_inode->i_sb;
+	lock_super(sb);
 	res = __ncp_lookup_validate(dentry);
-	unlock_kernel();
+	unlock_super(sb);
 	return res;
 }
 
@@ -404,6 +404,7 @@ static int ncp_readdir(struct file *filp, void *dirent, filldir_t filldir)
 {
 	struct dentry *dentry = filp->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
+	struct super_block *sb = inode->i_sb;
 	struct page *page = NULL;
 	struct ncp_server *server = NCP_SERVER(inode);
 	union  ncp_dir_cache *cache = NULL;
@@ -411,7 +412,7 @@ static int ncp_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	int result, mtime_valid = 0;
 	time_t mtime = 0;
 
-	lock_kernel();
+	lock_super(sb);
 
 	ctl.page  = NULL;
 	ctl.cache = NULL;
@@ -546,7 +547,7 @@ finished:
 		page_cache_release(ctl.page);
 	}
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return result;
 }
 
@@ -794,12 +795,13 @@ out:
 static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
 	struct ncp_server *server = NCP_SERVER(dir);
+	struct super_block *sb = dir->i_sb;
 	struct inode *inode = NULL;
 	struct ncp_entry_info finfo;
 	int error, res, len;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
-	lock_kernel();
+	lock_super(sb);
 	error = -EIO;
 	if (!ncp_conn_valid(server))
 		goto finished;
@@ -846,7 +848,7 @@ add_entry:
 
 finished:
 	PPRINTK("ncp_lookup: result=%d\n", error);
-	unlock_kernel();
+	unlock_super(sb);
 	return ERR_PTR(error);
 }
 
@@ -880,6 +882,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
 {
 	struct ncp_server *server = NCP_SERVER(dir);
 	struct ncp_entry_info finfo;
+	struct super_block *sb = dir->i_sb;
 	int error, result, len;
 	int opmode;
 	__u8 __name[NCP_MAXPATHLEN + 1];
@@ -888,7 +891,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
 		dentry->d_parent->d_name.name, dentry->d_name.name, mode);
 
 	error = -EIO;
-	lock_kernel();
+	lock_super(sb);
 	if (!ncp_conn_valid(server))
 		goto out;
 
@@ -935,7 +938,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
 
 	error = ncp_instantiate(dir, dentry, &finfo);
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
@@ -949,6 +952,7 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 {
 	struct ncp_entry_info finfo;
 	struct ncp_server *server = NCP_SERVER(dir);
+	struct super_block *sb = dir->i_sb;
 	int error, len;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
@@ -956,7 +960,7 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 
 	error = -EIO;
-	lock_kernel();
+	lock_super(sb);
 	if (!ncp_conn_valid(server))
 		goto out;
 
@@ -985,13 +989,14 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 		error = ncp_instantiate(dir, dentry, &finfo);
 	}
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
 static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	struct ncp_server *server = NCP_SERVER(dir);
+	struct super_block *sb = dir->i_sb;
 	int error, result, len;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
@@ -999,7 +1004,7 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 
 	error = -EIO;
-	lock_kernel();
+	lock_super(sb);
 	if (!ncp_conn_valid(server))
 		goto out;
 
@@ -1040,17 +1045,18 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
 			break;
        	}
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
 static int ncp_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = dentry->d_inode;
+	struct super_block *sb = dir->i_sb;
 	struct ncp_server *server;
 	int error;
 
-	lock_kernel();
+	lock_super(sb);
 	server = NCP_SERVER(dir);
 	DPRINTK("ncp_unlink: unlinking %s/%s\n",
 		dentry->d_parent->d_name.name, dentry->d_name.name);
@@ -1102,7 +1108,7 @@ static int ncp_unlink(struct inode *dir, struct dentry *dentry)
 	}
 		
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
@@ -1110,6 +1116,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
 		      struct inode *new_dir, struct dentry *new_dentry)
 {
 	struct ncp_server *server = NCP_SERVER(old_dir);
+	struct super_block *sb = old_dir->i_sb;
 	int error;
 	int old_len, new_len;
 	__u8 __old_name[NCP_MAXPATHLEN + 1], __new_name[NCP_MAXPATHLEN + 1];
@@ -1119,7 +1126,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
 		new_dentry->d_parent->d_name.name, new_dentry->d_name.name);
 
 	error = -EIO;
-	lock_kernel();
+	lock_super(sb);
 	if (!ncp_conn_valid(server))
 		goto out;
 
@@ -1165,7 +1172,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
 			break;
 	}
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
diff --git a/fs/ncpfs/file.c b/fs/ncpfs/file.c
index 3639cc5..a871df0 100644
--- a/fs/ncpfs/file.c
+++ b/fs/ncpfs/file.c
@@ -17,7 +17,6 @@
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 
 #include <linux/ncp_fs.h>
 #include "ncplib_kernel.h"
@@ -284,9 +283,11 @@ static int ncp_release(struct inode *inode, struct file *file) {
 static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
 {
 	loff_t ret;
-	lock_kernel();
+	struct super_block *sb = file->f_path.dentry->d_inode->i_sb;
+
+	lock_super(sb);
 	ret = generic_file_llseek_unlocked(file, offset, origin);
-	unlock_kernel();
+	unlock_super(sb);
 	return ret;
 }
 
diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
index cdf0fce..f37d297 100644
--- a/fs/ncpfs/inode.c
+++ b/fs/ncpfs/inode.c
@@ -26,7 +26,6 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/init.h>
-#include <linux/smp_lock.h>
 #include <linux/vfs.h>
 #include <linux/mount.h>
 #include <linux/seq_file.h>
@@ -445,12 +444,12 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 #endif
 	struct ncp_entry_info finfo;
 
-	lock_kernel();
+	lock_super(sb);
 
 	data.wdog_pid = NULL;
 	server = kzalloc(sizeof(struct ncp_server), GFP_KERNEL);
 	if (!server) {
-		unlock_kernel();
+		unlock_super(sb);
 		return -ENOMEM;
 	}
 	sb->s_fs_info = server;
@@ -704,7 +703,7 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
         if (!sb->s_root)
 		goto out_no_root;
 	sb->s_root->d_op = &ncp_root_dentry_operations;
-	unlock_kernel();
+	unlock_super(sb);
 	return 0;
 
 out_no_root:
@@ -741,7 +740,7 @@ out:
 	put_pid(data.wdog_pid);
 	sb->s_fs_info = NULL;
 	kfree(server);
-	unlock_kernel();
+	unlock_super(sb);
 	return error;
 }
 
@@ -749,7 +748,7 @@ static void ncp_put_super(struct super_block *sb)
 {
 	struct ncp_server *server = NCP_SBP(sb);
 
-	lock_kernel();
+	lock_super(sb);
 
 	ncp_lock_server(server);
 	ncp_disconnect(server);
@@ -778,7 +777,7 @@ static void ncp_put_super(struct super_block *sb)
 	sb->s_fs_info = NULL;
 	kfree(server);
 
-	unlock_kernel();
+	unlock_super(sb);
 }
 
 static int ncp_statfs(struct dentry *dentry, struct kstatfs *buf)
@@ -850,6 +849,7 @@ dflt:;
 int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
+	struct super_block *sb = inode->i_sb;
 	int result = 0;
 	__le32 info_mask;
 	struct nw_modify_dos_info info;
@@ -857,7 +857,7 @@ int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
 
 	result = -EIO;
 
-	lock_kernel();	
+	lock_super(sb);	
 
 	server = NCP_SERVER(inode);
 	if ((!server) || !ncp_conn_valid(server))
@@ -1011,7 +1011,7 @@ int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
 	mark_inode_dirty(inode);
 
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return result;
 }
 
diff --git a/fs/ncpfs/ioctl.c b/fs/ncpfs/ioctl.c
index 84a8cfc..4ce88d4 100644
--- a/fs/ncpfs/ioctl.c
+++ b/fs/ncpfs/ioctl.c
@@ -17,7 +17,6 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 #include <linux/highuid.h>
-#include <linux/smp_lock.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
 
@@ -844,8 +843,9 @@ static int ncp_ioctl_need_write(unsigned int cmd)
 long ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	long ret;
+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
 
-	lock_kernel();
+	lock_super(sb);
 	if (ncp_ioctl_need_write(cmd)) {
 		/*
 		 * inside the ioctl(), any failures which
@@ -863,19 +863,20 @@ long ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		mnt_drop_write(filp->f_path.mnt);
 
 out:
-	unlock_kernel();
+	unlock_super(sb);
 	return ret;
 }
 
 #ifdef CONFIG_COMPAT
 long ncp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
+	struct super_block *sb = file->f_path.dentry->d_inode->i_sb;
 	long ret;
 
-	lock_kernel();
+	lock_super(sb);
 	arg = (unsigned long) compat_ptr(arg);
 	ret = ncp_ioctl(file, cmd, arg);
-	unlock_kernel();
+	unlock_super(sb);
 	return ret;
 }
 #endif

Re: Remaining BKL users, what to do

[Christoph Hellwig]

On Fri, Sep 17, 2010 at 12:45:41PM +0200, Arnd Bergmann wrote:
> ncpfs: replace BKL with lock_super

Err, no.  lock_super is just as much on it's way out as the BKL.  We've
managed to move it down from the VFS into a few remaining filesystems
and now need to get rid of those users.  Please don't add any new ones.

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Friday 17 September 2010, Christoph Hellwig wrote:
> 
> On Fri, Sep 17, 2010 at 12:45:41PM +0200, Arnd Bergmann wrote:
> > ncpfs: replace BKL with lock_super
> 
> Err, no.  lock_super is just as much on it's way out as the BKL.  We've
> managed to move it down from the VFS into a few remaining filesystems
> and now need to get rid of those users.  Please don't add any new ones.

Ok. I guess that's also a NAK for my the isofs patch I posted yesterday
then. Do you have any suggestions for an alternative approach?

	Arnd

Re: Remaining BKL users, what to do

[Christoph Hellwig]

On Fri, Sep 17, 2010 at 03:50:49PM +0200, Arnd Bergmann wrote:
> On Friday 17 September 2010, Christoph Hellwig wrote:
> > 
> > On Fri, Sep 17, 2010 at 12:45:41PM +0200, Arnd Bergmann wrote:
> > > ncpfs: replace BKL with lock_super
> > 
> > Err, no.  lock_super is just as much on it's way out as the BKL.  We've
> > managed to move it down from the VFS into a few remaining filesystems
> > and now need to get rid of those users.  Please don't add any new ones.
> 
> Ok. I guess that's also a NAK for my the isofs patch I posted yesterday
> then. Do you have any suggestions for an alternative approach?

Just add a per-sb mutex inside the filesystem.  Given that lock_super
isn't used by the VFS anymore that's actually equivalent.

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Friday 17 September 2010, Christoph Hellwig wrote:
> 
> Just add a per-sb mutex inside the filesystem.  Given that lock_super
> isn't used by the VFS anymore that's actually equivalent.

Ok, thanks for the hint. I'll fix that for isofs.

	Arnd

[PATCH] BKL: Remove BKL from isofs

[Arnd Bergmann]

As in other file systems, we can replace the big kernel lock
with a private mutex in isofs. This means we can now access
multiple file systems concurrently, but it also means that
we serialize readdir and lookup across sleeping operations
which previously released the big kernel lock. This should
not matter though, as these operations are in practice
serialized through the hardware access.

The isofs_get_blocks functions now does not take any lock
any more, it used to recursively get the BKL. After looking
at the code for hours, I convinced myself that it was never
needed here anyway, because it only reads constant fields
of the inode and writes to a buffer head array that is
at this time only visible to the caller.

The get_sb and fill_super operations do not need the locking
at all because they operate on a file system that is either
about to be created or to be destroyed but in either case
is not visible to other threads.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

 fs/isofs/dir.c   |    6 +++---
 fs/isofs/inode.c |   17 ++---------------
 fs/isofs/isofs.h |    1 +
 fs/isofs/namei.c |    8 ++++----
 fs/isofs/rock.c  |   10 +++++-----
 5 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index e0aca9a..0542b6e 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -10,7 +10,6 @@
  *
  *  isofs directory handling functions
  */
-#include <linux/smp_lock.h>
 #include <linux/gfp.h>
 #include "isofs.h"
 
@@ -255,18 +254,19 @@ static int isofs_readdir(struct file *filp,
 	char *tmpname;
 	struct iso_directory_record *tmpde;
 	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
 
 	tmpname = (char *)__get_free_page(GFP_KERNEL);
 	if (tmpname == NULL)
 		return -ENOMEM;
 
-	lock_kernel();
+	mutex_lock(&sbi->s_mutex);
 	tmpde = (struct iso_directory_record *) (tmpname+1024);
 
 	result = do_isofs_readdir(inode, filp, dirent, filldir, tmpname, tmpde);
 
 	free_page((unsigned long) tmpname);
-	unlock_kernel();
+	mutex_unlock(&sbi->s_mutex);
 	return result;
 }
 
diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 05baf77..09ff41a 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -17,7 +17,6 @@
 #include <linux/slab.h>
 #include <linux/nls.h>
 #include <linux/ctype.h>
-#include <linux/smp_lock.h>
 #include <linux/statfs.h>
 #include <linux/cdrom.h>
 #include <linux/parser.h>
@@ -44,11 +43,7 @@ static void isofs_put_super(struct super_block *sb)
 	struct isofs_sb_info *sbi = ISOFS_SB(sb);
 
 #ifdef CONFIG_JOLIET
-	lock_kernel();
-
 	unload_nls(sbi->s_nls_iocharset);
-
-	unlock_kernel();
 #endif
 
 	kfree(sbi);
@@ -571,15 +566,11 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
 	int table, error = -EINVAL;
 	unsigned int vol_desc_start;
 
-	lock_kernel();
-
 	save_mount_options(s, data);
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
-	if (!sbi) {
-		unlock_kernel();
+	if (!sbi)
 		return -ENOMEM;
-	}
 	s->s_fs_info = sbi;
 
 	if (!parse_options((char *)data, &opt))
@@ -827,6 +818,7 @@ root_found:
 	sbi->s_utf8 = opt.utf8;
 	sbi->s_nocompress = opt.nocompress;
 	sbi->s_overriderockperm = opt.overriderockperm;
+	mutex_init(&sbi->s_mutex);
 	/*
 	 * It would be incredibly stupid to allow people to mark every file
 	 * on the disk as suid, so we merely allow them to set the default
@@ -904,7 +896,6 @@ root_found:
 
 	kfree(opt.iocharset);
 
-	unlock_kernel();
 	return 0;
 
 	/*
@@ -944,7 +935,6 @@ out_freesbi:
 	kfree(opt.iocharset);
 	kfree(sbi);
 	s->s_fs_info = NULL;
-	unlock_kernel();
 	return error;
 }
 
@@ -983,8 +973,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
 	int section, rv, error;
 	struct iso_inode_info *ei = ISOFS_I(inode);
 
-	lock_kernel();
-
 	error = -EIO;
 	rv = 0;
 	if (iblock < 0 || iblock != iblock_s) {
@@ -1060,7 +1048,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
 
 	error = 0;
 abort:
-	unlock_kernel();
 	return rv != 0 ? rv : error;
 }
 
diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
index 7d33de8..2882dc0 100644
--- a/fs/isofs/isofs.h
+++ b/fs/isofs/isofs.h
@@ -55,6 +55,7 @@ struct isofs_sb_info {
 	gid_t s_gid;
 	uid_t s_uid;
 	struct nls_table *s_nls_iocharset; /* Native language support table */
+	struct mutex s_mutex; /* replaces BKL, please remove if possible */
 };
 
 #define ISOFS_INVALID_MODE ((mode_t) -1)
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index ab438be..0d23abf 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -6,7 +6,6 @@
  *  (C) 1991  Linus Torvalds - minix filesystem
  */
 
-#include <linux/smp_lock.h>
 #include <linux/gfp.h>
 #include "isofs.h"
 
@@ -168,6 +167,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	int found;
 	unsigned long uninitialized_var(block);
 	unsigned long uninitialized_var(offset);
+	struct isofs_sb_info *sbi = ISOFS_SB(dir->i_sb);
 	struct inode *inode;
 	struct page *page;
 
@@ -177,7 +177,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	if (!page)
 		return ERR_PTR(-ENOMEM);
 
-	lock_kernel();
+	mutex_lock(&sbi->s_mutex);
 	found = isofs_find_entry(dir, dentry,
 				&block, &offset,
 				page_address(page),
@@ -188,10 +188,10 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
 		if (IS_ERR(inode)) {
-			unlock_kernel();
+			mutex_unlock(&sbi->s_mutex);
 			return ERR_CAST(inode);
 		}
 	}
-	unlock_kernel();
+	mutex_unlock(&sbi->s_mutex);
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index 96a685c..f9cd04d 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -8,7 +8,6 @@
 
 #include <linux/slab.h>
 #include <linux/pagemap.h>
-#include <linux/smp_lock.h>
 
 #include "isofs.h"
 #include "rock.h"
@@ -661,6 +660,7 @@ static int rock_ridge_symlink_readpage(struct file *file, struct page *page)
 {
 	struct inode *inode = page->mapping->host;
 	struct iso_inode_info *ei = ISOFS_I(inode);
+	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
 	char *link = kmap(page);
 	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
 	struct buffer_head *bh;
@@ -673,12 +673,12 @@ static int rock_ridge_symlink_readpage(struct file *file, struct page *page)
 	struct rock_state rs;
 	int ret;
 
-	if (!ISOFS_SB(inode->i_sb)->s_rock)
+	if (!sbi->s_rock)
 		goto error;
 
 	init_rock_state(&rs, inode);
 	block = ei->i_iget5_block;
-	lock_kernel();
+	mutex_lock(&sbi->s_mutex);
 	bh = sb_bread(inode->i_sb, block);
 	if (!bh)
 		goto out_noread;
@@ -748,7 +748,7 @@ repeat:
 		goto fail;
 	brelse(bh);
 	*rpnt = '\0';
-	unlock_kernel();
+	mutex_unlock(&sbi->s_mutex);
 	SetPageUptodate(page);
 	kunmap(page);
 	unlock_page(page);
@@ -765,7 +765,7 @@ out_bad_span:
 	printk("symlink spans iso9660 blocks\n");
 fail:
 	brelse(bh);
-	unlock_kernel();
+	mutex_unlock(&sbi->s_mutex);
 error:
 	SetPageError(page);
 	kunmap(page);

Re: [PATCH] BKL: Remove BKL from isofs

[Jan Kara]

On Fri 17-09-10 21:00:06, Arnd Bergmann wrote:
> As in other file systems, we can replace the big kernel lock
> with a private mutex in isofs. This means we can now access
> multiple file systems concurrently, but it also means that
> we serialize readdir and lookup across sleeping operations
> which previously released the big kernel lock. This should
> not matter though, as these operations are in practice
> serialized through the hardware access.
> 
> The isofs_get_blocks functions now does not take any lock
> any more, it used to recursively get the BKL. After looking
> at the code for hours, I convinced myself that it was never
> needed here anyway, because it only reads constant fields
> of the inode and writes to a buffer head array that is
> at this time only visible to the caller.
> 
> The get_sb and fill_super operations do not need the locking
> at all because they operate on a file system that is either
> about to be created or to be destroyed but in either case
> is not visible to other threads.
  Hmm, looking through the code, I actually don't see a reason
why we should need any per-sb lock at all. The filesystem is always
read-only and we don't seem to have any global data structures that
change. But that needs some testing I guess - I'll try to do that.

								Honza
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 
>  fs/isofs/dir.c   |    6 +++---
>  fs/isofs/inode.c |   17 ++---------------
>  fs/isofs/isofs.h |    1 +
>  fs/isofs/namei.c |    8 ++++----
>  fs/isofs/rock.c  |   10 +++++-----
>  5 files changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
> index e0aca9a..0542b6e 100644
> --- a/fs/isofs/dir.c
> +++ b/fs/isofs/dir.c
> @@ -10,7 +10,6 @@
>   *
>   *  isofs directory handling functions
>   */
> -#include <linux/smp_lock.h>
>  #include <linux/gfp.h>
>  #include "isofs.h"
>  
> @@ -255,18 +254,19 @@ static int isofs_readdir(struct file *filp,
>  	char *tmpname;
>  	struct iso_directory_record *tmpde;
>  	struct inode *inode = filp->f_path.dentry->d_inode;
> +	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
>  
>  	tmpname = (char *)__get_free_page(GFP_KERNEL);
>  	if (tmpname == NULL)
>  		return -ENOMEM;
>  
> -	lock_kernel();
> +	mutex_lock(&sbi->s_mutex);
>  	tmpde = (struct iso_directory_record *) (tmpname+1024);
>  
>  	result = do_isofs_readdir(inode, filp, dirent, filldir, tmpname, tmpde);
>  
>  	free_page((unsigned long) tmpname);
> -	unlock_kernel();
> +	mutex_unlock(&sbi->s_mutex);
>  	return result;
>  }
>  
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 05baf77..09ff41a 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -17,7 +17,6 @@
>  #include <linux/slab.h>
>  #include <linux/nls.h>
>  #include <linux/ctype.h>
> -#include <linux/smp_lock.h>
>  #include <linux/statfs.h>
>  #include <linux/cdrom.h>
>  #include <linux/parser.h>
> @@ -44,11 +43,7 @@ static void isofs_put_super(struct super_block *sb)
>  	struct isofs_sb_info *sbi = ISOFS_SB(sb);
>  
>  #ifdef CONFIG_JOLIET
> -	lock_kernel();
> -
>  	unload_nls(sbi->s_nls_iocharset);
> -
> -	unlock_kernel();
>  #endif
>  
>  	kfree(sbi);
> @@ -571,15 +566,11 @@ static int isofs_fill_super(struct super_block *s, void *data, int silent)
>  	int table, error = -EINVAL;
>  	unsigned int vol_desc_start;
>  
> -	lock_kernel();
> -
>  	save_mount_options(s, data);
>  
>  	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> -	if (!sbi) {
> -		unlock_kernel();
> +	if (!sbi)
>  		return -ENOMEM;
> -	}
>  	s->s_fs_info = sbi;
>  
>  	if (!parse_options((char *)data, &opt))
> @@ -827,6 +818,7 @@ root_found:
>  	sbi->s_utf8 = opt.utf8;
>  	sbi->s_nocompress = opt.nocompress;
>  	sbi->s_overriderockperm = opt.overriderockperm;
> +	mutex_init(&sbi->s_mutex);
>  	/*
>  	 * It would be incredibly stupid to allow people to mark every file
>  	 * on the disk as suid, so we merely allow them to set the default
> @@ -904,7 +896,6 @@ root_found:
>  
>  	kfree(opt.iocharset);
>  
> -	unlock_kernel();
>  	return 0;
>  
>  	/*
> @@ -944,7 +935,6 @@ out_freesbi:
>  	kfree(opt.iocharset);
>  	kfree(sbi);
>  	s->s_fs_info = NULL;
> -	unlock_kernel();
>  	return error;
>  }
>  
> @@ -983,8 +973,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
>  	int section, rv, error;
>  	struct iso_inode_info *ei = ISOFS_I(inode);
>  
> -	lock_kernel();
> -
>  	error = -EIO;
>  	rv = 0;
>  	if (iblock < 0 || iblock != iblock_s) {
> @@ -1060,7 +1048,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
>  
>  	error = 0;
>  abort:
> -	unlock_kernel();
>  	return rv != 0 ? rv : error;
>  }
>  
> diff --git a/fs/isofs/isofs.h b/fs/isofs/isofs.h
> index 7d33de8..2882dc0 100644
> --- a/fs/isofs/isofs.h
> +++ b/fs/isofs/isofs.h
> @@ -55,6 +55,7 @@ struct isofs_sb_info {
>  	gid_t s_gid;
>  	uid_t s_uid;
>  	struct nls_table *s_nls_iocharset; /* Native language support table */
> +	struct mutex s_mutex; /* replaces BKL, please remove if possible */
>  };
>  
>  #define ISOFS_INVALID_MODE ((mode_t) -1)
> diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
> index ab438be..0d23abf 100644
> --- a/fs/isofs/namei.c
> +++ b/fs/isofs/namei.c
> @@ -6,7 +6,6 @@
>   *  (C) 1991  Linus Torvalds - minix filesystem
>   */
>  
> -#include <linux/smp_lock.h>
>  #include <linux/gfp.h>
>  #include "isofs.h"
>  
> @@ -168,6 +167,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
>  	int found;
>  	unsigned long uninitialized_var(block);
>  	unsigned long uninitialized_var(offset);
> +	struct isofs_sb_info *sbi = ISOFS_SB(dir->i_sb);
>  	struct inode *inode;
>  	struct page *page;
>  
> @@ -177,7 +177,7 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
>  	if (!page)
>  		return ERR_PTR(-ENOMEM);
>  
> -	lock_kernel();
> +	mutex_lock(&sbi->s_mutex);
>  	found = isofs_find_entry(dir, dentry,
>  				&block, &offset,
>  				page_address(page),
> @@ -188,10 +188,10 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
>  	if (found) {
>  		inode = isofs_iget(dir->i_sb, block, offset);
>  		if (IS_ERR(inode)) {
> -			unlock_kernel();
> +			mutex_unlock(&sbi->s_mutex);
>  			return ERR_CAST(inode);
>  		}
>  	}
> -	unlock_kernel();
> +	mutex_unlock(&sbi->s_mutex);
>  	return d_splice_alias(inode, dentry);
>  }
> diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
> index 96a685c..f9cd04d 100644
> --- a/fs/isofs/rock.c
> +++ b/fs/isofs/rock.c
> @@ -8,7 +8,6 @@
>  
>  #include <linux/slab.h>
>  #include <linux/pagemap.h>
> -#include <linux/smp_lock.h>
>  
>  #include "isofs.h"
>  #include "rock.h"
> @@ -661,6 +660,7 @@ static int rock_ridge_symlink_readpage(struct file *file, struct page *page)
>  {
>  	struct inode *inode = page->mapping->host;
>  	struct iso_inode_info *ei = ISOFS_I(inode);
> +	struct isofs_sb_info *sbi = ISOFS_SB(inode->i_sb);
>  	char *link = kmap(page);
>  	unsigned long bufsize = ISOFS_BUFFER_SIZE(inode);
>  	struct buffer_head *bh;
> @@ -673,12 +673,12 @@ static int rock_ridge_symlink_readpage(struct file *file, struct page *page)
>  	struct rock_state rs;
>  	int ret;
>  
> -	if (!ISOFS_SB(inode->i_sb)->s_rock)
> +	if (!sbi->s_rock)
>  		goto error;
>  
>  	init_rock_state(&rs, inode);
>  	block = ei->i_iget5_block;
> -	lock_kernel();
> +	mutex_lock(&sbi->s_mutex);
>  	bh = sb_bread(inode->i_sb, block);
>  	if (!bh)
>  		goto out_noread;
> @@ -748,7 +748,7 @@ repeat:
>  		goto fail;
>  	brelse(bh);
>  	*rpnt = '\0';
> -	unlock_kernel();
> +	mutex_unlock(&sbi->s_mutex);
>  	SetPageUptodate(page);
>  	kunmap(page);
>  	unlock_page(page);
> @@ -765,7 +765,7 @@ out_bad_span:
>  	printk("symlink spans iso9660 blocks\n");
>  fail:
>  	brelse(bh);
> -	unlock_kernel();
> +	mutex_unlock(&sbi->s_mutex);
>  error:
>  	SetPageError(page);
>  	kunmap(page);
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] BKL: Remove BKL from isofs

[Arnd Bergmann]

On Monday 20 September 2010, Jan Kara wrote:
>   Hmm, looking through the code, I actually don't see a reason
> why we should need any per-sb lock at all. The filesystem is always
> read-only and we don't seem to have any global data structures that
> change. But that needs some testing I guess - I'll try to do that.

Ok, great! The BKL was basically as wrong as the global mutex protecting
the operations anyway, because it does not document what data is
actually getting protected in any of all the drivers that I'm converting
to a private mutex.

Given more time for code inspection and some testing, you can probably
come up with a good explanation why the BKL is not needed in all those
places, but since nobody ever bothered to do this for the last decade
for all these drivers, my approach was to simply prove (in a rather lose
sense) that I can't make it worse by converting to a mutex.

	Arnd

Re: [PATCH] BKL: Remove BKL from isofs

[Jan Kara]

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

On Mon 20-09-10 13:13:11, Arnd Bergmann wrote:
> On Monday 20 September 2010, Jan Kara wrote:
> >   Hmm, looking through the code, I actually don't see a reason
> > why we should need any per-sb lock at all. The filesystem is always
> > read-only and we don't seem to have any global data structures that
> > change. But that needs some testing I guess - I'll try to do that.
> 
> Ok, great! The BKL was basically as wrong as the global mutex protecting
> the operations anyway, because it does not document what data is
> actually getting protected in any of all the drivers that I'm converting
> to a private mutex.
> 
> Given more time for code inspection and some testing, you can probably
> come up with a good explanation why the BKL is not needed in all those
> places, but since nobody ever bothered to do this for the last decade
> for all these drivers, my approach was to simply prove (in a rather lose
> sense) that I can't make it worse by converting to a mutex.
  Attached is an obvious patch with some reasoning. I've also run for about
half an hour 10 parallel tar processes continuously reading the isofs image
in a loop. In parallel was running an infinite loop with "echo 2
>/proc/sys/vm/drop_caches" so directory entries were constantly reread from
the filesystem. The whole thing was running inside KVM so the fs image was
actually cached in memory. The machine has 8 CPUs so I think reasonable
paralelism has happened and all seemed happy. So I think the patch should
be safe to put into some tree for testing in inclusion...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-isofs-Remove-BKL.patch --]
[-- Type: text/x-patch, Size: 4026 bytes --]

>From 95263e60835707d394f4a4daa6b916737b569f01 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 20 Sep 2010 15:11:54 +0200
Subject: [PATCH] isofs: Remove BKL

BKL isn't needed for isofs at all so we can just remove it. Generally, since
isofs is always mounted read-only, filesystem structure cannot change under us.
So buffer_head contents stays constant after it's filled in. That leaves us
with possible changes of global data structures. Superblock changes only during
filesystem mount (even remount does not change it), inodes are only filled in
during reading from disk. So there are no changes of these structures to
bother about.

Arguments why BKL can be removed at each place:
isofs_readdir: Accesses sb, inode, filp, local variables => no BKL needed
isofs_put_super: VFS guards us against races with mount (for sb access).
  Otherwise BKL not needed.
isofs_get_blocks: Accesses sb, inode, local variables (filled in buffer heads
  are local) => no BLK needed
isofs_lookup: Protected by directory's i_mutex. Accesses sb, inode, dentry,
  local variables => no BKL needed
rock_ridge_symlink_readpage: Protected by page lock. Accesses sb, inode,
  local variables => no BKL needed.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/isofs/dir.c   |    2 --
 fs/isofs/inode.c |    7 -------
 fs/isofs/namei.c |    3 ---
 fs/isofs/rock.c  |    3 ---
 4 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/fs/isofs/dir.c b/fs/isofs/dir.c
index e0aca9a..b29d48e 100644
--- a/fs/isofs/dir.c
+++ b/fs/isofs/dir.c
@@ -260,13 +260,11 @@ static int isofs_readdir(struct file *filp,
 	if (tmpname == NULL)
 		return -ENOMEM;
 
-	lock_kernel();
 	tmpde = (struct iso_directory_record *) (tmpname+1024);
 
 	result = do_isofs_readdir(inode, filp, dirent, filldir, tmpname, tmpde);
 
 	free_page((unsigned long) tmpname);
-	unlock_kernel();
 	return result;
 }
 
diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 5a44811..d8a68e7 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -44,11 +44,7 @@ static void isofs_put_super(struct super_block *sb)
 	struct isofs_sb_info *sbi = ISOFS_SB(sb);
 
 #ifdef CONFIG_JOLIET
-	lock_kernel();
-
 	unload_nls(sbi->s_nls_iocharset);
-
-	unlock_kernel();
 #endif
 
 	kfree(sbi);
@@ -977,8 +973,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
 	int section, rv, error;
 	struct iso_inode_info *ei = ISOFS_I(inode);
 
-	lock_kernel();
-
 	error = -EIO;
 	rv = 0;
 	if (iblock < 0 || iblock != iblock_s) {
@@ -1054,7 +1048,6 @@ int isofs_get_blocks(struct inode *inode, sector_t iblock_s,
 
 	error = 0;
 abort:
-	unlock_kernel();
 	return rv != 0 ? rv : error;
 }
 
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index ab438be..c9c7498 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -177,7 +177,6 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	if (!page)
 		return ERR_PTR(-ENOMEM);
 
-	lock_kernel();
 	found = isofs_find_entry(dir, dentry,
 				&block, &offset,
 				page_address(page),
@@ -188,10 +187,8 @@ struct dentry *isofs_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	if (found) {
 		inode = isofs_iget(dir->i_sb, block, offset);
 		if (IS_ERR(inode)) {
-			unlock_kernel();
 			return ERR_CAST(inode);
 		}
 	}
-	unlock_kernel();
 	return d_splice_alias(inode, dentry);
 }
diff --git a/fs/isofs/rock.c b/fs/isofs/rock.c
index 96a685c..a679b55 100644
--- a/fs/isofs/rock.c
+++ b/fs/isofs/rock.c
@@ -678,7 +678,6 @@ static int rock_ridge_symlink_readpage(struct file *file, struct page *page)
 
 	init_rock_state(&rs, inode);
 	block = ei->i_iget5_block;
-	lock_kernel();
 	bh = sb_bread(inode->i_sb, block);
 	if (!bh)
 		goto out_noread;
@@ -748,7 +747,6 @@ repeat:
 		goto fail;
 	brelse(bh);
 	*rpnt = '\0';
-	unlock_kernel();
 	SetPageUptodate(page);
 	kunmap(page);
 	unlock_page(page);
@@ -765,7 +763,6 @@ out_bad_span:
 	printk("symlink spans iso9660 blocks\n");
 fail:
 	brelse(bh);
-	unlock_kernel();
 error:
 	SetPageError(page);
 	kunmap(page);
-- 
1.6.4.2

Re: [PATCH] BKL: Remove BKL from isofs

[Alexander E. Patrakov]

[I know nothing about linux filesystem code, please disregard this mail 
if it is stupid]

20.09.2010 21:18, Jan Kara wrote:

> BKL isn't needed for isofs at all so we can just remove it. Generally, since
> isofs is always mounted read-only, filesystem structure cannot change under us.

Does your statement also cover the case of read errors on scratched 
CD-ROMs (i.e., the driver has successfully read a sector with a 
directory once after a lot of strugging, and failed the next time), or 
ejects of mounted media?

-- 
Alexander E. Patrakov

Re: [PATCH] BKL: Remove BKL from isofs

[Jan Kara]

On Mon 20-09-10 21:40:55, Alexander E. Patrakov wrote:
> [I know nothing about linux filesystem code, please disregard this
> mail if it is stupid]
> 
> 20.09.2010 21:18, Jan Kara wrote:
> 
> >BKL isn't needed for isofs at all so we can just remove it. Generally, since
> >isofs is always mounted read-only, filesystem structure cannot change under us.
> 
> Does your statement also cover the case of read errors on scratched
> CD-ROMs (i.e., the driver has successfully read a sector with a
> directory once after a lot of strugging, and failed the next time),
> or ejects of mounted media?
  Well, BKL definitely won't help in protecting races triggered by such
problems. Also if we fail to read some data, we just bail out with
error so data appearing later do not bother us. Similarly once we get hold
of some buffer with data, it cannot disappear under us while we hold a
reference to it so again we are safe... I don't think these case pose any
problem.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Remaining BKL users, what to do

[Alan Cox]

> net/appletalk:
> net/ipx/af_ipx.c:
> net/irda/af_irda.c:
> 	Can probably be saved from retirement in drivers/staging if the
> 	maintainers still care.

IPX and Appletalk both have active users. They also look fairly fixable
as the lock_kernel just maps to a stack private mutex, or in several
cases can simply be dropped - its just a push down legacy.

IRDA may well be a candidate for staging

Re: Remaining BKL users, what to do

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Thu, 16 Sep 2010 16:07:59 +0100

>> net/appletalk:
>> net/ipx/af_ipx.c:
>> net/irda/af_irda.c:
>> 	Can probably be saved from retirement in drivers/staging if the
>> 	maintainers still care.
> 
> IPX and Appletalk both have active users. They also look fairly fixable
> as the lock_kernel just maps to a stack private mutex, or in several
> cases can simply be dropped - its just a push down legacy.

I'll take a stab at IPX and Appletalk.

Re: Remaining BKL users, what to do

[Anders Larsen]

On 2010-09-16 16:32:59, Arnd Bergmann wrote:
> The big kernel lock is gone from almost all code in linux-next, this is
> the status of what I think will happen to the remaining users:

> fs/qnx4:
> 	Should be easy to fix, there are only a few places in the code that
> 	use the BKL. Anders?

Will do.

Cheers
Anders

Re: Remaining BKL users, what to do

[Samuel Ortiz]

On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
> net/appletalk:
> net/ipx/af_ipx.c:
> net/irda/af_irda.c:
> 	Can probably be saved from retirement in drivers/staging if the
> 	maintainers still care.
I'll take care of the IrDA part.

Cheers,
Samuel.

Re: Remaining BKL users, what to do

[David Miller]

From: Samuel Ortiz <samuel@sortiz.org>
Date: Thu, 16 Sep 2010 18:57:56 +0200

> On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
>> net/appletalk:
>> net/ipx/af_ipx.c:
>> net/irda/af_irda.c:
>> 	Can probably be saved from retirement in drivers/staging if the
>> 	maintainers still care.
> I'll take care of the IrDA part.

Thanks a lot Sam.

Re: Remaining BKL users, what to do

[Jan Harkes]

On Thu, Sep 16, 2010 at 11:19:28AM -0400, Arnd Bergmann wrote:
> fs/coda:
> 	Coda seems to have an active community, but not all of their
> 	code is actually part of linux (pioctl!),

Probably more than 99% of the code is running in userspace. And that
isn't really pioctl related, a pioctl is simply an ioctl that can in a
somewhat cross-platform way operate not just on files, but also on
symlinks and directories because it is path based instead of fd based.

These path-ioctls are there for user commands on specific objects
visible in the filesystem, examining and repairing conflicts, forcing
cache refresh or prefetch, accessing extended information such as which
servers store a replica of the object.

Some of this could move to extended attributes, other things might work
without kernel involvement over a unix-domain socket, but the hard part
is that our userspace code is mostly identical across several systems
and such features aren't universally available.

> 	                                          while the last official
> 	release is missing many of the cleanups that were don in Linux.

And some of those might have broken at least mounting the Coda file
system. I haven't bisected yet, but I got a report that on a recent RC
kernel any attempt to start Coda caused some sort of lockup.

> 	Just removing the BKL without the Coda community seems like a heap
> 	of pointless work.

It depends, it might get rid of that mount lockup. There shouldn't be
too much shared state because the kernel module mostly just forwards
requests to userspace and the BKL right now seems to be mostly used to
protects access to the upcall lists and could probably without too much
trouble be replaced with a single 'global' (but Coda-only) or
mount-point specific mutex.

I have to figure out what broke mount first though.

Jan

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Thursday 16 September 2010 21:00:25 Jan Harkes wrote:
> >       Just removing the BKL without the Coda community seems like a heap
> >       of pointless work.
> 
> It depends, it might get rid of that mount lockup. There shouldn't be
> too much shared state because the kernel module mostly just forwards
> requests to userspace and the BKL right now seems to be mostly used to
> protects access to the upcall lists and could probably without too much
> trouble be replaced with a single 'global' (but Coda-only) or
> mount-point specific mutex.

Ok, that would be nice.

There are two strategies forward then based on the current code:

1. introduce a global or per-superblock mutex and convert all
instances of lock-kernel to that, then see what breaks (lockdep
helps here) and fix up all places where you get potential
deadlocks. The easiest replacement would be the existing superblock
mutex, doing s/lock_kernel()/lock_super(sb)/.

2. understand what data structures are actually being protected
by the BKL right now, then add proper locking around all accesses
to them and finally remove all uses of the BKL.

	Arnd

Re: [autofs] Remaining BKL users, what to do

[Ian Kent]

On Thu, 2010-09-16 at 16:32 +0200, Arnd Bergmann wrote:
> The big kernel lock is gone from almost all code in linux-next, this is
> the status of what I think will happen to the remaining users:
> 

...

> fs/autofs:
> 	Pretty much dead, replaced by autofs4. I'd suggest moving this
> 	to drivers/staging in 2.6.37 and letting it die there.

Not sure that's what we need.
What actually needs to happen is that autofs4 needs to be renamed to
autofs.

Ian

[v2] Remaining BKL users, what to do

[Arnd Bergmann]

This is a update on the current progress for the BKL removal, reflecting
what is currently in linux-next.

Maybe we can briefly discuss this at the kernel summit to decide if we
want a quick death of the BKL, i.e. fixing/disabling/staging-out the
remaining users in 2.6.38 or rather leave them there indefinitely.

On Thursday 16 September 2010, Arnd Bergmann wrote:
> The big kernel lock is gone from almost all code in linux-next, this is
> the status of what I think will happen to the remaining users:
> 
> drivers/gpu/drm/i810/{i810,i830}_dma.c:
> 	Fixable, but needs someone with the hardware to test. Can probably be
> 	marked CONFIG_BROKEN_ON_SMP if nobody cares.

Still open, no good solution for this.

> drivers/media/video (V4L):
> 	Mauro is working on it, some drivers get moved to staging while the
> 	others get fixed. An easy workaround would be possible by adding
> 	per-driver mutexes, but Mauro wants to it properly by locking all
> 	the right places.

Progressing well, patches are being worked on.

> fs/adfs:
> 	Probably not hard to fix, but needs someone to test it.
> 	adfs has only seen janitorial fixes for the last 5 years.
> 	Do we know of any users?

Nobody replied.

> fs/autofs:
> 	Pretty much dead, replaced by autofs4. I'd suggest moving this
> 	to drivers/staging in 2.6.37 and letting it die there.

Now in staging.

> fs/coda:
> 	Coda seems to have an active community, but not all of their
> 	code is actually part of linux (pioctl!), while the last official
> 	release is missing many of the cleanups that were don in Linux.
> 	Not sure what to do, if someone is interested, the best way might
> 	be a fresh start with a merger of the mainline linux and the
> 	coda.cs.cmu.edu	codebase in drivers/staging.
> 	Just removing the BKL without the Coda community seems like a heap
> 	of pointless work.

Jan Harkes showed interest, looks like this will get fixed eventually,
but probably not in time for 2.6.37.

> fs/freevxfs:
> 	Uses the BKL in readdir and lookup, should be easy to fix. Christoph?

Still waiting for confirmation from Christoph Hellwig that the BKL
is not needed here. I can do the patch to remove it then.

> fs/hpfs:
> 	Looks fixable, if anyone cares. Maybe it's time for retirement in
> 	drivers/staging though. The web page only has a Link to the
> 	linux-2.2 version.

No replies.

> fs/lockd:
> 	Trond writes that he has someone working on BKL removal here.

Bryan Schumaker took care of this, looks like the locking is independent
of the fs/locks.c locking now, although it still uses the BKL internally.

I assume that this will get fixed as well, doesn't seem hard. As long
as lockd uses the BKL, both nfs and nfsd depend on the BKL implicitly.

> fs/locks.c:
> 	Patch is under discussion, blocked by work on fs/lockd currently.

No longer blocked now, both lockd and ceph can deal with this converted
to spinlocks. I will follow up with the final patch once they hit mainline.

> fs/ncpfs:
> 	Should be fixable if Petr still cares about it. Otherwise suggest
> 	moving to drivers/staging if there are no users left.

Fixed by Petr Vandrovec.

> fs/qnx4:
> 	Should be easy to fix, there are only a few places in the code that
> 	use the BKL. Anders?

Anders Larsen volunteered.

> fs/smbfs:
> 	Last I heard this was considered obsolete. Should be move it to
> 	drivers/staging now?

Now in staging.

> fs/udf:
> 	Not completely trivial, but probably necessary to fix. Project web
> 	site is dead, I hope that Jan Kara can be motivated to fix it though.

Jan Kara volunteered to do it.

> fs/ufs:
> 	Evgeniy Dushistov is maintaining this, I hope he can take care of
> 	getting rid of the BKL in UFS.

No replies.

> kernel/trace/blktrace.c:
> 	Should be easy. Ingo? Steven?

Done.

> net/appletalk:
> net/ipx/af_ipx.c:
> net/irda/af_irda.c:
> 	Can probably be saved from retirement in drivers/staging if the
> 	maintainers still care.

Samuel Ortiz fixed irda.

David Miller volunteered to do appletalk and ipx.

> net/x25:
> 	Andrew Hendry has started working on it.

Patches have shown up in -next now, I suppose Andrew will finish it soon.

Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
up not getting fixed at all, we can either mark them non-SMP or move them
to drivers/staging once all the others are done.

	Arnd

Re: [v2] Remaining BKL users, what to do

[Christoph Hellwig]

Before we get into all these fringe drivers:

 - I've not seen any progrss on ->get_sb BKL removal for a while
 - locks.c is probably a higher priorit, too.

Re: [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Monday 18 October 2010 18:19:24 Christoph Hellwig wrote:
> Before we get into all these fringe drivers:
> 
>  - I've not seen any progrss on ->get_sb BKL removal for a while

Not sure what you mean. Jan Blunck did the pushdown into get_sb
last year, which is included into linux-next through my bkl/vfs
tree. Subsequent patches remove it from most file systems along with
the other BKL uses in them. If you like, I can post the series
once more, but it has been posted a few times now.

>  - locks.c is probably a higher priorit, too.

As mentioned in the list, I expect the trivial final patch to
be applied in 2.6.37-rc1 after Linus has pulled the trees that
this depends on (bkl/vfs, nfs, nfsd, ceph), see below.

This is currently not in -next because of the prerequisites.

	Arnd
---

diff --git a/fs/Kconfig b/fs/Kconfig
index c386a9f..25ce2dc 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -50,7 +50,6 @@ endif # BLOCK
 config FILE_LOCKING
 	bool "Enable POSIX file locking API" if EMBEDDED
 	default y
-	select BKL
 	help
 	  This option enables standard file locking support, required
           for filesystems like NFS and for the flock() system
diff --git a/fs/locks.c b/fs/locks.c
index 8b2b6ad..02b6e0e 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -142,6 +142,7 @@ int lease_break_time = 45;
 
 static LIST_HEAD(file_lock_list);
 static LIST_HEAD(blocked_list);
+static DEFINE_SPINLOCK(file_lock_lock);
 
 /*
  * Protects the two list heads above, plus the inode->i_flock list
@@ -149,13 +150,13 @@ static LIST_HEAD(blocked_list);
  */
 void lock_flocks(void)
 {
-	lock_kernel();
+	spin_lock(&file_lock_lock);
 }
 EXPORT_SYMBOL_GPL(lock_flocks);
 
 void unlock_flocks(void)
 {
-	unlock_kernel();
+	spin_unlock(&file_lock_lock);
 }
 EXPORT_SYMBOL_GPL(unlock_flocks);
 

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Greg KH]

On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
> 
> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
> up not getting fixed at all, we can either mark them non-SMP or move them
> to drivers/staging once all the others are done.

I recommend moving them to staging, and then retire them from there if
no one steps up to maintain them.

thanks,

greg k-h

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

On Tue, Oct 19, 2010 at 4:43 AM, Greg KH <greg@kroah.com> wrote:
> On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
>>
>> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
>> up not getting fixed at all, we can either mark them non-SMP or move them
>> to drivers/staging once all the others are done.
>
> I recommend moving them to staging, and then retire them from there if
> no one steps up to maintain them.

I think this sets a bad precedent, these drivers work fine. Removing
BKL from them is hard, and involves finding and booting hw that
developers don't have much time/interest in at the moment. Anyone who
has access to the i810 hw and has time to work out the locking has
more important things to be doing with modern hw, however it doesn't
mean we should just drop support for old drivers because they don't
have active maintainers. Removing the BKL from the kernel is a great
goal, but breaking userspace ABI by removing drivers isn't.

Dave.

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Greg KH]

On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
> On Tue, Oct 19, 2010 at 4:43 AM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
> >>
> >> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
> >> up not getting fixed at all, we can either mark them non-SMP or move them
> >> to drivers/staging once all the others are done.
> >
> > I recommend moving them to staging, and then retire them from there if
> > no one steps up to maintain them.
> 
> I think this sets a bad precedent, these drivers work fine. Removing
> BKL from them is hard, and involves finding and booting hw that
> developers don't have much time/interest in at the moment. Anyone who
> has access to the i810 hw and has time to work out the locking has
> more important things to be doing with modern hw, however it doesn't
> mean we should just drop support for old drivers because they don't
> have active maintainers. Removing the BKL from the kernel is a great
> goal, but breaking userspace ABI by removing drivers isn't.

Should we just restrict such drivers to only be able to build on UP
machines with preempt disabled so that the BKL could be safely removed
from them?

Or what other idea do you have as to what could be done here?

I do have access to this hardware, but its on an old single processor
laptop, so any work that it would take to help do this development,
really wouldn't be able to be tested to be valid at all.

thanks,

greg k-h

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

On Tue, Oct 19, 2010 at 10:40 AM, Greg KH <greg@kroah.com> wrote:
> On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
>> On Tue, Oct 19, 2010 at 4:43 AM, Greg KH <greg@kroah.com> wrote:
>> > On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
>> >>
>> >> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
>> >> up not getting fixed at all, we can either mark them non-SMP or move them
>> >> to drivers/staging once all the others are done.
>> >
>> > I recommend moving them to staging, and then retire them from there if
>> > no one steps up to maintain them.
>>
>> I think this sets a bad precedent, these drivers work fine. Removing
>> BKL from them is hard, and involves finding and booting hw that
>> developers don't have much time/interest in at the moment. Anyone who
>> has access to the i810 hw and has time to work out the locking has
>> more important things to be doing with modern hw, however it doesn't
>> mean we should just drop support for old drivers because they don't
>> have active maintainers. Removing the BKL from the kernel is a great
>> goal, but breaking userspace ABI by removing drivers isn't.
>
> Should we just restrict such drivers to only be able to build on UP
> machines with preempt disabled so that the BKL could be safely removed
> from them?
>
> Or what other idea do you have as to what could be done here?
>
> I do have access to this hardware, but its on an old single processor
> laptop, so any work that it would take to help do this development,
> really wouldn't be able to be tested to be valid at all.

There is only very rare case where the i830 driver might get used with
SMP and really I think that case is in the don't care place, since if
you have that hw you probably should be using i915 on it anyways.

So it really only leaves the problem case of what do distros do if we
mark things as BROKEN_ON_SMP, since no distro builds UP kernels and
when you boot the SMP kernels on UP they don't run as SMP so not
having the driver load on those is a problem. Maybe we just need some
sort of warn on smp if a smp unfriendly driver is loaded and we
transition to SMP mode. Though this sounds like either (a) something
we do now and I don't about it, (b) work.

Dave.

>
> thanks,
>
> greg k-h
>

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Greg KH]

On Tue, Oct 19, 2010 at 10:57:43AM +1000, Dave Airlie wrote:
> On Tue, Oct 19, 2010 at 10:40 AM, Greg KH <greg@kroah.com> wrote:
> > On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
> >> On Tue, Oct 19, 2010 at 4:43 AM, Greg KH <greg@kroah.com> wrote:
> >> > On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
> >> >>
> >> >> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
> >> >> up not getting fixed at all, we can either mark them non-SMP or move them
> >> >> to drivers/staging once all the others are done.
> >> >
> >> > I recommend moving them to staging, and then retire them from there if
> >> > no one steps up to maintain them.
> >>
> >> I think this sets a bad precedent, these drivers work fine. Removing
> >> BKL from them is hard, and involves finding and booting hw that
> >> developers don't have much time/interest in at the moment. Anyone who
> >> has access to the i810 hw and has time to work out the locking has
> >> more important things to be doing with modern hw, however it doesn't
> >> mean we should just drop support for old drivers because they don't
> >> have active maintainers. Removing the BKL from the kernel is a great
> >> goal, but breaking userspace ABI by removing drivers isn't.
> >
> > Should we just restrict such drivers to only be able to build on UP
> > machines with preempt disabled so that the BKL could be safely removed
> > from them?
> >
> > Or what other idea do you have as to what could be done here?
> >
> > I do have access to this hardware, but its on an old single processor
> > laptop, so any work that it would take to help do this development,
> > really wouldn't be able to be tested to be valid at all.
> 
> There is only very rare case where the i830 driver might get used with
> SMP and really I think that case is in the don't care place, since if
> you have that hw you probably should be using i915 on it anyways.

So, there is no need for the i830 driver?  Can it just be removed
because i915 works instead?

> So it really only leaves the problem case of what do distros do if we
> mark things as BROKEN_ON_SMP, since no distro builds UP kernels and
> when you boot the SMP kernels on UP they don't run as SMP so not
> having the driver load on those is a problem. Maybe we just need some
> sort of warn on smp if a smp unfriendly driver is loaded and we
> transition to SMP mode. Though this sounds like either (a) something
> we do now and I don't about it, (b) work.

So you are saying that just because distros will never build such a
thing, we should keep it building for SMP mode?  Why not prevent it from
being built and if a distro really cares, then they will pony up the
development to fix the driver up?

In other words, if someone really cares, then they will do the work,
otherwise why worry?  Especially as it seems that no one here is going
to do it, right?

thanks,

greg k-h

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

On Tue, Oct 19, 2010 at 12:24 PM, Greg KH <greg@kroah.com> wrote:
> On Tue, Oct 19, 2010 at 10:57:43AM +1000, Dave Airlie wrote:
>> On Tue, Oct 19, 2010 at 10:40 AM, Greg KH <greg@kroah.com> wrote:
>> > On Tue, Oct 19, 2010 at 09:00:09AM +1000, Dave Airlie wrote:
>> >> On Tue, Oct 19, 2010 at 4:43 AM, Greg KH <greg@kroah.com> wrote:
>> >> > On Mon, Oct 18, 2010 at 05:42:06PM +0200, Arnd Bergmann wrote:
>> >> >>
>> >> >> Out of the remaining modules, I guess i810/i830, adfs, hpfs and ufs might end
>> >> >> up not getting fixed at all, we can either mark them non-SMP or move them
>> >> >> to drivers/staging once all the others are done.
>> >> >
>> >> > I recommend moving them to staging, and then retire them from there if
>> >> > no one steps up to maintain them.
>> >>
>> >> I think this sets a bad precedent, these drivers work fine. Removing
>> >> BKL from them is hard, and involves finding and booting hw that
>> >> developers don't have much time/interest in at the moment. Anyone who
>> >> has access to the i810 hw and has time to work out the locking has
>> >> more important things to be doing with modern hw, however it doesn't
>> >> mean we should just drop support for old drivers because they don't
>> >> have active maintainers. Removing the BKL from the kernel is a great
>> >> goal, but breaking userspace ABI by removing drivers isn't.
>> >
>> > Should we just restrict such drivers to only be able to build on UP
>> > machines with preempt disabled so that the BKL could be safely removed
>> > from them?
>> >
>> > Or what other idea do you have as to what could be done here?
>> >
>> > I do have access to this hardware, but its on an old single processor
>> > laptop, so any work that it would take to help do this development,
>> > really wouldn't be able to be tested to be valid at all.
>>
>> There is only very rare case where the i830 driver might get used with
>> SMP and really I think that case is in the don't care place, since if
>> you have that hw you probably should be using i915 on it anyways.
>
> So, there is no need for the i830 driver?  Can it just be removed
> because i915 works instead?

No because it provides a different userspace ABI to the i915 driver to
a different userspace X driver etc.

like I'm sure the intersection of this driver and reality are getting
quite limited, but its still a userspace ABI change and needs to be
treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the
old driver/API.

>> So it really only leaves the problem case of what do distros do if we
>> mark things as BROKEN_ON_SMP, since no distro builds UP kernels and
>> when you boot the SMP kernels on UP they don't run as SMP so not
>> having the driver load on those is a problem. Maybe we just need some
>> sort of warn on smp if a smp unfriendly driver is loaded and we
>> transition to SMP mode. Though this sounds like either (a) something
>> we do now and I don't about it, (b) work.
>
> So you are saying that just because distros will never build such a
> thing, we should keep it building for SMP mode?  Why not prevent it from
> being built and if a distro really cares, then they will pony up the
> development to fix the driver up?

Distros build the driver now even it it didn't work on SMP it wouldn't
matter to the 99% of people who have this hw since it can't suppport
SMP except in some corner cases. So not building for SMP is the same
as just throwing it out of the kernel since most people don't run
kernel.org kernels, and shouldn't have to just to get a driver for
some piece of hardware that worked fine up until now.

Look at this from a user who has this hardware pov, it works for them
now with a distro kernel, us breaking it isn't going to help that user
or make any distro care, its just going to screw over the people who
are actually using it.

> In other words, if someone really cares, then they will do the work,
> otherwise why worry?  Especially as it seems that no one here is going
> to do it, right?

Well the thing is doing the work right is a non-trivial task and just
dropping support only screws the people using the hardware,
it doesn't place any burden on the distro developers to fix it up. If
people are really serious about making the BKL go away completely, I
think the onus should be on them to fix the drivers not on the users
who are using it, like I'm  guessing if this gets broken the bug will
end up in Novell or RH bugzilla in a year and nobody will ever see it.

Dave.

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Steven Rostedt]

On Tue, 2010-10-19 at 12:45 +1000, Dave Airlie wrote:
> On Tue, Oct 19, 2010 at 12:24 PM, Greg KH <greg@kroah.com> wrote:

> > So, there is no need for the i830 driver?  Can it just be removed
> > because i915 works instead?
> 
> No because it provides a different userspace ABI to the i915 driver to
> a different userspace X driver etc.
> 
> like I'm sure the intersection of this driver and reality are getting
> quite limited, but its still a userspace ABI change and needs to be
> treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the
> old driver/API.

Thus, you are saying that this will break for people with older user
apps and have a newer kernel?

> 
> >> So it really only leaves the problem case of what do distros do if we
> >> mark things as BROKEN_ON_SMP, since no distro builds UP kernels and
> >> when you boot the SMP kernels on UP they don't run as SMP so not
> >> having the driver load on those is a problem. Maybe we just need some
> >> sort of warn on smp if a smp unfriendly driver is loaded and we
> >> transition to SMP mode. Though this sounds like either (a) something
> >> we do now and I don't about it, (b) work.
> >
> > So you are saying that just because distros will never build such a
> > thing, we should keep it building for SMP mode?  Why not prevent it from
> > being built and if a distro really cares, then they will pony up the
> > development to fix the driver up?
> 
> Distros build the driver now even it it didn't work on SMP it wouldn't
> matter to the 99% of people who have this hw since it can't suppport
> SMP except in some corner cases. So not building for SMP is the same
> as just throwing it out of the kernel since most people don't run
> kernel.org kernels, and shouldn't have to just to get a driver for
> some piece of hardware that worked fine up until now.

Ah! Exactly! Thus, those that do not run kernel.org kernels are using a
distro kernel. Wont these same people use the distro userspace? That is,
if they have upgraded their kernel, most likely, they also update their
X interface.

> 
> Look at this from a user who has this hardware pov, it works for them
> now with a distro kernel, us breaking it isn't going to help that user
> or make any distro care, its just going to screw over the people who
> are actually using it.

But they can use the i915 driver instead, because they are using the
newer userspace apps.

> 
> > In other words, if someone really cares, then they will do the work,
> > otherwise why worry?  Especially as it seems that no one here is going
> > to do it, right?
> 
> Well the thing is doing the work right is a non-trivial task and just
> dropping support only screws the people using the hardware,
> it doesn't place any burden on the distro developers to fix it up. If
> people are really serious about making the BKL go away completely, I
> think the onus should be on them to fix the drivers not on the users
> who are using it, like I'm  guessing if this gets broken the bug will
> end up in Novell or RH bugzilla in a year and nobody will ever see it.

Well the problem comes down to testing it. I don't know of any developer
that is removing the BKL that actually owns hardware to test out these
broken drivers. And for the change not being trivial, means that there's
no way to do in correctly.

-- Steve

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

>>
>> like I'm sure the intersection of this driver and reality are getting
>> quite limited, but its still a userspace ABI change and needs to be
>> treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the
>> old driver/API.
>
> Thus, you are saying that this will break for people with older user
> apps and have a newer kernel?

There are two drivers here:

i810

i830

The i830 case is the case I care less about since the ABI is only used
by older userspace and i915 provides a replacement.

the i810 case ABI is still in use today by distro userspaces that are
still released, i.e. i810 is still used in F14, Ubuntu 10.10, RHEL6
Beta etc.

I've snipped the rest of the argument on the grounds you are
conflating two cases that aren't the same.

>
>>
>> Well the thing is doing the work right is a non-trivial task and just
>> dropping support only screws the people using the hardware,
>> it doesn't place any burden on the distro developers to fix it up. If
>> people are really serious about making the BKL go away completely, I
>> think the onus should be on them to fix the drivers not on the users
>> who are using it, like I'm  guessing if this gets broken the bug will
>> end up in Novell or RH bugzilla in a year and nobody will ever see it.
>
> Well the problem comes down to testing it. I don't know of any developer
> that is removing the BKL that actually owns hardware to test out these
> broken drivers. And for the change not being trivial, means that there's
> no way to do in correctly.
>

So we can drop i830 using deprecation, however its pointless since the
fix for i810 is the same fix for i830 if we can work out the fix.

Well the way to do it correctly is make it so if the driver is
initialised and we do an SMP transition we warn the users, or we make
BROKEN_ON_SMP into a runtime thing that warns when the driver is
loaded on an SMP system. The intersection of SMP and this hardware is
definitely a very very small number and a lot more workable.

Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Theodore Kilgore]

On Mon, 18 Oct 2010, Steven Rostedt wrote:

> On Tue, 2010-10-19 at 12:45 +1000, Dave Airlie wrote:
> > On Tue, Oct 19, 2010 at 12:24 PM, Greg KH <greg@kroah.com> wrote:
> 
> > > So, there is no need for the i830 driver?  Can it just be removed
> > > because i915 works instead?
> > 
> > No because it provides a different userspace ABI to the i915 driver to
> > a different userspace X driver etc.
> > 
> > like I'm sure the intersection of this driver and reality are getting
> > quite limited, but its still a userspace ABI change and needs to be
> > treated as such. Xorg 6.7 and XFree86 4.3 were the last users of the
> > old driver/API.
> 
> Thus, you are saying that this will break for people with older user
> apps and have a newer kernel?
> 
> > 
> > >> So it really only leaves the problem case of what do distros do if we
> > >> mark things as BROKEN_ON_SMP, since no distro builds UP kernels and
> > >> when you boot the SMP kernels on UP they don't run as SMP so not
> > >> having the driver load on those is a problem. Maybe we just need some
> > >> sort of warn on smp if a smp unfriendly driver is loaded and we
> > >> transition to SMP mode. Though this sounds like either (a) something
> > >> we do now and I don't about it, (b) work.
> > >
> > > So you are saying that just because distros will never build such a
> > > thing, we should keep it building for SMP mode?  Why not prevent it from
> > > being built and if a distro really cares, then they will pony up the
> > > development to fix the driver up?
> > 
> > Distros build the driver now even it it didn't work on SMP it wouldn't
> > matter to the 99% of people who have this hw since it can't suppport
> > SMP except in some corner cases. So not building for SMP is the same
> > as just throwing it out of the kernel since most people don't run
> > kernel.org kernels, and shouldn't have to just to get a driver for
> > some piece of hardware that worked fine up until now.
> 
> Ah! Exactly! Thus, those that do not run kernel.org kernels are using a
> distro kernel. Wont these same people use the distro userspace? That is,
> if they have upgraded their kernel, most likely, they also update their
> X interface.
> 
> > 
> > Look at this from a user who has this hardware pov, it works for them
> > now with a distro kernel, us breaking it isn't going to help that user
> > or make any distro care, its just going to screw over the people who
> > are actually using it.
> 
> But they can use the i915 driver instead, because they are using the
> newer userspace apps.
> 
> > 
> > > In other words, if someone really cares, then they will do the work,
> > > otherwise why worry?  Especially as it seems that no one here is going
> > > to do it, right?
> > 
> > Well the thing is doing the work right is a non-trivial task and just
> > dropping support only screws the people using the hardware,
> > it doesn't place any burden on the distro developers to fix it up. If
> > people are really serious about making the BKL go away completely, I
> > think the onus should be on them to fix the drivers not on the users
> > who are using it, like I'm  guessing if this gets broken the bug will
> > end up in Novell or RH bugzilla in a year and nobody will ever see it.
> 
> Well the problem comes down to testing it. I don't know of any developer
> that is removing the BKL that actually owns hardware to test out these
> broken drivers. And for the change not being trivial, means that there's
> no way to do in correctly.
> 
> -- Steve

I might be able to find some hardware still lying around here that uses an 
i810. Not sure unless I go hunting it. But I get the impression that if 
the kernel is a single-CPU kernel there is not any problem anyway? Don't 
distros offer a non-smp kernel as an installation option in case the user 
needs it? So in reality how big a problem is this?

Theodore Kilgore

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

> I might be able to find some hardware still lying around here that uses an
> i810. Not sure unless I go hunting it. But I get the impression that if
> the kernel is a single-CPU kernel there is not any problem anyway? Don't
> distros offer a non-smp kernel as an installation option in case the user
> needs it? So in reality how big a problem is this?

Not anymore, which is my old point of making a fuss. Nowadays in the
modern distro world, we supply a single kernel that can at runtime
decide if its running on SMP or UP and rewrite the text section
appropriately with locks etc. Its like magic, and something like
marking drivers as BROKEN_ON_SMP at compile time is really wrong when
what you want now is a runtime warning if someone tries to hotplug a
CPU with a known iffy driver loaded or if someone tries to load the
driver when we are already in SMP mode.

Dave.

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
> > I might be able to find some hardware still lying around here that uses an
> > i810. Not sure unless I go hunting it. But I get the impression that if
> > the kernel is a single-CPU kernel there is not any problem anyway? Don't
> > distros offer a non-smp kernel as an installation option in case the user
> > needs it? So in reality how big a problem is this?
> 
> Not anymore, which is my old point of making a fuss. Nowadays in the
> modern distro world, we supply a single kernel that can at runtime
> decide if its running on SMP or UP and rewrite the text section
> appropriately with locks etc. Its like magic, and something like
> marking drivers as BROKEN_ON_SMP at compile time is really wrong when
> what you want now is a runtime warning if someone tries to hotplug a
> CPU with a known iffy driver loaded or if someone tries to load the
> driver when we are already in SMP mode.

We could make the driver run-time non-SMP by adding

	if (num_present_cpus() > 1) {
		pr_err("i810 no longer supports SMP\n");
		return -EINVAL;
	}

to the init function. That would cover the vast majority of the
users of i810 hardware, I guess.

	Arnd

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Steven Rostedt]

On Tue, 2010-10-19 at 09:26 +0200, Arnd Bergmann wrote:
> On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
> > > I might be able to find some hardware still lying around here that uses an
> > > i810. Not sure unless I go hunting it. But I get the impression that if
> > > the kernel is a single-CPU kernel there is not any problem anyway? Don't
> > > distros offer a non-smp kernel as an installation option in case the user
> > > needs it? So in reality how big a problem is this?
> > 
> > Not anymore, which is my old point of making a fuss. Nowadays in the
> > modern distro world, we supply a single kernel that can at runtime
> > decide if its running on SMP or UP and rewrite the text section
> > appropriately with locks etc. Its like magic, and something like
> > marking drivers as BROKEN_ON_SMP at compile time is really wrong when
> > what you want now is a runtime warning if someone tries to hotplug a
> > CPU with a known iffy driver loaded or if someone tries to load the
> > driver when we are already in SMP mode.
> 
> We could make the driver run-time non-SMP by adding
> 
> 	if (num_present_cpus() > 1) {
> 		pr_err("i810 no longer supports SMP\n");
> 		return -EINVAL;
> 	}
> 
> to the init function. That would cover the vast majority of the
> users of i810 hardware, I guess.

I think we also need to cover the PREEMPT case too. But that could be a
compile time check, since you can't boot a preempt kernel and make it
non preempt.

-- Steve

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Paul Mundt]

On Tue, Oct 19, 2010 at 08:39:58AM -0400, Steven Rostedt wrote:
> On Tue, 2010-10-19 at 09:26 +0200, Arnd Bergmann wrote:
> > On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
> > > > I might be able to find some hardware still lying around here that uses an
> > > > i810. Not sure unless I go hunting it. But I get the impression that if
> > > > the kernel is a single-CPU kernel there is not any problem anyway? Don't
> > > > distros offer a non-smp kernel as an installation option in case the user
> > > > needs it? So in reality how big a problem is this?
> > > 
> > > Not anymore, which is my old point of making a fuss. Nowadays in the
> > > modern distro world, we supply a single kernel that can at runtime
> > > decide if its running on SMP or UP and rewrite the text section
> > > appropriately with locks etc. Its like magic, and something like
> > > marking drivers as BROKEN_ON_SMP at compile time is really wrong when
> > > what you want now is a runtime warning if someone tries to hotplug a
> > > CPU with a known iffy driver loaded or if someone tries to load the
> > > driver when we are already in SMP mode.
> > 
> > We could make the driver run-time non-SMP by adding
> > 
> > 	if (num_present_cpus() > 1) {
> > 		pr_err("i810 no longer supports SMP\n");
> > 		return -EINVAL;
> > 	}
> > 
> > to the init function. That would cover the vast majority of the
> > users of i810 hardware, I guess.
> 
> I think we also need to cover the PREEMPT case too. But that could be a
> compile time check, since you can't boot a preempt kernel and make it
> non preempt.
> 
There are enough nameless embedded vendors that have turned a preempt
kernel in to a non-preempt one at run-time by leaking the preempt count,
whether by design or not, so it's certainly possile :-)

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Tuesday 19 October 2010, Arnd Bergmann wrote:
> On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
> > > I might be able to find some hardware still lying around here that uses an
> > > i810. Not sure unless I go hunting it. But I get the impression that if
> > > the kernel is a single-CPU kernel there is not any problem anyway? Don't
> > > distros offer a non-smp kernel as an installation option in case the user
> > > needs it? So in reality how big a problem is this?
> > 
> > Not anymore, which is my old point of making a fuss. Nowadays in the
> > modern distro world, we supply a single kernel that can at runtime
> > decide if its running on SMP or UP and rewrite the text section
> > appropriately with locks etc. Its like magic, and something like
> > marking drivers as BROKEN_ON_SMP at compile time is really wrong when
> > what you want now is a runtime warning if someone tries to hotplug a
> > CPU with a known iffy driver loaded or if someone tries to load the
> > driver when we are already in SMP mode.
> 
> We could make the driver run-time non-SMP by adding
> 
> 	if (num_present_cpus() > 1) {
> 		pr_err("i810 no longer supports SMP\n");
> 		return -EINVAL;
> 	}
> 
> to the init function. That would cover the vast majority of the
> users of i810 hardware, I guess.

Some research showed that Intel never support i810/i815 SMP setups,
but there was indeed one company (http://www.acorpusa.com at the time,
now owned by a domain squatter) that made i815E based dual Pentium-III
boards like this one: http://cgi.ebay.com/280319795096

The first person that can send me an authentic log file showing the
use of X.org with DRM on a 2.6.35 kernel with two processors on that
mainboard dated today or earlier gets a free upgrade to an AGP graphics
card of comparable or better 3D performance from me. Please include
the story how why you are running this machine with a new kernel.

i830 is harder, apparently some i865G boards support Pentium 4 with HT
and even later dual-core processors.

	Arnd

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Airlie]

On Tue, Oct 19, 2010 at 11:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 19 October 2010, Arnd Bergmann wrote:
>> On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
>> > > I might be able to find some hardware still lying around here that uses an
>> > > i810. Not sure unless I go hunting it. But I get the impression that if
>> > > the kernel is a single-CPU kernel there is not any problem anyway? Don't
>> > > distros offer a non-smp kernel as an installation option in case the user
>> > > needs it? So in reality how big a problem is this?
>> >
>> > Not anymore, which is my old point of making a fuss. Nowadays in the
>> > modern distro world, we supply a single kernel that can at runtime
>> > decide if its running on SMP or UP and rewrite the text section
>> > appropriately with locks etc. Its like magic, and something like
>> > marking drivers as BROKEN_ON_SMP at compile time is really wrong when
>> > what you want now is a runtime warning if someone tries to hotplug a
>> > CPU with a known iffy driver loaded or if someone tries to load the
>> > driver when we are already in SMP mode.
>>
>> We could make the driver run-time non-SMP by adding
>>
>>       if (num_present_cpus() > 1) {
>>               pr_err("i810 no longer supports SMP\n");
>>               return -EINVAL;
>>       }
>>
>> to the init function. That would cover the vast majority of the
>> users of i810 hardware, I guess.
>
> Some research showed that Intel never support i810/i815 SMP setups,
> but there was indeed one company (http://www.acorpusa.com at the time,
> now owned by a domain squatter) that made i815E based dual Pentium-III
> boards like this one: http://cgi.ebay.com/280319795096

Also that board has no on-board GPU enabled i815EP (P means no on-board GPU).

So I think i810 is fine.
>
> The first person that can send me an authentic log file showing the
> use of X.org with DRM on a 2.6.35 kernel with two processors on that
> mainboard dated today or earlier gets a free upgrade to an AGP graphics
> card of comparable or better 3D performance from me. Please include
> the story how why you are running this machine with a new kernel.
>
> i830 is harder, apparently some i865G boards support Pentium 4 with HT
> and even later dual-core processors.

Also hyper-threaded 845G boards, however I'm happy to start a proper
deprecation procedure on the i830 ABI,
Its been a few years since a distro shipped with it, I think even
RHEL5 has the i915 driver enabled, so we are
probably talking RHEL4 era.

Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Ville Syrjälä]

On Wed, Oct 20, 2010 at 06:50:58AM +1000, Dave Airlie wrote:
> On Tue, Oct 19, 2010 at 11:26 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 19 October 2010, Arnd Bergmann wrote:
> >> On Tuesday 19 October 2010 06:52:32 Dave Airlie wrote:
> >> > > I might be able to find some hardware still lying around here that uses an
> >> > > i810. Not sure unless I go hunting it. But I get the impression that if
> >> > > the kernel is a single-CPU kernel there is not any problem anyway? Don't
> >> > > distros offer a non-smp kernel as an installation option in case the user
> >> > > needs it? So in reality how big a problem is this?
> >> >
> >> > Not anymore, which is my old point of making a fuss. Nowadays in the
> >> > modern distro world, we supply a single kernel that can at runtime
> >> > decide if its running on SMP or UP and rewrite the text section
> >> > appropriately with locks etc. Its like magic, and something like
> >> > marking drivers as BROKEN_ON_SMP at compile time is really wrong when
> >> > what you want now is a runtime warning if someone tries to hotplug a
> >> > CPU with a known iffy driver loaded or if someone tries to load the
> >> > driver when we are already in SMP mode.
> >>
> >> We could make the driver run-time non-SMP by adding
> >>
> >>       if (num_present_cpus() > 1) {
> >>               pr_err("i810 no longer supports SMP\n");
> >>               return -EINVAL;
> >>       }
> >>
> >> to the init function. That would cover the vast majority of the
> >> users of i810 hardware, I guess.
> >
> > Some research showed that Intel never support i810/i815 SMP setups,
> > but there was indeed one company (http://www.acorpusa.com at the time,
> > now owned by a domain squatter) that made i815E based dual Pentium-III
> > boards like this one: http://cgi.ebay.com/280319795096
> 
> Also that board has no on-board GPU enabled i815EP (P means no on-board GPU).

A quick search seems to indicate that an i815E variant also existed.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Valdis.Kletnieks]

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

On Mon, 18 Oct 2010 17:40:04 PDT, Greg KH said:

> I do have access to this hardware, but its on an old single processor
> laptop, so any work that it would take to help do this development,
> really wouldn't be able to be tested to be valid at all.

The i810 is a graphics chipset embedded on the memory controller, which
was designed for the Intel Pentium II, Pentium III, and Celeron CPUs.  Page 8
of the datasheet specifically says:

Processor/Host Bus Support
- Optimized for the Intel Pentium II processor, Intel Pentium III processor, and Intel
CeleronTM processor
- Supports processor 370-Pin Socket and SC242
connectors
- Supports 32-Bit System Bus Addressing
- 4 deep in-order queue; 4 or 1 deep request queue
- Supports Uni-processor systems only

So no need to clean it up for multiprocessor support.

http://download.intel.com/design/chipsets/datashts/29067602.pdf
http://www.intel.com/design/chipsets/specupdt/29069403.pdf



[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Greg KH]

On Tue, Oct 19, 2010 at 02:24:53PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 18 Oct 2010 17:40:04 PDT, Greg KH said:
> 
> > I do have access to this hardware, but its on an old single processor
> > laptop, so any work that it would take to help do this development,
> > really wouldn't be able to be tested to be valid at all.
> 
> The i810 is a graphics chipset embedded on the memory controller, which
> was designed for the Intel Pentium II, Pentium III, and Celeron CPUs.  Page 8
> of the datasheet specifically says:
> 
> Processor/Host Bus Support
> - Optimized for the Intel Pentium II processor, Intel Pentium III processor, and Intel
> CeleronTM processor
> - Supports processor 370-Pin Socket and SC242
> connectors
> - Supports 32-Bit System Bus Addressing
> - 4 deep in-order queue; 4 or 1 deep request queue
> - Supports Uni-processor systems only
> 
> So no need to clean it up for multiprocessor support.
> 
> http://download.intel.com/design/chipsets/datashts/29067602.pdf
> http://www.intel.com/design/chipsets/specupdt/29069403.pdf

Great, we can just drop all calls to lock_kernel() and the like in the
driver and be done with it, right?

thanks,

greg k-h

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Oliver Neukum]

Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
> > So no need to clean it up for multiprocessor support.
> > 
> > http://download.intel.com/design/chipsets/datashts/29067602.pdf
> > http://www.intel.com/design/chipsets/specupdt/29069403.pdf
> 
> Great, we can just drop all calls to lock_kernel() and the like in the
> driver and be done with it, right?

No,

you still need to switch off preemption.

	Regards
		Oliver

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Greg KH]

On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
> Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
> > > So no need to clean it up for multiprocessor support.
> > > 
> > > http://download.intel.com/design/chipsets/datashts/29067602.pdf
> > > http://www.intel.com/design/chipsets/specupdt/29069403.pdf
> > 
> > Great, we can just drop all calls to lock_kernel() and the like in the
> > driver and be done with it, right?
> 
> No,
> 
> you still need to switch off preemption.

Hm, how would you do that from within a driver?

thanks,

greg k-h

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Jiri Kosina]

On Tue, 19 Oct 2010, Greg KH wrote:

> > > > So no need to clean it up for multiprocessor support.
> > > > 
> > > > http://download.intel.com/design/chipsets/datashts/29067602.pdf
> > > > http://www.intel.com/design/chipsets/specupdt/29069403.pdf
> > > 
> > > Great, we can just drop all calls to lock_kernel() and the like in the
> > > driver and be done with it, right?
> > 
> > No,
> > 
> > you still need to switch off preemption.
> 
> Hm, how would you do that from within a driver?

preempt_disable()

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Alan Cox]

> > you still need to switch off preemption.
> 
> Hm, how would you do that from within a driver?

Do we care - unless I misunderstand the current intel DRM driver handles
the i810 as well ?

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Tuesday 19 October 2010 22:41:22 Alan Cox wrote:
> > > you still need to switch off preemption.
> > 
> > Hm, how would you do that from within a driver?
> 
> Do we care - unless I misunderstand the current intel DRM driver handles
> the i810 as well ?

No, it does handle all devices supported by i830.ko (830M, 845M, 854, 855GM,
865G), but not those supported by i810.ko (810, 815).

	Arnd

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Tuesday 19 October 2010 22:29:12 Greg KH wrote:
> On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
> > Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
> > > > So no need to clean it up for multiprocessor support.
> > > > 
> > > > http://download.intel.com/design/chipsets/datashts/29067602.pdf
> > > > http://www.intel.com/design/chipsets/specupdt/29069403.pdf
> > > 
> > > Great, we can just drop all calls to lock_kernel() and the like in the
> > > driver and be done with it, right?
> > 
> > No,
> > 
> > you still need to switch off preemption.
> 
> Hm, how would you do that from within a driver?

I think this would do:
---
drm/i810: remove SMP support and BKL

The i810 and i815 chipsets supported by the i810 drm driver were not
officially designed for SMP operation, so the big kernel lock is
only required for kernel preemption. This disables the driver if
preemption is enabled and removes all calls to lock_kernel in it.

If you own an Acorp 6A815EPD mainboard with a i815 chipset and
two Pentium-III sockets, and want to run recent kernels on it,
tell me about it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b755301..e071bc8 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -73,8 +73,8 @@ source "drivers/gpu/drm/radeon/Kconfig"
 
 config DRM_I810
 	tristate "Intel I810"
-	# BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
-	depends on DRM && AGP && AGP_INTEL && BKL
+	# PREEMPT requires BKL support here, which was removed
+	depends on DRM && AGP && AGP_INTEL && !PREEMPT
 	help
 	  Choose this option if you have an Intel I810 graphics card.  If M is
 	  selected, the module will be called i810.  AGP support is required
diff --git a/drivers/gpu/drm/i810/i810_dma.c b/drivers/gpu/drm/i810/i810_dma.c
index ff33e53..8f371e8 100644
--- a/drivers/gpu/drm/i810/i810_dma.c
+++ b/drivers/gpu/drm/i810/i810_dma.c
@@ -37,7 +37,6 @@
 #include <linux/interrupt.h>	/* For task queue support */
 #include <linux/delay.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/pagemap.h>
 
 #define I810_BUF_FREE		2
@@ -94,7 +93,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	struct drm_buf *buf;
 	drm_i810_buf_priv_t *buf_priv;
 
-	lock_kernel();
 	dev = priv->minor->dev;
 	dev_priv = dev->dev_private;
 	buf = dev_priv->mmap_buffer;
@@ -104,7 +102,6 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_file = filp;
 
 	buf_priv->currently_mapped = I810_BUF_MAPPED;
-	unlock_kernel();
 
 	if (io_remap_pfn_range(vma, vma->vm_start,
 			       vma->vm_pgoff,
@@ -116,7 +113,7 @@ static int i810_mmap_buffers(struct file *filp, struct vm_area_struct *vma)
 static const struct file_operations i810_buffer_fops = {
 	.open = drm_open,
 	.release = drm_release,
-	.unlocked_ioctl = i810_ioctl,
+	.unlocked_ioctl = drm_ioctl,
 	.mmap = i810_mmap_buffers,
 	.fasync = drm_fasync,
 	.llseek = noop_llseek,
@@ -1242,19 +1239,6 @@ int i810_driver_dma_quiescent(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * call the drm_ioctl under the big kernel lock because
- * to lock against the i810_mmap_buffers function.
- */
-long i810_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	int ret;
-	lock_kernel();
-	ret = drm_ioctl(file, cmd, arg);
-	unlock_kernel();
-	return ret;
-}
-
 struct drm_ioctl_desc i810_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I810_INIT, i810_dma_init, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I810_VERTEX, i810_dma_vertex, DRM_AUTH|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/i810/i810_drv.c b/drivers/gpu/drm/i810/i810_drv.c
index 88bcd33..9642d3c 100644
--- a/drivers/gpu/drm/i810/i810_drv.c
+++ b/drivers/gpu/drm/i810/i810_drv.c
@@ -57,7 +57,7 @@ static struct drm_driver driver = {
 		 .owner = THIS_MODULE,
 		 .open = drm_open,
 		 .release = drm_release,
-		 .unlocked_ioctl = i810_ioctl,
+		 .unlocked_ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
 		 .fasync = drm_fasync,
@@ -79,6 +79,10 @@ static struct drm_driver driver = {
 
 static int __init i810_init(void)
 {
+	if (num_present_cpus() > 1) {
+		pr_err("drm/i810 does not support SMP\n");
+		return -EINVAL;
+	}
 	driver.num_ioctls = i810_max_ioctl;
 	return drm_init(&driver);
 }

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Dave Young]

On Wed, Oct 20, 2010 at 4:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 19 October 2010 22:29:12 Greg KH wrote:
>> On Tue, Oct 19, 2010 at 09:40:47PM +0200, Oliver Neukum wrote:
>> > Am Dienstag, 19. Oktober 2010, 21:37:35 schrieb Greg KH:
>> > > > So no need to clean it up for multiprocessor support.
>> > > >
>> > > > http://download.intel.com/design/chipsets/datashts/29067602.pdf
>> > > > http://www.intel.com/design/chipsets/specupdt/29069403.pdf
>> > >
>> > > Great, we can just drop all calls to lock_kernel() and the like in the
>> > > driver and be done with it, right?
>> >
>> > No,
>> >
>> > you still need to switch off preemption.
>>
>> Hm, how would you do that from within a driver?
>
> I think this would do:
> ---
> drm/i810: remove SMP support and BKL
>
> The i810 and i815 chipsets supported by the i810 drm driver were not
> officially designed for SMP operation, so the big kernel lock is
> only required for kernel preemption. This disables the driver if
> preemption is enabled and removes all calls to lock_kernel in it.
>
> If you own an Acorp 6A815EPD mainboard with a i815 chipset and
> two Pentium-III sockets, and want to run recent kernels on it,
> tell me about it.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index b755301..e071bc8 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -73,8 +73,8 @@ source "drivers/gpu/drm/radeon/Kconfig"
>
>  config DRM_I810
>        tristate "Intel I810"
> -       # BKL usage in order to avoid AB-BA deadlocks, may become BROKEN_ON_SMP
> -       depends on DRM && AGP && AGP_INTEL && BKL
> +       # PREEMPT requires BKL support here, which was removed
> +       depends on DRM && AGP && AGP_INTEL && !PREEMPT

be curious, why can't just fix the lock_kernel logic of i810? Fixing
is too hard?

Find a i810 hardware should be possible, even if the hardware does not
support SMP, can't we test the fix with preemption?

-- 
Regards
dave

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Arnd Bergmann]

On Wednesday 20 October 2010, Dave Young wrote:
> be curious, why can't just fix the lock_kernel logic of i810? Fixing
> is too hard?
> 
> Find a i810 hardware should be possible, even if the hardware does not
> support SMP, can't we test the fix with preemption?

Yes, that should work too. My usual approach for removing the BKL without
having the hardware myself was to make locking stricter, i.e. replace
the BKL with a new spinlock or mutex. This way all the code would still
be serialized and if I did something wrong, lockdep would complain about
it, but there would be no risk of silent data corruption.

In case of i810, locking across DRM is rather complicated and there is no
way of doing this without making changes to other DRM code.

In fact, the only critical section that is actually protected by the BKL
are the few lines in i810_mmap_buffers. They look like they might not even
need the BKL to start with and we can just remove it even on SMP/PREEMPT,
except for perhaps the assignment to buf_priv->currently_mapped.
Someone who understands more about the driver than I do can probably figure
this out easily, but I couldn't come up with a way that doesn't risk
breaking in corner cases.

	Arnd

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Pavel Machek]

Hi!

> @@ -79,6 +79,10 @@ static struct drm_driver driver = {
>  
>  static int __init i810_init(void)
>  {
> +	if (num_present_cpus() > 1) {
> +		pr_err("drm/i810 does not support SMP\n");
> +		return -EINVAL;
> +	}
>  	driver.num_ioctls = i810_max_ioctl;
>  	return drm_init(&driver);

Umm, and now someone onlines second cpu?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Ksummit-2010-discuss] [v2] Remaining BKL users, what to do

[Pekka Enberg]

On Tue, Nov 2, 2010 at 3:21 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> @@ -79,6 +79,10 @@ static struct drm_driver driver = {
>>
>>  static int __init i810_init(void)
>>  {
>> +     if (num_present_cpus() > 1) {
>> +             pr_err("drm/i810 does not support SMP\n");
>> +             return -EINVAL;
>> +     }
>>       driver.num_ioctls = i810_max_ioctl;
>>       return drm_init(&driver);
>
> Umm, and now someone onlines second cpu?

Well, I see it's being fixed in a different way but yeah,
num_possible_cpus() would be more appropriate here.

Re: Remaining BKL users, what to do

[Christoph Hellwig]

On Thu, Sep 16, 2010 at 04:32:59PM +0200, Arnd Bergmann wrote:
> fs/freevxfs:
> 	Uses the BKL in readdir and lookup, should be easy to fix. Christoph?

Can simply be dropped.  Ditto for ->put_super and the not yet done
->get_sb pushdown.  It's a trivial read-only filesystem with no state
of it's own.

> 
> fs/hpfs:
> 	Looks fixable, if anyone cares. Maybe it's time for retirement in
> 	drivers/staging though. The web page only has a Link to the
> 	linux-2.2 version.

Di you ping Mikulas?  He's not done too lately but at least ACKed a few
patches.

> fs/ncpfs:
> 	Should be fixable if Petr still cares about it. Otherwise suggest
> 	moving to drivers/staging if there are no users left.

Didn't he fix it recently?

> fs/qnx4:
> 	Should be easy to fix, there are only a few places in the code that
> 	use the BKL. Anders?

Another trivial read-only fs.  Can be trivially dropped.

> fs/ufs:
> 	Evgeniy Dushistov is maintaining this, I hope he can take care of
> 	getting rid of the BKL in UFS.

Did you ping him.  Shouldn't be too hard despite your probably unfixable
comment that somehow made it to linux-next.

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Thursday 21 October 2010, Christoph Hellwig wrote:
> On Thu, Sep 16, 2010 at 04:32:59PM +0200, Arnd Bergmann wrote:
> > fs/freevxfs:
> > 	Uses the BKL in readdir and lookup, should be easy to fix. Christoph?
> 
> Can simply be dropped.  Ditto for ->put_super and the not yet done
> ->get_sb pushdown.  It's a trivial read-only filesystem with no state
> of it's own.

Cool, thanks for looking at it. I'll add a patch to my series.

> > 
> > fs/hpfs:
> > 	Looks fixable, if anyone cares. Maybe it's time for retirement in
> > 	drivers/staging though. The web page only has a Link to the
> > 	linux-2.2 version.
> 
> Di you ping Mikulas?  He's not done too lately but at least ACKed a few
> patches.

He was on Cc on all these mails, maybe he got overwhelmed by the amount
of mail.

> > fs/ncpfs:
> > 	Should be fixable if Petr still cares about it. Otherwise suggest
> > 	moving to drivers/staging if there are no users left.
> 
> Didn't he fix it recently?

Yes, see my update "[v2] Remaining BKL users, what to do". Note that the
mail you just replied to is a month old.

> > fs/qnx4:
> > 	Should be easy to fix, there are only a few places in the code that
> > 	use the BKL. Anders?
> 
> Another trivial read-only fs.  Can be trivially dropped.

ok.

> > fs/ufs:
> > 	Evgeniy Dushistov is maintaining this, I hope he can take care of
> > 	getting rid of the BKL in UFS.
> 
> Did you ping him.  Shouldn't be too hard despite your probably unfixable
> comment that somehow made it to linux-next.

He was on Cc as well. I'll update the comment and others that have become
obsolete now. I didn't realize that this was a read-only fs, which certainly
makes it easier.

Thanks,

	Arnd

[PATCH 1/2] BKL: remove BKL from qnx4

[Arnd Bergmann]

All uses of the BKL in qnx4 were the result of a pushdown into
code that doesn't really need it. As Christoph points out, this
is a read-only file system, which eliminates most of the races in
readdir/lookup.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Anders Larsen <al@alarsen.net>
Cc: Christoph Hellwig <hch@infradead.org>
---
 fs/qnx4/dir.c   |    3 ---
 fs/qnx4/inode.c |   14 +-------------
 fs/qnx4/namei.c |    4 ----
 3 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index 6e8fc62..06de116 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -11,7 +11,6 @@
  * 20-06-1998 by Frank Denis : Linux 2.1.99+ & dcache support.
  */
 
-#include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include "qnx4.h"
 
@@ -29,8 +28,6 @@ static int qnx4_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	QNX4DEBUG((KERN_INFO "qnx4_readdir:i_size = %ld\n", (long) inode->i_size));
 	QNX4DEBUG((KERN_INFO "filp->f_pos         = %ld\n", (long) filp->f_pos));
 
-	lock_kernel();
-
 	while (filp->f_pos < inode->i_size) {
 		blknum = qnx4_block_map( inode, filp->f_pos >> QNX4_BLOCK_SIZE_BITS );
 		bh = sb_bread(inode->i_sb, blknum);
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 86a7be1..01bad30 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -16,7 +16,6 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/highuid.h>
-#include <linux/smp_lock.h>
 #include <linux/pagemap.h>
 #include <linux/buffer_head.h>
 #include <linux/writeback.h>
@@ -157,8 +156,6 @@ static int qnx4_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct super_block *sb = dentry->d_sb;
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 
-	lock_kernel();
-
 	buf->f_type    = sb->s_magic;
 	buf->f_bsize   = sb->s_blocksize;
 	buf->f_blocks  = le32_to_cpu(qnx4_sb(sb)->BitMap->di_size) * 8;
@@ -168,8 +165,6 @@ static int qnx4_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_fsid.val[0] = (u32)id;
 	buf->f_fsid.val[1] = (u32)(id >> 32);
 
-	unlock_kernel();
-
 	return 0;
 }
 
@@ -234,13 +229,9 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
 	struct qnx4_sb_info *qs;
 	int ret = -EINVAL;
 
-	lock_kernel();
-
 	qs = kzalloc(sizeof(struct qnx4_sb_info), GFP_KERNEL);
-	if (!qs) {
-		unlock_kernel();
+	if (!qs)
 		return -ENOMEM;
-	}
 	s->s_fs_info = qs;
 
 	sb_set_blocksize(s, QNX4_BLOCK_SIZE);
@@ -287,8 +278,6 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
  		goto outi;
 
 	brelse(bh);
-
-	unlock_kernel();
 	return 0;
 
       outi:
@@ -298,7 +287,6 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
       outnobh:
 	kfree(qs);
 	s->s_fs_info = NULL;
-	unlock_kernel();
 	return ret;
 }
 
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 58703eb..275327b 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -12,7 +12,6 @@
  * 04-07-1998 by Frank Denis : first step for rmdir/unlink.
  */
 
-#include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include "qnx4.h"
 
@@ -109,7 +108,6 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, struct nam
 	int len = dentry->d_name.len;
 	struct inode *foundinode = NULL;
 
-	lock_kernel();
 	if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
 		goto out;
 	/* The entry is linked, let's get the real info */
@@ -123,13 +121,11 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, struct nam
 
 	foundinode = qnx4_iget(dir->i_sb, ino);
 	if (IS_ERR(foundinode)) {
-		unlock_kernel();
 		QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n",
 			   PTR_ERR(foundinode)));
 		return ERR_CAST(foundinode);
 	}
 out:
-	unlock_kernel();
 	d_add(dentry, foundinode);
 
 	return NULL;
-- 
1.7.1

Re: [PATCH 1/2] BKL: remove BKL from qnx4

[Anders Larsen]

On 2010-10-21 15:49:59, Arnd Bergmann wrote:
> All uses of the BKL in qnx4 were the result of a pushdown into
> code that doesn't really need it. As Christoph points out, this
> is a read-only file system, which eliminates most of the races in
> readdir/lookup.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Anders Larsen <al@alarsen.net>
> Cc: Christoph Hellwig <hch@infradead.org>

Acked-by: Anders Larsen <al@alarsen.net>

> ---
>  fs/qnx4/dir.c   |    3 ---
>  fs/qnx4/inode.c |   14 +-------------
>  fs/qnx4/namei.c |    4 ----
>  3 files changed, 1 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
> index 6e8fc62..06de116 100644
> --- a/fs/qnx4/dir.c
> +++ b/fs/qnx4/dir.c
> @@ -11,7 +11,6 @@
>   * 20-06-1998 by Frank Denis : Linux 2.1.99+ & dcache support.
>   */
>  
> -#include <linux/smp_lock.h>
>  #include <linux/buffer_head.h>
>  #include "qnx4.h"
>  
> @@ -29,8 +28,6 @@ static int qnx4_readdir(struct file *filp, void *dirent, filldir_t filldir)
>  	QNX4DEBUG((KERN_INFO "qnx4_readdir:i_size = %ld\n", (long) inode->i_size));
>  	QNX4DEBUG((KERN_INFO "filp->f_pos         = %ld\n", (long) filp->f_pos));
>  
> -	lock_kernel();
> -
>  	while (filp->f_pos < inode->i_size) {
>  		blknum = qnx4_block_map( inode, filp->f_pos >> QNX4_BLOCK_SIZE_BITS );
>  		bh = sb_bread(inode->i_sb, blknum);
> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> index 86a7be1..01bad30 100644
> --- a/fs/qnx4/inode.c
> +++ b/fs/qnx4/inode.c
> @@ -16,7 +16,6 @@
>  #include <linux/init.h>
>  #include <linux/slab.h>
>  #include <linux/highuid.h>
> -#include <linux/smp_lock.h>
>  #include <linux/pagemap.h>
>  #include <linux/buffer_head.h>
>  #include <linux/writeback.h>
> @@ -157,8 +156,6 @@ static int qnx4_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	struct super_block *sb = dentry->d_sb;
>  	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
>  
> -	lock_kernel();
> -
>  	buf->f_type    = sb->s_magic;
>  	buf->f_bsize   = sb->s_blocksize;
>  	buf->f_blocks  = le32_to_cpu(qnx4_sb(sb)->BitMap->di_size) * 8;
> @@ -168,8 +165,6 @@ static int qnx4_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	buf->f_fsid.val[0] = (u32)id;
>  	buf->f_fsid.val[1] = (u32)(id >> 32);
>  
> -	unlock_kernel();
> -
>  	return 0;
>  }
>  
> @@ -234,13 +229,9 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
>  	struct qnx4_sb_info *qs;
>  	int ret = -EINVAL;
>  
> -	lock_kernel();
> -
>  	qs = kzalloc(sizeof(struct qnx4_sb_info), GFP_KERNEL);
> -	if (!qs) {
> -		unlock_kernel();
> +	if (!qs)
>  		return -ENOMEM;
> -	}
>  	s->s_fs_info = qs;
>  
>  	sb_set_blocksize(s, QNX4_BLOCK_SIZE);
> @@ -287,8 +278,6 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
>   		goto outi;
>  
>  	brelse(bh);
> -
> -	unlock_kernel();
>  	return 0;
>  
>        outi:
> @@ -298,7 +287,6 @@ static int qnx4_fill_super(struct super_block *s, void *data, int silent)
>        outnobh:
>  	kfree(qs);
>  	s->s_fs_info = NULL;
> -	unlock_kernel();
>  	return ret;
>  }
>  
> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 58703eb..275327b 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -12,7 +12,6 @@
>   * 04-07-1998 by Frank Denis : first step for rmdir/unlink.
>   */
>  
> -#include <linux/smp_lock.h>
>  #include <linux/buffer_head.h>
>  #include "qnx4.h"
>  
> @@ -109,7 +108,6 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, struct nam
>  	int len = dentry->d_name.len;
>  	struct inode *foundinode = NULL;
>  
> -	lock_kernel();
>  	if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
>  		goto out;
>  	/* The entry is linked, let's get the real info */
> @@ -123,13 +121,11 @@ struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, struct nam
>  
>  	foundinode = qnx4_iget(dir->i_sb, ino);
>  	if (IS_ERR(foundinode)) {
> -		unlock_kernel();
>  		QNX4DEBUG((KERN_ERR "qnx4: lookup->iget -> error %ld\n",
>  			   PTR_ERR(foundinode)));
>  		return ERR_CAST(foundinode);
>  	}
>  out:
> -	unlock_kernel();
>  	d_add(dentry, foundinode);
>  
>  	return NULL;
> -- 
> 1.7.1

[PATCH 2/2] BKL: remove BKL from freevxfs

[Arnd Bergmann]

All uses of the BKL in freevxfs were the result of a pushdown into
code that doesn't really need it. As Christoph points out, this
is a read-only file system, which eliminates most of the races in
readdir/lookup.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Christoph Hellwig <hch@infradead.org>
---
 fs/freevxfs/vxfs_lookup.c |   14 ++------------
 fs/freevxfs/vxfs_super.c  |   10 ----------
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/fs/freevxfs/vxfs_lookup.c b/fs/freevxfs/vxfs_lookup.c
index 0ec7bb2..6c5131d 100644
--- a/fs/freevxfs/vxfs_lookup.c
+++ b/fs/freevxfs/vxfs_lookup.c
@@ -36,7 +36,6 @@
 #include <linux/highmem.h>
 #include <linux/kernel.h>
 #include <linux/pagemap.h>
-#include <linux/smp_lock.h>
 
 #include "vxfs.h"
 #include "vxfs_dir.h"
@@ -212,16 +211,12 @@ vxfs_lookup(struct inode *dip, struct dentry *dp, struct nameidata *nd)
 	if (dp->d_name.len > VXFS_NAMELEN)
 		return ERR_PTR(-ENAMETOOLONG);
 				 
-	lock_kernel();
 	ino = vxfs_inode_by_name(dip, dp);
 	if (ino) {
 		ip = vxfs_iget(dip->i_sb, ino);
-		if (IS_ERR(ip)) {
-			unlock_kernel();
+		if (IS_ERR(ip))
 			return ERR_CAST(ip);
-		}
 	}
-	unlock_kernel();
 	d_add(dp, ip);
 	return NULL;
 }
@@ -248,8 +243,6 @@ vxfs_readdir(struct file *fp, void *retp, filldir_t filler)
 	u_long			page, npages, block, pblocks, nblocks, offset;
 	loff_t			pos;
 
-	lock_kernel();
-
 	switch ((long)fp->f_pos) {
 	case 0:
 		if (filler(retp, ".", 1, fp->f_pos, ip->i_ino, DT_DIR) < 0)
@@ -265,10 +258,8 @@ vxfs_readdir(struct file *fp, void *retp, filldir_t filler)
 
 	pos = fp->f_pos - 2;
 	
-	if (pos > VXFS_DIRROUND(ip->i_size)) {
-		unlock_kernel();
+	if (pos > VXFS_DIRROUND(ip->i_size))
 		return 0;
-	}
 
 	npages = dir_pages(ip);
 	nblocks = dir_blocks(ip);
@@ -327,6 +318,5 @@ vxfs_readdir(struct file *fp, void *retp, filldir_t filler)
 done:
 	fp->f_pos = ((page << PAGE_CACHE_SHIFT) | offset) + 2;
 out:
-	unlock_kernel();
 	return 0;
 }
diff --git a/fs/freevxfs/vxfs_super.c b/fs/freevxfs/vxfs_super.c
index eb2b9e0..71b0148 100644
--- a/fs/freevxfs/vxfs_super.c
+++ b/fs/freevxfs/vxfs_super.c
@@ -38,7 +38,6 @@
 #include <linux/buffer_head.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/stat.h>
 #include <linux/vfs.h>
 #include <linux/mount.h>
@@ -81,16 +80,12 @@ vxfs_put_super(struct super_block *sbp)
 {
 	struct vxfs_sb_info	*infp = VXFS_SBI(sbp);
 
-	lock_kernel();
-
 	vxfs_put_fake_inode(infp->vsi_fship);
 	vxfs_put_fake_inode(infp->vsi_ilist);
 	vxfs_put_fake_inode(infp->vsi_stilist);
 
 	brelse(infp->vsi_bp);
 	kfree(infp);
-
-	unlock_kernel();
 }
 
 /**
@@ -159,14 +154,11 @@ static int vxfs_fill_super(struct super_block *sbp, void *dp, int silent)
 	struct inode *root;
 	int ret = -EINVAL;
 
-	lock_kernel();
-
 	sbp->s_flags |= MS_RDONLY;
 
 	infp = kzalloc(sizeof(*infp), GFP_KERNEL);
 	if (!infp) {
 		printk(KERN_WARNING "vxfs: unable to allocate incore superblock\n");
-		unlock_kernel();
 		return -ENOMEM;
 	}
 
@@ -239,7 +231,6 @@ static int vxfs_fill_super(struct super_block *sbp, void *dp, int silent)
 		goto out_free_ilist;
 	}
 
-	unlock_kernel();
 	return 0;
 	
 out_free_ilist:
@@ -249,7 +240,6 @@ out_free_ilist:
 out:
 	brelse(bp);
 	kfree(infp);
-	unlock_kernel();
 	return ret;
 }
 
-- 
1.7.1

Re: Remaining BKL users, what to do

[Petr Vandrovec]

I'll try to come up with something for ncpfs.

Trivial lock replacement will open deadlock possibility when someone reads to page which is also mmaped from the same filesystem (like grep likes to do). BKL with its automated release on sleep helped (or papered over) a lot here.

Petr

"Arnd Bergmann" <arnd@arndb.de> wrote:

>On Thursday 16 September 2010, Anton Altaparmakov wrote:
>> On 16 Sep 2010, at 16:04, Jan Kara wrote:
>> > On Thu 16-09-10 16:32:59, Arnd Bergmann wrote:
>> >> The big kernel lock is gone from almost all code in linux-next, this is
>> >> the status of what I think will happen to the remaining users:
>> > ...
>> >> fs/ncpfs:
>> >>      Should be fixable if Petr still cares about it. Otherwise suggest
>> >>      moving to drivers/staging if there are no users left.
>> >  I think some people still use this...
>> 
>> Yes, indeed.  Netware is still alive (unfortunately!) and ncpfs is used in a lot of 
>> Universities here in the UK at least (we use it about a thousand workstations and
>> servers here at Cambridge University!).
>
>Ok, that means at least when someone gets around to fix it, there will be
>people that can test the patches.
>
>If you know someone who would like to help on this, it would be nice to try
>out the patch below, unless someone can come up with a better solution.
>My naïve understanding of the code tells me that simply using the super block
>lock there may work. In fact it makes locking stricter, so if it still works
>with that patch, there are probably no subtle regressions.
>The patch applies to current linux-next of my bkl/vfs series.
>
>	Arnd
>
>---
>ncpfs: replace BKL with lock_super
>
>This mindlessly changes every instance of lock_kernel in ncpfs to
>lock_super. I haven't tested this, it may work or may break horribly.
>Please test with CONFIG_LOCKDEP enabled.
>
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
>index 9578cbe..303338d 100644
>--- a/fs/ncpfs/dir.c
>+++ b/fs/ncpfs/dir.c
>@@ -19,7 +19,6 @@
> #include <linux/mm.h>
> #include <asm/uaccess.h>
> #include <asm/byteorder.h>
>-#include <linux/smp_lock.h>
> 
> #include <linux/ncp_fs.h>
> 
>@@ -339,9 +338,10 @@ static int
> ncp_lookup_validate(struct dentry * dentry, struct nameidata *nd)
> {
> 	int res;
>-	lock_kernel();
>+	struct super_block *sb = dentry->d_inode->i_sb;
>+	lock_super(sb);
> 	res = __ncp_lookup_validate(dentry);
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return res;
> }
> 
>@@ -404,6 +404,7 @@ static int ncp_readdir(struct file *filp, void *dirent, filldir_t filldir)
> {
> 	struct dentry *dentry = filp->f_path.dentry;
> 	struct inode *inode = dentry->d_inode;
>+	struct super_block *sb = inode->i_sb;
> 	struct page *page = NULL;
> 	struct ncp_server *server = NCP_SERVER(inode);
> 	union  ncp_dir_cache *cache = NULL;
>@@ -411,7 +412,7 @@ static int ncp_readdir(struct file *filp, void *dirent, filldir_t filldir)
> 	int result, mtime_valid = 0;
> 	time_t mtime = 0;
> 
>-	lock_kernel();
>+	lock_super(sb);
> 
> 	ctl.page  = NULL;
> 	ctl.cache = NULL;
>@@ -546,7 +547,7 @@ finished:
> 		page_cache_release(ctl.page);
> 	}
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return result;
> }
> 
>@@ -794,12 +795,13 @@ out:
> static struct dentry *ncp_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
> {
> 	struct ncp_server *server = NCP_SERVER(dir);
>+	struct super_block *sb = dir->i_sb;
> 	struct inode *inode = NULL;
> 	struct ncp_entry_info finfo;
> 	int error, res, len;
> 	__u8 __name[NCP_MAXPATHLEN + 1];
> 
>-	lock_kernel();
>+	lock_super(sb);
> 	error = -EIO;
> 	if (!ncp_conn_valid(server))
> 		goto finished;
>@@ -846,7 +848,7 @@ add_entry:
> 
> finished:
> 	PPRINTK("ncp_lookup: result=%d\n", error);
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return ERR_PTR(error);
> }
> 
>@@ -880,6 +882,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
> {
> 	struct ncp_server *server = NCP_SERVER(dir);
> 	struct ncp_entry_info finfo;
>+	struct super_block *sb = dir->i_sb;
> 	int error, result, len;
> 	int opmode;
> 	__u8 __name[NCP_MAXPATHLEN + 1];
>@@ -888,7 +891,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
> 		dentry->d_parent->d_name.name, dentry->d_name.name, mode);
> 
> 	error = -EIO;
>-	lock_kernel();
>+	lock_super(sb);
> 	if (!ncp_conn_valid(server))
> 		goto out;
> 
>@@ -935,7 +938,7 @@ int ncp_create_new(struct inode *dir, struct dentry *dentry, int mode,
> 
> 	error = ncp_instantiate(dir, dentry, &finfo);
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
>@@ -949,6 +952,7 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> {
> 	struct ncp_entry_info finfo;
> 	struct ncp_server *server = NCP_SERVER(dir);
>+	struct super_block *sb = dir->i_sb;
> 	int error, len;
> 	__u8 __name[NCP_MAXPATHLEN + 1];
> 
>@@ -956,7 +960,7 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> 		dentry->d_parent->d_name.name, dentry->d_name.name);
> 
> 	error = -EIO;
>-	lock_kernel();
>+	lock_super(sb);
> 	if (!ncp_conn_valid(server))
> 		goto out;
> 
>@@ -985,13 +989,14 @@ static int ncp_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> 		error = ncp_instantiate(dir, dentry, &finfo);
> 	}
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
> static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
> {
> 	struct ncp_server *server = NCP_SERVER(dir);
>+	struct super_block *sb = dir->i_sb;
> 	int error, result, len;
> 	__u8 __name[NCP_MAXPATHLEN + 1];
> 
>@@ -999,7 +1004,7 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
> 		dentry->d_parent->d_name.name, dentry->d_name.name);
> 
> 	error = -EIO;
>-	lock_kernel();
>+	lock_super(sb);
> 	if (!ncp_conn_valid(server))
> 		goto out;
> 
>@@ -1040,17 +1045,18 @@ static int ncp_rmdir(struct inode *dir, struct dentry *dentry)
> 			break;
>        	}
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
> static int ncp_unlink(struct inode *dir, struct dentry *dentry)
> {
> 	struct inode *inode = dentry->d_inode;
>+	struct super_block *sb = dir->i_sb;
> 	struct ncp_server *server;
> 	int error;
> 
>-	lock_kernel();
>+	lock_super(sb);
> 	server = NCP_SERVER(dir);
> 	DPRINTK("ncp_unlink: unlinking %s/%s\n",
> 		dentry->d_parent->d_name.name, dentry->d_name.name);
>@@ -1102,7 +1108,7 @@ static int ncp_unlink(struct inode *dir, struct dentry *dentry)
> 	}
> 		
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
>@@ -1110,6 +1116,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
> 		      struct inode *new_dir, struct dentry *new_dentry)
> {
> 	struct ncp_server *server = NCP_SERVER(old_dir);
>+	struct super_block *sb = old_dir->i_sb;
> 	int error;
> 	int old_len, new_len;
> 	__u8 __old_name[NCP_MAXPATHLEN + 1], __new_name[NCP_MAXPATHLEN + 1];
>@@ -1119,7 +1126,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
> 		new_dentry->d_parent->d_name.name, new_dentry->d_name.name);
> 
> 	error = -EIO;
>-	lock_kernel();
>+	lock_super(sb);
> 	if (!ncp_conn_valid(server))
> 		goto out;
> 
>@@ -1165,7 +1172,7 @@ static int ncp_rename(struct inode *old_dir, struct dentry *old_dentry,
> 			break;
> 	}
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
>diff --git a/fs/ncpfs/file.c b/fs/ncpfs/file.c
>index 3639cc5..a871df0 100644
>--- a/fs/ncpfs/file.c
>+++ b/fs/ncpfs/file.c
>@@ -17,7 +17,6 @@
> #include <linux/mm.h>
> #include <linux/vmalloc.h>
> #include <linux/sched.h>
>-#include <linux/smp_lock.h>
> 
> #include <linux/ncp_fs.h>
> #include "ncplib_kernel.h"
>@@ -284,9 +283,11 @@ static int ncp_release(struct inode *inode, struct file *file) {
> static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
> {
> 	loff_t ret;
>-	lock_kernel();
>+	struct super_block *sb = file->f_path.dentry->d_inode->i_sb;
>+
>+	lock_super(sb);
> 	ret = generic_file_llseek_unlocked(file, offset, origin);
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return ret;
> }
> 
>diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
>index cdf0fce..f37d297 100644
>--- a/fs/ncpfs/inode.c
>+++ b/fs/ncpfs/inode.c
>@@ -26,7 +26,6 @@
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> #include <linux/init.h>
>-#include <linux/smp_lock.h>
> #include <linux/vfs.h>
> #include <linux/mount.h>
> #include <linux/seq_file.h>
>@@ -445,12 +444,12 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
> #endif
> 	struct ncp_entry_info finfo;
> 
>-	lock_kernel();
>+	lock_super(sb);
> 
> 	data.wdog_pid = NULL;
> 	server = kzalloc(sizeof(struct ncp_server), GFP_KERNEL);
> 	if (!server) {
>-		unlock_kernel();
>+		unlock_super(sb);
> 		return -ENOMEM;
> 	}
> 	sb->s_fs_info = server;
>@@ -704,7 +703,7 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
>         if (!sb->s_root)
> 		goto out_no_root;
> 	sb->s_root->d_op = &ncp_root_dentry_operations;
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return 0;
> 
> out_no_root:
>@@ -741,7 +740,7 @@ out:
> 	put_pid(data.wdog_pid);
> 	sb->s_fs_info = NULL;
> 	kfree(server);
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return error;
> }
> 
>@@ -749,7 +748,7 @@ static void ncp_put_super(struct super_block *sb)
> {
> 	struct ncp_server *server = NCP_SBP(sb);
> 
>-	lock_kernel();
>+	lock_super(sb);
> 
> 	ncp_lock_server(server);
> 	ncp_disconnect(server);
>@@ -778,7 +777,7 @@ static void ncp_put_super(struct super_block *sb)
> 	sb->s_fs_info = NULL;
> 	kfree(server);
> 
>-	unlock_kernel();
>+	unlock_super(sb);
> }
> 
> static int ncp_statfs(struct dentry *dentry, struct kstatfs *buf)
>@@ -850,6 +849,7 @@ dflt:;
> int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
> {
> 	struct inode *inode = dentry->d_inode;
>+	struct super_block *sb = inode->i_sb;
> 	int result = 0;
> 	__le32 info_mask;
> 	struct nw_modify_dos_info info;
>@@ -857,7 +857,7 @@ int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
> 
> 	result = -EIO;
> 
>-	lock_kernel();	
>+	lock_super(sb);	
> 
> 	server = NCP_SERVER(inode);
> 	if ((!server) || !ncp_conn_valid(server))
>@@ -1011,7 +1011,7 @@ int ncp_notify_change(struct dentry *dentry, struct iattr *attr)
> 	mark_inode_dirty(inode);
> 
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return result;
> }
> 
>diff --git a/fs/ncpfs/ioctl.c b/fs/ncpfs/ioctl.c
>index 84a8cfc..4ce88d4 100644
>--- a/fs/ncpfs/ioctl.c
>+++ b/fs/ncpfs/ioctl.c
>@@ -17,7 +17,6 @@
> #include <linux/mount.h>
> #include <linux/slab.h>
> #include <linux/highuid.h>
>-#include <linux/smp_lock.h>
> #include <linux/vmalloc.h>
> #include <linux/sched.h>
> 
>@@ -844,8 +843,9 @@ static int ncp_ioctl_need_write(unsigned int cmd)
> long ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> 	long ret;
>+	struct super_block *sb = filp->f_path.dentry->d_inode->i_sb;
> 
>-	lock_kernel();
>+	lock_super(sb);
> 	if (ncp_ioctl_need_write(cmd)) {
> 		/*
> 		 * inside the ioctl(), any failures which
>@@ -863,19 +863,20 @@ long ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		mnt_drop_write(filp->f_path.mnt);
> 
> out:
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return ret;
> }
> 
> #ifdef CONFIG_COMPAT
> long ncp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
>+	struct super_block *sb = file->f_path.dentry->d_inode->i_sb;
> 	long ret;
> 
>-	lock_kernel();
>+	lock_super(sb);
> 	arg = (unsigned long) compat_ptr(arg);
> 	ret = ncp_ioctl(file, cmd, arg);
>-	unlock_kernel();
>+	unlock_super(sb);
> 	return ret;
> }
> #endif

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: Remaining BKL users, what to do

[Arnd Bergmann]

On Saturday 18 September 2010 01:21:41 Petr Vandrovec wrote:
> 
> I'll try to come up with something for ncpfs.

Ok, good.

> Trivial lock replacement will open deadlock possibility when
> someone reads to page which is also mmaped from the same 
> filesystem (like grep likes to do). BKL with its automated
> release on sleep helped (or papered over) a lot here.

Right, I was more or less expecting something like this.
So I guess this is some AB-BA deadlock with another mutex
or a call to flush_scheduled_work that is currently done
under the BKL?

There is still the possibility of just working around those
by adding explicit mutex_unlock() calls around those, which
is what I initially did in the tty subsystem. The better
long-term approach would obviously be to understand all of
the data structures that actually need locking and only
lock the actual accesses, but that may be more work than
you are willing to spend on it.

	Arnd