Re: Remaining BKL users, what to do
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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" 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
>
>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
> #include
> #include
>-#include
>
> #include
>
>@@ -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_SER
Re: Remaining BKL users, what to do
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
Cc: Jens Axboe
Cc: Steven Rostedt
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
#include
#include
-#include
#include
#include
@@ -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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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
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
#include
#include
-#include
#include
@@ -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))
Re: Remaining BKL users, what to do
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 (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer, http://www.linux-ntfs.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
From: Alan Cox 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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
From: Samuel Ortiz 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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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 SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Remaining BKL users, what to do
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
