* Re: Remaining BKL users, what to do
@ 2010-09-17 23:21 Petr Vandrovec
2010-09-21 20:01 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Petr Vandrovec @ 2010-09-17 23:21 UTC (permalink / raw)
To: Arnd Bergmann, Anton Altaparmakov
Cc: Jan Kara, codalist, autofs, linux-media, dri-devel,
Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Anders Larsen, Evgeniy Dushistov, Ingo Molnar, netdev,
Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-17 23:21 Remaining BKL users, what to do Petr Vandrovec
@ 2010-09-21 20:01 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-21 20:01 UTC (permalink / raw)
To: Petr Vandrovec
Cc: Anton Altaparmakov, Jan Kara, codalist, autofs, linux-media,
dri-devel, Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Anders Larsen, Evgeniy Dushistov, Ingo Molnar, netdev,
Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Remaining BKL users, what to do
@ 2010-09-16 14:32 Arnd Bergmann
2010-09-16 14:49 ` Steven Rostedt
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-16 14:32 UTC (permalink / raw)
To: codalist
Cc: autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Anders Larsen,
Jan Kara, Evgeniy Dushistov, Ingo Molnar, netdev, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
@ 2010-09-16 14:49 ` Steven Rostedt
2010-09-16 18:32 ` Jens Axboe
2010-09-16 15:04 ` Jan Kara
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Steven Rostedt @ 2010-09-16 14:49 UTC (permalink / raw)
To: Arnd Bergmann, Jens Axboe
Cc: codalist, autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Anders Larsen,
Jan Kara, Evgeniy Dushistov, Ingo Molnar, netdev, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 14:49 ` Steven Rostedt
@ 2010-09-16 18:32 ` Jens Axboe
2010-09-17 18:46 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Jens Axboe @ 2010-09-16 18:32 UTC (permalink / raw)
To: Steven Rostedt
Cc: Arnd Bergmann, codalist@coda.cs.cmu.edu, autofs@linux.kernel.org,
linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Jan Kara, Evgeniy Dushistov,
Ingo Molnar, netdev@vger.kernel.org, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 18:32 ` Jens Axboe
@ 2010-09-17 18:46 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-17 18:46 UTC (permalink / raw)
To: Jens Axboe
Cc: Steven Rostedt, codalist@coda.cs.cmu.edu, autofs@linux.kernel.org,
linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Jan Kara, Evgeniy Dushistov,
Ingo Molnar, netdev@vger.kernel.org, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Hendry
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;
}
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
2010-09-16 14:49 ` Steven Rostedt
@ 2010-09-16 15:04 ` Jan Kara
2010-09-16 21:26 ` Anton Altaparmakov
2010-09-16 15:07 ` Alan Cox
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Jan Kara @ 2010-09-16 15:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: codalist, autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Anders Larsen,
Jan Kara, Evgeniy Dushistov, Ingo Molnar, netdev, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 15:04 ` Jan Kara
@ 2010-09-16 21:26 ` Anton Altaparmakov
2010-09-17 10:45 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Anton Altaparmakov @ 2010-09-16 21:26 UTC (permalink / raw)
To: Jan Kara
Cc: Arnd Bergmann, codalist, autofs, linux-media, dri-devel,
Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Evgeniy Dushistov, Ingo Molnar,
netdev, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 21:26 ` Anton Altaparmakov
@ 2010-09-17 10:45 ` Arnd Bergmann
2010-09-17 13:32 ` Christoph Hellwig
0 siblings, 1 reply; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-17 10:45 UTC (permalink / raw)
To: Anton Altaparmakov
Cc: Jan Kara, codalist, autofs, linux-media, dri-devel,
Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Evgeniy Dushistov, Ingo Molnar,
netdev, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-17 10:45 ` Arnd Bergmann
@ 2010-09-17 13:32 ` Christoph Hellwig
2010-09-17 13:50 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2010-09-17 13:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Anton Altaparmakov, Jan Kara, codalist, autofs, linux-media,
dri-devel, Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Evgeniy Dushistov, Ingo Molnar,
netdev, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-17 13:32 ` Christoph Hellwig
@ 2010-09-17 13:50 ` Arnd Bergmann
2010-09-17 14:02 ` Christoph Hellwig
0 siblings, 1 reply; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-17 13:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Altaparmakov, Jan Kara, codalist, autofs, linux-media,
dri-devel, Mikulas Patocka, Trond Myklebust, Petr Vandrovec,
Anders Larsen, Evgeniy Dushistov, Ingo Molnar, netdev,
Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-17 13:50 ` Arnd Bergmann
@ 2010-09-17 14:02 ` Christoph Hellwig
2010-09-17 14:56 ` Arnd Bergmann
0 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2010-09-17 14:02 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, Anton Altaparmakov, Jan Kara, codalist, autofs,
linux-media, dri-devel, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Evgeniy Dushistov, Ingo Molnar,
netdev, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-17 14:02 ` Christoph Hellwig
@ 2010-09-17 14:56 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-17 14:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Altaparmakov, Jan Kara, codalist, autofs, linux-media,
dri-devel, Mikulas Patocka, Trond Myklebust, Petr Vandrovec,
Anders Larsen, Evgeniy Dushistov, Ingo Molnar, netdev,
Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
2010-09-16 14:49 ` Steven Rostedt
2010-09-16 15:04 ` Jan Kara
@ 2010-09-16 15:07 ` Alan Cox
2010-09-16 20:08 ` David Miller
2010-09-16 16:09 ` Anders Larsen
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Alan Cox @ 2010-09-16 15:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: codalist, autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Anders Larsen,
Jan Kara, Evgeniy Dushistov, Ingo Molnar, netdev, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 15:07 ` Alan Cox
@ 2010-09-16 20:08 ` David Miller
0 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2010-09-16 20:08 UTC (permalink / raw)
To: alan
Cc: arnd, codalist, autofs, linux-media, dri-devel, hch, mikulas,
Trond.Myklebust, vandrove, al, jack, dushistov, mingo, netdev,
samuel, acme, linux-kernel, linux-fsdevel, andrew.hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
` (2 preceding siblings ...)
2010-09-16 15:07 ` Alan Cox
@ 2010-09-16 16:09 ` Anders Larsen
2010-09-16 16:57 ` Samuel Ortiz
` (2 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Anders Larsen @ 2010-09-16 16:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: codalist, autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Jan Kara,
Evgeniy Dushistov, Ingo Molnar, netdev, Samuel Ortiz,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
` (3 preceding siblings ...)
2010-09-16 16:09 ` Anders Larsen
@ 2010-09-16 16:57 ` Samuel Ortiz
2010-09-16 20:08 ` David Miller
2010-09-16 19:00 ` Jan Harkes
2010-10-21 12:47 ` Christoph Hellwig
6 siblings, 1 reply; 22+ messages in thread
From: Samuel Ortiz @ 2010-09-16 16:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: codalist, autofs, linux-media, dri-devel, Christoph Hellwig,
Mikulas Patocka, Trond Myklebust, Petr Vandrovec, Anders Larsen,
Jan Kara, Evgeniy Dushistov, Ingo Molnar, netdev,
Arnaldo Carvalho de Melo, linux-kernel, linux-fsdevel,
Andrew Hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 16:57 ` Samuel Ortiz
@ 2010-09-16 20:08 ` David Miller
0 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2010-09-16 20:08 UTC (permalink / raw)
To: samuel
Cc: arnd, codalist, autofs, linux-media, dri-devel, hch, mikulas,
Trond.Myklebust, vandrove, al, jack, dushistov, mingo, netdev,
acme, linux-kernel, linux-fsdevel, andrew.hendry
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
` (4 preceding siblings ...)
2010-09-16 16:57 ` Samuel Ortiz
@ 2010-09-16 19:00 ` Jan Harkes
2010-09-16 19:26 ` Arnd Bergmann
2010-10-21 12:47 ` Christoph Hellwig
6 siblings, 1 reply; 22+ messages in thread
From: Jan Harkes @ 2010-09-16 19:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Jan Kara, Evgeniy Dushistov,
Ingo Molnar, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-09-16 19:00 ` Jan Harkes
@ 2010-09-16 19:26 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-09-16 19:26 UTC (permalink / raw)
To: Jan Harkes
Cc: Christoph Hellwig, Mikulas Patocka, Trond Myklebust,
Petr Vandrovec, Anders Larsen, Jan Kara, Evgeniy Dushistov,
Ingo Molnar, Samuel Ortiz, Arnaldo Carvalho de Melo, linux-kernel,
linux-fsdevel, Andrew Hendry
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Remaining BKL users, what to do
2010-09-16 14:32 Arnd Bergmann
` (5 preceding siblings ...)
2010-09-16 19:00 ` Jan Harkes
@ 2010-10-21 12:47 ` Christoph Hellwig
2010-10-21 13:38 ` Arnd Bergmann
6 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2010-10-21 12:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, linux-fsdevel, Mikulas Patocka, Evgeniy Dushistov
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: Remaining BKL users, what to do
2010-10-21 12:47 ` Christoph Hellwig
@ 2010-10-21 13:38 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2010-10-21 13:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, linux-fsdevel, Mikulas Patocka, Evgeniy Dushistov,
Jan Kara
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
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2010-10-21 13:37 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-17 23:21 Remaining BKL users, what to do Petr Vandrovec
2010-09-21 20:01 ` Arnd Bergmann
-- strict thread matches above, loose matches on Subject: below --
2010-09-16 14:32 Arnd Bergmann
2010-09-16 14:49 ` Steven Rostedt
2010-09-16 18:32 ` Jens Axboe
2010-09-17 18:46 ` Arnd Bergmann
2010-09-16 15:04 ` Jan Kara
2010-09-16 21:26 ` Anton Altaparmakov
2010-09-17 10:45 ` Arnd Bergmann
2010-09-17 13:32 ` Christoph Hellwig
2010-09-17 13:50 ` Arnd Bergmann
2010-09-17 14:02 ` Christoph Hellwig
2010-09-17 14:56 ` Arnd Bergmann
2010-09-16 15:07 ` Alan Cox
2010-09-16 20:08 ` David Miller
2010-09-16 16:09 ` Anders Larsen
2010-09-16 16:57 ` Samuel Ortiz
2010-09-16 20:08 ` David Miller
2010-09-16 19:00 ` Jan Harkes
2010-09-16 19:26 ` Arnd Bergmann
2010-10-21 12:47 ` Christoph Hellwig
2010-10-21 13:38 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).