During the realtime preemption mini-summit we discussed the entire removal of
the big kernel lock. I've started working on this for some filesystems. My plan
is to push the BKL down to the implementations first and remove it from there
later.
This series is pushing the BKL from do_new_mount() down to the filesystems and
removes it from ext series of filesystems and one other trivial use: if the BKL
is only used in get_sb/fill_super due to the push-down, we just need to make
sure that parallel calls to get_sb/fill_super would race against each other.
I addressed the feedback from Matthew Wilcox and tried to be more verbose about
why I think it is safe to remove the BKL. In most cases there is no shared
resource accessed anyway so it is trivially safe to remove the lock.
There are two filesystems where I'm not 100% sure if the proposed patch is
enough, namely btrfs and xfs. Chris(+toph), could you help me here?
I left out the patch to remove default_llseek() on purpose. This indeed needs
some more lovin'.
Comments?
Jan
Jan Blunck (27):
BKL: Push down BKL from do_new_mount() to the filesystems
get_sb/fill_super operation
BKL: Remove outdated comment and include
BKL: Remove BKL from simple_fill_super
ext2: Add ext2_sb_info mutex
BKL: Remove BKL from ext2 filesystem
BKL: Remove BKL from ext3 fill_super()
BKL: Remove BKL from ext3_put_super() and ext3_remount()
BKL: Remove BKL from ext4 filesystem
BKL: Remove BKL from 9p
BKL: Remove BKL from autofs4
BKL: Remove BKL from befs
BKL: Remove BKL from btrfs
BKL: Remove BKL from configfs
BKL: Remove BKL from cramfs
BKL: Remove BKL from devpts
BKL: Remove BKL from efs
BKL: Remove BKL from exofs
BKL: Remove BKL from hostfs
BKL: Remove BKL from hugetlbfs
BKL: Remove BKL from minix
BKL: Remove BKL from omfs
BKL: Remove BKL from openpromfs
BKL: Remove BKL from ramfs
BKL: Remove BKL from romfs
BKL: Remove BKL from sysfs
BKL: Remove BKL from ubifs
BKL: Remove BKL from xfs
fs/adfs/super.c | 8 ++++++-
fs/affs/super.c | 9 +++++++-
fs/afs/super.c | 5 ++++
fs/bfs/inode.c | 9 +++++++-
fs/binfmt_misc.c | 6 ++++-
fs/cifs/cifsfs.c | 12 +++++++++-
fs/coda/inode.c | 8 ++++++-
fs/ecryptfs/main.c | 3 ++
fs/exofs/super.c | 2 +-
fs/ext2/inode.c | 5 +--
fs/ext2/super.c | 48 ++++++++++++++++++++++++++++---------------
fs/ext3/super.c | 12 -----------
fs/ext4/super.c | 11 ----------
fs/fat/namei_msdos.c | 6 ++++-
fs/fat/namei_vfat.c | 6 ++++-
fs/freevxfs/vxfs_super.c | 7 +++++-
fs/fuse/control.c | 9 +++++++-
fs/fuse/inode.c | 5 ++++
fs/gfs2/ops_fstype.c | 9 ++++++++
fs/hfs/super.c | 8 ++++++-
fs/hfsplus/super.c | 8 ++++++-
fs/hpfs/super.c | 8 ++++++-
fs/hppfs/hppfs.c | 4 +++
fs/isofs/inode.c | 8 ++++++-
fs/jffs2/super.c | 11 ++++++++-
fs/jfs/super.c | 14 +++++++++++-
fs/libfs.c | 1 +
fs/namespace.c | 2 -
fs/ncpfs/inode.c | 8 ++++++-
fs/nfs/super.c | 19 +++++++++++++++++
fs/nfsd/nfsctl.c | 7 +++++-
fs/nilfs2/super.c | 9 +++++++-
fs/ntfs/super.c | 5 ++++
fs/ocfs2/dlm/dlmfs.c | 8 ++++++-
fs/ocfs2/super.c | 5 ++++
fs/proc/root.c | 9 +++++++-
fs/qnx4/inode.c | 8 ++++++-
fs/reiserfs/super.c | 4 +++
fs/smbfs/inode.c | 5 ++++
fs/squashfs/super.c | 6 +++++
fs/super.c | 3 --
fs/sysfs/mount.c | 2 +-
fs/sysv/super.c | 24 +++++++++++++++++----
fs/udf/super.c | 8 ++++++-
fs/ufs/super.c | 5 ++++
include/linux/ext2_fs_sb.h | 2 +
46 files changed, 300 insertions(+), 81 deletions(-)
remount_fs does not need lock_kernel() anymore since that was pushed down some
time ago. Since nothing needs smp_lock.h anymore lets remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/super.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 19eb70b..6fe4611 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -23,7 +23,6 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/init.h>
-#include <linux/smp_lock.h>
#include <linux/acct.h>
#include <linux/blkdev.h>
#include <linux/quotaops.h>
@@ -618,8 +617,6 @@ static void do_emergency_remount(struct work_struct *work)
down_write(&sb->s_umount);
if (sb->s_root && sb->s_bdev && !(sb->s_flags & MS_RDONLY)) {
/*
- * ->remount_fs needs lock_kernel().
- *
* What lock protects sb->s_flags??
*/
do_remount_sb(sb, MS_RDONLY, NULL, 1);
--
1.6.4.2
simple_fill_super is used in following filesystems that don't use the BKL
otherwise:
- security/selinux/selinuxfs.c
- security/inode.c
- security/smack/smackfs.c
- drivers/infiniband/hw/ipath/ipath_fs.c
- drivers/xen/xenfs/super.c
- fs/debugfs/inode.c
It is used in following filesystems that make use of the BKL:
- fs/fuse/control.c
- fs/binfmt_misc.c
- fs/nfsd/nfsctl.c
All three filesystems protect the call to simple_fill_super() by the BKL
themself. Therefore it is safe to remove the BKL from simpe_fill_super().
Signed-off-by: Jan Blunck <[email protected]>
---
fs/libfs.c | 11 ++---------
1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index 3484040..4a70729 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -11,7 +11,6 @@
#include <linux/exportfs.h>
#include <linux/writeback.h>
#include <linux/buffer_head.h>
-#include <linux/smp_lock.h> /* Only for lock_kernel() */
#include <asm/uaccess.h>
@@ -423,8 +422,6 @@ int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files
struct dentry *dentry;
int i;
- lock_kernel();
-
s->s_blocksize = PAGE_CACHE_SIZE;
s->s_blocksize_bits = PAGE_CACHE_SHIFT;
s->s_magic = magic;
@@ -432,10 +429,9 @@ int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files
s->s_time_gran = 1;
inode = new_inode(s);
- if (!inode) {
- unlock_kernel();
+ if (!inode)
return -ENOMEM;
- }
+
/*
* because the root inode is 1, the files array must not contain an
* entry at index 1
@@ -449,7 +445,6 @@ int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files
root = d_alloc_root(inode);
if (!root) {
iput(inode);
- unlock_kernel();
return -ENOMEM;
}
for (i = 0; !files->name || files->name[0]; i++, files++) {
@@ -475,12 +470,10 @@ int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files
d_add(dentry, inode);
}
s->s_root = root;
- unlock_kernel();
return 0;
out:
d_genocide(root);
dput(root);
- unlock_kernel();
return -ENOMEM;
}
--
1.6.4.2
Add a mutex that protects the ext2_sb_info from concurrent modifications.
This is a preparation for removing the BKL later.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext2/inode.c | 2 ++
fs/ext2/super.c | 30 +++++++++++++++++++++++++++---
include/linux/ext2_fs_sb.h | 2 ++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index ade6340..057074e 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1401,9 +1401,11 @@ int ext2_write_inode(struct inode *inode, int do_sync)
* created, add a flag to the superblock.
*/
lock_kernel();
+ mutex_lock(&EXT2_SB(sb)->s_mutex);
ext2_update_dynamic_rev(sb);
EXT2_SET_RO_COMPAT_FEATURE(sb,
EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
+ mutex_unlock(&EXT2_SB(sb)->s_mutex);
unlock_kernel();
ext2_write_super(sb);
}
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 5af1775..d610eb9 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -84,6 +84,9 @@ void ext2_warning (struct super_block * sb, const char * function,
va_end(args);
}
+/*
+ * This must be called with sbi->s_mutex held.
+ */
void ext2_update_dynamic_rev(struct super_block *sb)
{
struct ext2_super_block *es = EXT2_SB(sb)->s_es;
@@ -117,8 +120,8 @@ static void ext2_put_super (struct super_block * sb)
lock_kernel();
- if (sb->s_dirt)
- ext2_write_super(sb);
+ if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
+ ext2_sync_fs(sb, 1);
ext2_xattr_put_super(sb);
if (!(sb->s_flags & MS_RDONLY)) {
@@ -207,6 +210,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
struct ext2_super_block *es = sbi->s_es;
unsigned long def_mount_opts;
+ mutex_lock(&sbi->s_mutex);
def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
if (sbi->s_sb_block != 1)
@@ -279,6 +283,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
if (!test_opt(sb, RESERVATION))
seq_puts(seq, ",noreservation");
+ mutex_unlock(&sbi->s_mutex);
return 0;
}
@@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_sb_block = sb_block;
/*
+ * mutex for protection of modifications of the superblock while being
+ * write out by ext2_write_super() or ext2_sync_fs().
+ */
+ mutex_init(&sbi->s_mutex);
+
+ /*
* See what the current blocksize for the device is, and
* use that as the blocksize. Otherwise (or if the blocksize
* is smaller than the default) use the default.
@@ -1122,8 +1133,9 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
* flags to 0. We need to set this flag to 0 since the fs
* may have been checked while mounted and e2fsck may have
* set s_state to EXT2_VALID_FS after some corrections.
+ *
+ * This must be called with sbi->s_mutex held.
*/
-
static int ext2_sync_fs(struct super_block *sb, int wait)
{
struct ext2_super_block *es = EXT2_SB(sb)->s_es;
@@ -1150,10 +1162,14 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
void ext2_write_super(struct super_block *sb)
{
+ struct ext2_sb_info * sbi = EXT2_SB(sb);
+
+ mutex_lock(&sbi->s_mutex);
if (!(sb->s_flags & MS_RDONLY))
ext2_sync_fs(sb, 1);
else
sb->s_dirt = 0;
+ mutex_unlock(&sbi->s_mutex);
}
static int ext2_remount (struct super_block * sb, int * flags, char * data)
@@ -1166,6 +1182,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
int err;
lock_kernel();
+ mutex_lock(&sbi->s_mutex);
/* Store the old options */
old_sb_flags = sb->s_flags;
@@ -1203,12 +1220,14 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
sbi->s_mount_opt |= old_mount_opt & EXT2_MOUNT_XIP;
}
if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY)) {
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return 0;
}
if (*flags & MS_RDONLY) {
if (le16_to_cpu(es->s_state) & EXT2_VALID_FS ||
!(sbi->s_mount_state & EXT2_VALID_FS)) {
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return 0;
}
@@ -1238,6 +1257,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
sb->s_flags &= ~MS_RDONLY;
}
ext2_sync_super(sb, es);
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return 0;
restore_opts:
@@ -1245,6 +1265,7 @@ restore_opts:
sbi->s_resuid = old_opts.s_resuid;
sbi->s_resgid = old_opts.s_resgid;
sb->s_flags = old_sb_flags;
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return err;
}
@@ -1256,6 +1277,8 @@ static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)
struct ext2_super_block *es = sbi->s_es;
u64 fsid;
+ mutex_lock(&sbi->s_mutex);
+
if (test_opt (sb, MINIX_DF))
sbi->s_overhead_last = 0;
else if (sbi->s_blocks_last != le32_to_cpu(es->s_blocks_count)) {
@@ -1310,6 +1333,7 @@ static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)
le64_to_cpup((void *)es->s_uuid + sizeof(u64));
buf->f_fsid.val[0] = fsid & 0xFFFFFFFFUL;
buf->f_fsid.val[1] = (fsid >> 32) & 0xFFFFFFFFUL;
+ mutex_unlock(&sbi->s_mutex);
return 0;
}
diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
index 1cdb663..8518361 100644
--- a/include/linux/ext2_fs_sb.h
+++ b/include/linux/ext2_fs_sb.h
@@ -106,6 +106,8 @@ struct ext2_sb_info {
spinlock_t s_rsv_window_lock;
struct rb_root s_rsv_window_root;
struct ext2_reserve_window_node s_rsv_window_head;
+ /* protect against concurrent modifications of this structure */
+ struct mutex s_mutex;
};
static inline spinlock_t *
--
1.6.4.2
The BKL is still used in ext2_put_super(), ext2_fill_super(), ext2_sync_fs()
ext2_remount() and ext2_write_inode(). From these calls ext2_put_super(),
ext2_fill_super() and ext2_remount() are protected against each other by
the struct super_block s_umount rw semaphore. The call in ext2_write_inode()
could only protect the modification of the ext2_sb_info through
ext2_update_dynamic_rev() against concurrent ext2_sync_fs() or ext2_remount().
ext2_fill_super() and ext2_put_super() can be left out because you need a
valid filesystem reference in all three cases, which you do not have when
you are one of these functions.
If the BKL is only protecting the modification of the ext2_sb_info it can
safely be removed since this is protected by the struct ext2_sb_info s_mutex.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext2/inode.c | 3 ---
fs/ext2/super.c | 16 ----------------
2 files changed, 0 insertions(+), 19 deletions(-)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 057074e..8b51416 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -22,7 +22,6 @@
* Assorted race fixes, rewrite of ext2_get_block() by Al Viro, 2000
*/
-#include <linux/smp_lock.h>
#include <linux/time.h>
#include <linux/highuid.h>
#include <linux/pagemap.h>
@@ -1400,13 +1399,11 @@ int ext2_write_inode(struct inode *inode, int do_sync)
/* If this is the first large file
* created, add a flag to the superblock.
*/
- lock_kernel();
mutex_lock(&EXT2_SB(sb)->s_mutex);
ext2_update_dynamic_rev(sb);
EXT2_SET_RO_COMPAT_FEATURE(sb,
EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
mutex_unlock(&EXT2_SB(sb)->s_mutex);
- unlock_kernel();
ext2_write_super(sb);
}
}
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index d610eb9..7e819ac 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -26,7 +26,6 @@
#include <linux/random.h>
#include <linux/buffer_head.h>
#include <linux/exportfs.h>
-#include <linux/smp_lock.h>
#include <linux/vfs.h>
#include <linux/seq_file.h>
#include <linux/mount.h>
@@ -118,8 +117,6 @@ static void ext2_put_super (struct super_block * sb)
int i;
struct ext2_sb_info *sbi = EXT2_SB(sb);
- lock_kernel();
-
if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
ext2_sync_fs(sb, 1);
@@ -143,8 +140,6 @@ static void ext2_put_super (struct super_block * sb)
sb->s_fs_info = NULL;
kfree(sbi->s_blockgroup_lock);
kfree(sbi);
-
- unlock_kernel();
}
static struct kmem_cache * ext2_inode_cachep;
@@ -750,8 +745,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
__le32 features;
int err;
- lock_kernel();
-
err = -ENOMEM;
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
@@ -1077,7 +1070,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
ext2_warning(sb, __func__,
"mounting ext3 filesystem as ext2");
ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
- unlock_kernel();
return 0;
cantfind_ext2:
@@ -1102,7 +1094,6 @@ failed_sbi:
kfree(sbi->s_blockgroup_lock);
kfree(sbi);
failed_unlock:
- unlock_kernel();
return ret;
}
@@ -1140,7 +1131,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
{
struct ext2_super_block *es = EXT2_SB(sb)->s_es;
- lock_kernel();
if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
ext2_debug("setting valid to 0\n");
es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
@@ -1154,7 +1144,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
ext2_commit_super(sb, es);
}
sb->s_dirt = 0;
- unlock_kernel();
return 0;
}
@@ -1181,7 +1170,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
unsigned long old_sb_flags;
int err;
- lock_kernel();
mutex_lock(&sbi->s_mutex);
/* Store the old options */
@@ -1221,14 +1209,12 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
}
if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY)) {
mutex_unlock(&sbi->s_mutex);
- unlock_kernel();
return 0;
}
if (*flags & MS_RDONLY) {
if (le16_to_cpu(es->s_state) & EXT2_VALID_FS ||
!(sbi->s_mount_state & EXT2_VALID_FS)) {
mutex_unlock(&sbi->s_mutex);
- unlock_kernel();
return 0;
}
/*
@@ -1258,7 +1244,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
}
ext2_sync_super(sb, es);
mutex_unlock(&sbi->s_mutex);
- unlock_kernel();
return 0;
restore_opts:
sbi->s_mount_opt = old_opts.s_mount_opt;
@@ -1266,7 +1251,6 @@ restore_opts:
sbi->s_resgid = old_opts.s_resgid;
sb->s_flags = old_sb_flags;
mutex_unlock(&sbi->s_mutex);
- unlock_kernel();
return err;
}
--
1.6.4.2
The BKL is protecting nothing than two memory allocations here.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext3/super.c | 13 +------------
1 files changed, 1 insertions(+), 12 deletions(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 38261a5..4b635b7 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -1568,19 +1568,14 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
__le32 features;
int err;
- lock_kernel();
-
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
sbi->s_blockgroup_lock =
kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
if (!sbi->s_blockgroup_lock) {
kfree(sbi);
- unlock_kernel();
return -ENOMEM;
}
sb->s_fs_info = sbi;
@@ -1589,8 +1584,6 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
sbi->s_resgid = EXT3_DEF_RESGID;
sbi->s_sb_block = sb_block;
- unlock_kernel();
-
blocksize = sb_min_blocksize(sb, EXT3_MIN_BLOCK_SIZE);
if (!blocksize) {
printk(KERN_ERR "EXT3-fs: unable to set blocksize\n");
@@ -1996,8 +1989,6 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
test_opt(sb,DATA_FLAGS) == EXT3_MOUNT_ORDERED_DATA ? "ordered":
"writeback");
- lock_kernel();
- unlock_kernel();
return 0;
cantfind_ext3:
@@ -2027,8 +2018,6 @@ out_fail:
sb->s_fs_info = NULL;
kfree(sbi->s_blockgroup_lock);
kfree(sbi);
- lock_kernel();
- unlock_kernel();
return ret;
}
--
1.6.4.2
The BKL lock is protecting the remounting against a potential call to
ext3_put_super(). This could not happen, since this is protected by the
s_umount rw semaphore of struct super_block.
Therefore I think the BKL is protecting nothing here.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext3/super.c | 8 --------
1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 4b635b7..3ddce03 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -398,8 +398,6 @@ static void ext3_put_super (struct super_block * sb)
struct ext3_super_block *es = sbi->s_es;
int i, err;
- lock_kernel();
-
ext3_xattr_put_super(sb);
err = journal_destroy(sbi->s_journal);
sbi->s_journal = NULL;
@@ -448,8 +446,6 @@ static void ext3_put_super (struct super_block * sb)
sb->s_fs_info = NULL;
kfree(sbi->s_blockgroup_lock);
kfree(sbi);
-
- unlock_kernel();
}
static struct kmem_cache *ext3_inode_cachep;
@@ -2495,8 +2491,6 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
int i;
#endif
- lock_kernel();
-
/* Store the original options */
lock_super(sb);
old_sb_flags = sb->s_flags;
@@ -2607,7 +2601,6 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
kfree(old_opts.s_qf_names[i]);
#endif
unlock_super(sb);
- unlock_kernel();
return 0;
restore_opts:
sb->s_flags = old_sb_flags;
@@ -2625,7 +2618,6 @@ restore_opts:
}
#endif
unlock_super(sb);
- unlock_kernel();
return err;
}
--
1.6.4.2
The BKL is still used in ext4_put_super(), ext4_fill_super() and
ext4_remount(). All three calles are protected against concurrent calls by
the s_umount rw semaphore of struct super_block.
Therefore the BKL is protecting nothing in this case.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext4/super.c | 16 +---------------
1 files changed, 1 insertions(+), 15 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9db81d2..328e9dc 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -26,7 +26,6 @@
#include <linux/init.h>
#include <linux/blkdev.h>
#include <linux/parser.h>
-#include <linux/smp_lock.h>
#include <linux/buffer_head.h>
#include <linux/exportfs.h>
#include <linux/vfs.h>
@@ -599,7 +598,6 @@ static void ext4_put_super(struct super_block *sb)
destroy_workqueue(sbi->dio_unwritten_wq);
lock_super(sb);
- lock_kernel();
if (sb->s_dirt)
ext4_commit_super(sb, 1);
@@ -665,7 +663,6 @@ static void ext4_put_super(struct super_block *sb)
* Now that we are completely done shutting down the
* superblock, we need to actually destroy the kobject.
*/
- unlock_kernel();
unlock_super(sb);
kobject_put(&sbi->s_kobj);
wait_for_completion(&sbi->s_kobj_unregister);
@@ -2328,19 +2325,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
int err;
unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
- lock_kernel();
-
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
sbi->s_blockgroup_lock =
kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
if (!sbi->s_blockgroup_lock) {
kfree(sbi);
- unlock_kernel();
return -ENOMEM;
}
sb->s_fs_info = sbi;
@@ -2352,8 +2344,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_sectors_written_start = part_stat_read(sb->s_bdev->bd_part,
sectors[1]);
- unlock_kernel();
-
/* Cleanup superblock name */
for (cp = sb->s_id; (cp = strchr(cp, '/'));)
*cp = '!';
@@ -3456,8 +3446,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
int i;
#endif
- lock_kernel();
-
/* Store the original options */
lock_super(sb);
old_sb_flags = sb->s_flags;
@@ -3587,7 +3575,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
kfree(old_opts.s_qf_names[i]);
#endif
unlock_super(sb);
- unlock_kernel();
return 0;
restore_opts:
@@ -3608,7 +3595,6 @@ restore_opts:
}
#endif
unlock_super(sb);
- unlock_kernel();
return err;
}
--
1.6.4.2
BKL is only used in get_sb. It is safe to remove it.
The only shared data structure that is accessed during get_sb is the session
list which is protected by a spinlock.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/9p/vfs_super.c | 9 +--------
1 files changed, 1 insertions(+), 8 deletions(-)
diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 4156a0c..14a8644 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -106,15 +106,11 @@ static int v9fs_get_sb(struct file_system_type *fs_type, int flags,
struct p9_fid *fid;
int retval = 0;
- lock_kernel();
-
P9_DPRINTK(P9_DEBUG_VFS, " \n");
v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);
- if (!v9ses) {
- unlock_kernel();
+ if (!v9ses)
return -ENOMEM;
- }
fid = v9fs_session_init(v9ses, dev_name, data);
if (IS_ERR(fid)) {
@@ -159,7 +155,6 @@ static int v9fs_get_sb(struct file_system_type *fs_type, int flags,
P9_DPRINTK(P9_DEBUG_VFS, " simple set mount, return 0\n");
simple_set_mnt(mnt, sb);
- unlock_kernel();
return 0;
free_stat:
@@ -172,14 +167,12 @@ clunk_fid:
close_session:
v9fs_session_close(v9ses);
kfree(v9ses);
- unlock_kernel();
return retval;
release_sb:
p9stat_free(st);
kfree(st);
deactivate_locked_super(sb);
- unlock_kernel();
return retval;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/autofs4/inode.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 3adaba9..69c8142 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -323,8 +323,6 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
struct autofs_sb_info *sbi;
struct autofs_info *ino;
- lock_kernel();
-
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
goto fail_unlock;
@@ -420,7 +418,6 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
* Success! Install the root dentry now to indicate completion.
*/
s->s_root = root;
- unlock_kernel();
return 0;
/*
@@ -442,7 +439,6 @@ fail_free:
kfree(sbi);
s->s_fs_info = NULL;
fail_unlock:
- unlock_kernel();
return -EINVAL;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/befs/linuxvfs.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index f2aa193..33baf27 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -759,8 +759,6 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
const unsigned long sb_block = 0;
const off_t x86_sb_off = 512;
- lock_kernel();
-
save_mount_options(sb, data);
sb->s_fs_info = kmalloc(sizeof (*befs_sb), GFP_KERNEL);
@@ -869,7 +867,6 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
befs_sb->nls = load_nls_default();
}
- unlock_kernel();
return 0;
/*****************/
unacquire_bh:
@@ -880,7 +877,6 @@ befs_fill_super(struct super_block *sb, void *data, int silent)
unacquire_none:
sb->s_fs_info = NULL;
- unlock_kernel();
return ret;
}
--
1.6.4.2
BKL is only used in get_sb. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/btrfs/super.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index e5cd2cf..752a546 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -480,17 +480,13 @@ static int btrfs_get_sb(struct file_system_type *fs_type, int flags,
fmode_t mode = FMODE_READ;
int error = 0;
- lock_kernel();
-
if (!(flags & MS_RDONLY))
mode |= FMODE_WRITE;
error = btrfs_parse_early_options(data, mode, fs_type,
&subvol_name, &fs_devices);
- if (error) {
- unlock_kernel();
+ if (error)
return error;
- }
error = btrfs_scan_one_device(dev_name, mode, fs_type, &fs_devices);
if (error)
@@ -559,7 +555,6 @@ static int btrfs_get_sb(struct file_system_type *fs_type, int flags,
mnt->mnt_root = root;
kfree(subvol_name);
- unlock_kernel();
return 0;
error_s:
@@ -568,7 +563,6 @@ error_close_devices:
btrfs_close_devices(fs_devices);
error_free_subvol_name:
kfree(subvol_name);
- unlock_kernel();
return error;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Since this filesystem uses get_sb_single(), fill_super is only called once.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/configfs/mount.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/fs/configfs/mount.c b/fs/configfs/mount.c
index 5b2e06e..8421cea 100644
--- a/fs/configfs/mount.c
+++ b/fs/configfs/mount.c
@@ -71,8 +71,6 @@ static int configfs_fill_super(struct super_block *sb, void *data, int silent)
struct inode *inode;
struct dentry *root;
- lock_kernel();
-
sb->s_blocksize = PAGE_CACHE_SIZE;
sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
sb->s_magic = CONFIGFS_MAGIC;
@@ -89,7 +87,6 @@ static int configfs_fill_super(struct super_block *sb, void *data, int silent)
inc_nlink(inode);
} else {
pr_debug("configfs: could not get root inode\n");
- unlock_kernel();
return -ENOMEM;
}
@@ -97,14 +94,12 @@ static int configfs_fill_super(struct super_block *sb, void *data, int silent)
if (!root) {
pr_debug("%s: could not get root dentry!\n",__func__);
iput(inode);
- unlock_kernel();
return -ENOMEM;
}
config_group_init(&configfs_root_group);
configfs_root_group.cg_item.ci_dentry = root;
root->d_fsdata = &configfs_root;
sb->s_root = root;
- unlock_kernel();
return 0;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/cramfs/inode.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 13e696a..dd3634e 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -227,15 +227,11 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
struct cramfs_sb_info *sbi;
struct inode *root;
- lock_kernel();
-
sb->s_flags |= MS_RDONLY;
sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
sb->s_fs_info = sbi;
/* Invalidate the read buffers on mount: think disk change.. */
@@ -312,12 +308,10 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
iput(root);
goto out;
}
- unlock_kernel();
return 0;
out:
kfree(sbi);
sb->s_fs_info = NULL;
- unlock_kernel();
return -EINVAL;
}
--
1.6.4.2
BKL is only used in get_sb. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/devpts/inode.c | 13 ++-----------
1 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e206eef..d5f8c96 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -24,7 +24,6 @@
#include <linux/parser.h>
#include <linux/fsnotify.h>
#include <linux/seq_file.h>
-#include <linux/smp_lock.h> /* just for lock_kernel() */
#define DEVPTS_DEFAULT_MODE 0600
/*
@@ -364,23 +363,17 @@ static int devpts_get_sb(struct file_system_type *fs_type,
struct pts_mount_opts opts;
struct super_block *s;
- lock_kernel();
-
error = parse_mount_options(data, PARSE_MOUNT, &opts);
- if (error) {
- unlock_kernel();
+ if (error)
return error;
- }
if (opts.newinstance)
s = sget(fs_type, NULL, set_anon_super, NULL);
else
s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
- if (IS_ERR(s)) {
- unlock_kernel();
+ if (IS_ERR(s))
return PTR_ERR(s);
- }
if (!s->s_root) {
s->s_flags = flags;
@@ -398,7 +391,6 @@ static int devpts_get_sb(struct file_system_type *fs_type,
if (error)
goto out_dput;
- unlock_kernel();
return 0;
out_dput:
@@ -406,7 +398,6 @@ out_dput:
out_undo_sget:
deactivate_locked_super(s);
- unlock_kernel();
return error;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/efs/super.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/fs/efs/super.c b/fs/efs/super.c
index 0981141..f049428 100644
--- a/fs/efs/super.c
+++ b/fs/efs/super.c
@@ -249,13 +249,9 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
struct inode *root;
int ret = -EINVAL;
- lock_kernel();
-
- sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
- if (!sb) {
- unlock_kernel();
+ sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
+ if (!sb)
return -ENOMEM;
- }
s->s_fs_info = sb;
s->s_magic = EFS_SUPER_MAGIC;
@@ -323,14 +319,12 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
goto out_no_fs;
}
- unlock_kernel();
return 0;
out_no_fs_ul:
out_no_fs:
s->s_fs_info = NULL;
kfree(sb);
- unlock_kernel();
return ret;
}
--
1.6.4.2
We don't need the BKL in exofs.
Signed-off-by: Boaz Harrosh <[email protected]>
Signed-off-by: Jan Blunck <[email protected]>
---
fs/exofs/super.c | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index ea045b8..8358532 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -31,7 +31,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*/
-#include <linux/smp_lock.h>
#include <linux/string.h>
#include <linux/parser.h>
#include <linux/vfs.h>
@@ -297,13 +296,10 @@ static int exofs_fill_super(struct super_block *sb, void *data, int silent)
struct osd_obj_id obj;
int ret;
- lock_kernel();
-
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
+
sb->s_fs_info = sbi;
/* use mount options to fill superblock */
@@ -403,7 +399,6 @@ static int exofs_fill_super(struct super_block *sb, void *data, int silent)
out:
if (or)
osd_end_request(or);
- unlock_kernel();
return ret;
free_sbi:
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/hostfs/hostfs_kern.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 5eb2c26..032604e 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -968,8 +968,6 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
char *host_root_path, *req_root = d;
int err;
- lock_kernel();
-
sb->s_blocksize = 1024;
sb->s_blocksize_bits = 10;
sb->s_magic = HOSTFS_SUPER_MAGIC;
@@ -1018,7 +1016,6 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
goto out;
}
- unlock_kernel();
return 0;
out_put:
@@ -1026,7 +1023,6 @@ out_put:
out_free:
kfree(host_root_path);
out:
- unlock_kernel();
return err;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/hugetlbfs/inode.c | 12 ++----------
1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 53be2b9..87a1258 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -32,7 +32,6 @@
#include <linux/security.h>
#include <linux/ima.h>
#include <linux/magic.h>
-#include <linux/smp_lock.h> /* Only for lock_kernel() */
#include <asm/uaccess.h>
@@ -825,7 +824,6 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
struct hugetlbfs_config config;
struct hugetlbfs_sb_info *sbinfo;
- lock_kernel();
save_mount_options(sb, data);
config.nr_blocks = -1; /* No limit on size by default */
@@ -835,16 +833,12 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
config.mode = 0755;
config.hstate = &default_hstate;
ret = hugetlbfs_parse_options(data, &config);
- if (ret) {
- unlock_kernel();
+ if (ret)
return ret;
- }
sbinfo = kmalloc(sizeof(struct hugetlbfs_sb_info), GFP_KERNEL);
- if (!sbinfo) {
- unlock_kernel();
+ if (!sbinfo)
return -ENOMEM;
- }
sb->s_fs_info = sbinfo;
sbinfo->hstate = config.hstate;
spin_lock_init(&sbinfo->stat_lock);
@@ -869,11 +863,9 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
goto out_free;
}
sb->s_root = root;
- unlock_kernel();
return 0;
out_free:
kfree(sbinfo);
- unlock_kernel();
return -ENOMEM;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/minix/inode.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index b8aa0a6..74ea82d 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -147,13 +147,9 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
struct minix_sb_info *sbi;
int ret = -EINVAL;
- lock_kernel();
-
sbi = kzalloc(sizeof(struct minix_sb_info), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
s->s_fs_info = sbi;
BUILD_BUG_ON(32 != sizeof (struct minix_inode));
@@ -269,7 +265,6 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
else if (sbi->s_mount_state & MINIX_ERROR_FS)
printk("MINIX-fs: mounting file system with errors, "
"running fsck is recommended\n");
- unlock_kernel();
return 0;
out_iput:
@@ -319,7 +314,6 @@ out_bad_sb:
out:
s->s_fs_info = NULL;
kfree(sbi);
- unlock_kernel();
return ret;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/omfs/inode.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c
index ddfbb22..f3b7c15 100644
--- a/fs/omfs/inode.c
+++ b/fs/omfs/inode.c
@@ -416,15 +416,11 @@ static int omfs_fill_super(struct super_block *sb, void *data, int silent)
sector_t start;
int ret = -EINVAL;
- lock_kernel();
-
save_mount_options(sb, (char *) data);
sbi = kzalloc(sizeof(struct omfs_sb_info), GFP_KERNEL);
- if (!sbi) {
- unlock_kernel();
+ if (!sbi)
return -ENOMEM;
- }
sb->s_fs_info = sbi;
@@ -529,7 +525,6 @@ out_brelse_bh2:
out_brelse_bh:
brelse(bh);
end:
- unlock_kernel();
return ret;
}
--
1.6.4.2
BKL is only used in fill_super and get_sb_single() is used. It is safe to
remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/openpromfs/inode.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c
index 50dc4be..ffcd04f 100644
--- a/fs/openpromfs/inode.c
+++ b/fs/openpromfs/inode.c
@@ -386,8 +386,6 @@ static int openprom_fill_super(struct super_block *s, void *data, int silent)
struct op_inode_info *oi;
int ret;
- lock_kernel();
-
s->s_flags |= MS_NOATIME;
s->s_blocksize = 1024;
s->s_blocksize_bits = 10;
@@ -407,7 +405,6 @@ static int openprom_fill_super(struct super_block *s, void *data, int silent)
s->s_root = d_alloc_root(root_inode);
if (!s->s_root)
goto out_no_root_dentry;
- unlock_kernel();
return 0;
out_no_root_dentry:
@@ -415,7 +412,6 @@ out_no_root_dentry:
ret = -ENOMEM;
out_no_root:
printk("openprom_fill_super: get root inode failed\n");
- unlock_kernel();
return ret;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ramfs/inode.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index 9b3983f..a6090aa 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -35,7 +35,6 @@
#include <linux/sched.h>
#include <linux/parser.h>
#include <linux/magic.h>
-#include <linux/smp_lock.h> /* Only for lock_kernel() */
#include <asm/uaccess.h>
#include "internal.h"
@@ -221,8 +220,6 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
struct dentry *root;
int err;
- lock_kernel();
-
save_mount_options(sb, data);
fsi = kzalloc(sizeof(struct ramfs_fs_info), GFP_KERNEL);
@@ -256,13 +253,11 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
goto fail;
}
- unlock_kernel();
return 0;
fail:
kfree(fsi);
sb->s_fs_info = NULL;
iput(inode);
- unlock_kernel();
return err;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/romfs/super.c | 9 +--------
1 files changed, 1 insertions(+), 8 deletions(-)
diff --git a/fs/romfs/super.c b/fs/romfs/super.c
index 7342617..c117fa8 100644
--- a/fs/romfs/super.c
+++ b/fs/romfs/super.c
@@ -468,8 +468,6 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
size_t len;
int ret;
- lock_kernel();
-
#ifdef CONFIG_BLOCK
if (!sb->s_mtd) {
sb_set_blocksize(sb, ROMBSIZE);
@@ -486,10 +484,8 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
/* read the image superblock and check it */
rsb = kmalloc(512, GFP_KERNEL);
- if (!rsb) {
- unlock_kernel();
+ if (!rsb)
return -ENOMEM;
- }
sb->s_fs_info = (void *) 512;
ret = romfs_dev_read(sb, 0, rsb, 512);
@@ -539,18 +535,15 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent)
if (!sb->s_root)
goto error_i;
- unlock_kernel();
return 0;
error_i:
iput(root);
error:
- unlock_kernel();
return -EINVAL;
error_rsb_inval:
ret = -EINVAL;
error_rsb:
- unlock_kernel();
return ret;
}
--
1.6.4.2
BKL is only used in fill_super and get_sb_single() is used. It is safe to
remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/sysfs/mount.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 2e5a870..9487575 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -10,7 +10,7 @@
* Please see Documentation/filesystems/sysfs.txt for more information.
*/
-#define DEBUG
+#define DEBUG
#include <linux/fs.h>
#include <linux/mount.h>
@@ -18,7 +18,6 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/magic.h>
-#include <linux/smp_lock.h> /* Only for lock_kernel() */
#include "sysfs.h"
@@ -46,8 +45,6 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
struct inode *inode;
struct dentry *root;
- lock_kernel();
-
sb->s_blocksize = PAGE_CACHE_SIZE;
sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
sb->s_magic = SYSFS_MAGIC;
@@ -61,7 +58,6 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
mutex_unlock(&sysfs_mutex);
if (!inode) {
pr_debug("sysfs: could not get root inode\n");
- unlock_kernel();
return -ENOMEM;
}
@@ -70,12 +66,10 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
if (!root) {
pr_debug("%s: could not get root dentry!\n",__func__);
iput(inode);
- unlock_kernel();
return -ENOMEM;
}
root->d_fsdata = &sysfs_root;
sb->s_root = root;
- unlock_kernel();
return 0;
}
--
1.6.4.2
BKL is only used in get_sb. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ubifs/super.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 04a0fc9..333e181 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2029,8 +2029,6 @@ static int ubifs_get_sb(struct file_system_type *fs_type, int flags,
struct super_block *sb;
int err;
- lock_kernel();
-
dbg_gen("name %s, flags %#x", name, flags);
/*
@@ -2042,7 +2040,6 @@ static int ubifs_get_sb(struct file_system_type *fs_type, int flags,
if (IS_ERR(ubi)) {
ubifs_err("cannot open \"%s\", error %d",
name, (int)PTR_ERR(ubi));
- unlock_kernel();
return PTR_ERR(ubi);
}
ubi_get_volume_info(ubi, &vi);
@@ -2080,14 +2077,12 @@ static int ubifs_get_sb(struct file_system_type *fs_type, int flags,
ubi_close_volume(ubi);
simple_set_mnt(mnt, sb);
- unlock_kernel();
return 0;
out_deact:
deactivate_locked_super(sb);
out_close:
ubi_close_volume(ubi);
- unlock_kernel();
return err;
}
--
1.6.4.2
BKL is only used in fill_super. It is safe to remove it.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/xfs/linux-2.6/xfs_super.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 7426166..18a4b8e 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -1412,8 +1412,6 @@ xfs_fs_fill_super(
int flags = 0, error = ENOMEM;
char *mtpt = NULL;
- lock_kernel();
-
mp = kzalloc(sizeof(struct xfs_mount), GFP_KERNEL);
if (!mp)
goto out;
@@ -1508,7 +1506,6 @@ xfs_fs_fill_super(
kfree(mtpt);
xfs_itrace_exit(XFS_I(sb->s_root->d_inode));
- unlock_kernel();
return 0;
out_filestream_unmount:
@@ -1525,7 +1522,6 @@ xfs_fs_fill_super(
kfree(mtpt);
kfree(mp);
out:
- unlock_kernel();
return -error;
fail_vnrele:
--
1.6.4.2
> @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_sb_block = sb_block;
>
> /*
> + * mutex for protection of modifications of the superblock while being
> + * write out by ext2_write_super() or ext2_sync_fs().
> + */
> + mutex_init(&sbi->s_mutex);
I didn't go over all the code paths in detail, but if you replace
the BKL with a mutex that is hold over a longer write-out sleep
period you potentially limit IO parallelism a lot.
In this case since the BKL didn't protect over the sleep anyways it might
be reasonable to drop it during the IO operation (and possibly if
you're sure nothing else sleeps use a spin lock)
-Andi
Acked-by: Coly Li <[email protected]>
On 2009年11月02日 18:04, Jan Blunck Wrote:
> BKL is only used in fill_super. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/cramfs/inode.c | 8 +-------
> 1 files changed, 1 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 13e696a..dd3634e 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -227,15 +227,11 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
> struct cramfs_sb_info *sbi;
> struct inode *root;
>
> - lock_kernel();
> -
> sb->s_flags |= MS_RDONLY;
>
> sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
> - if (!sbi) {
> - unlock_kernel();
> + if (!sbi)
> return -ENOMEM;
> - }
> sb->s_fs_info = sbi;
>
> /* Invalidate the read buffers on mount: think disk change.. */
> @@ -312,12 +308,10 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
> iput(root);
> goto out;
> }
> - unlock_kernel();
> return 0;
> out:
> kfree(sbi);
> sb->s_fs_info = NULL;
> - unlock_kernel();
> return -EINVAL;
> }
>
--
Coly Li
SuSE Labs
Acked-by: Coly Li <[email protected]>
On 2009年11月02日 18:05, Jan Blunck Wrote:
> BKL is only used in fill_super. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/minix/inode.c | 8 +-------
> 1 files changed, 1 insertions(+), 7 deletions(-)
>
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index b8aa0a6..74ea82d 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -147,13 +147,9 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
> struct minix_sb_info *sbi;
> int ret = -EINVAL;
>
> - lock_kernel();
> -
> sbi = kzalloc(sizeof(struct minix_sb_info), GFP_KERNEL);
> - if (!sbi) {
> - unlock_kernel();
> + if (!sbi)
> return -ENOMEM;
> - }
> s->s_fs_info = sbi;
>
> BUILD_BUG_ON(32 != sizeof (struct minix_inode));
> @@ -269,7 +265,6 @@ static int minix_fill_super(struct super_block *s, void *data, int silent)
> else if (sbi->s_mount_state & MINIX_ERROR_FS)
> printk("MINIX-fs: mounting file system with errors, "
> "running fsck is recommended\n");
> - unlock_kernel();
> return 0;
>
> out_iput:
> @@ -319,7 +314,6 @@ out_bad_sb:
> out:
> s->s_fs_info = NULL;
> kfree(sbi);
> - unlock_kernel();
> return ret;
> }
>
--
Coly Li
SuSE Labs
Acked-by: Coly Li <[email protected]>
On 2009年11月02日 18:04, Jan Blunck Wrote:
> BKL is only used in fill_super. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/efs/super.c | 10 ++--------
> 1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/fs/efs/super.c b/fs/efs/super.c
> index 0981141..f049428 100644
> --- a/fs/efs/super.c
> +++ b/fs/efs/super.c
> @@ -249,13 +249,9 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
> struct inode *root;
> int ret = -EINVAL;
>
> - lock_kernel();
> -
> - sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
> - if (!sb) {
> - unlock_kernel();
> + sb = kzalloc(sizeof(struct efs_sb_info), GFP_KERNEL);
> + if (!sb)
> return -ENOMEM;
> - }
> s->s_fs_info = sb;
>
> s->s_magic = EFS_SUPER_MAGIC;
> @@ -323,14 +319,12 @@ static int efs_fill_super(struct super_block *s, void *d, int silent)
> goto out_no_fs;
> }
>
> - unlock_kernel();
> return 0;
>
> out_no_fs_ul:
> out_no_fs:
> s->s_fs_info = NULL;
> kfree(sb);
> - unlock_kernel();
> return ret;
> }
>
--
Coly Li
SuSE Labs
Jan Blunck wrote:
> BKL is only used in fill_super. It is safe to remove it.
Yes, I think so too.
Of course this will make co-coordinating the rename of autofs4 to autofs
a bit more painful, ;)
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: Ian Kent <[email protected]>
> ---
> fs/autofs4/inode.c | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index 3adaba9..69c8142 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -323,8 +323,6 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
> struct autofs_sb_info *sbi;
> struct autofs_info *ino;
>
> - lock_kernel();
> -
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> if (!sbi)
> goto fail_unlock;
> @@ -420,7 +418,6 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
> * Success! Install the root dentry now to indicate completion.
> */
> s->s_root = root;
> - unlock_kernel();
> return 0;
>
> /*
> @@ -442,7 +439,6 @@ fail_free:
> kfree(sbi);
> s->s_fs_info = NULL;
> fail_unlock:
> - unlock_kernel();
> return -EINVAL;
> }
>
Am Montag 02 November 2009 11:04:40 schrieb Jan Blunck:
> During the realtime preemption mini-summit we discussed the entire removal
> of the big kernel lock. I've started working on this for some filesystems.
> My plan is to push the BKL down to the implementations first and remove it
> from there later.
>
> This series is pushing the BKL from do_new_mount() down to the filesystems
> and removes it from ext series of filesystems and one other trivial use:
> if the BKL is only used in get_sb/fill_super due to the push-down, we just
> need to make sure that parallel calls to get_sb/fill_super would race
> against each other.
seems that patch 1 (the pushdown) did not yet made it to the list. Looking at
you diffstat it seems that you only touched fs/*
There are filesystems in other places, e.g.
drivers/isdn/capi/capifs.c,
arch/powerpc/platforms/cell/spufs/inode.c
or
arch/s390/hypfs/inode.c
I am really not an expert in filesystems, so my comment might be bogus: My
expection was, that a simple pushdown should also affect these filesystems,
especially if the filesystems dont use simple_fill_super, no?
On Mon, Nov 02, 2009 at 11:04:52AM +0100, Jan Blunck wrote:
> BKL is only used in get_sb. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: Chris Mason <[email protected]>
Thanks!
-chris
> ---
> fs/btrfs/super.c | 8 +-------
> 1 files changed, 1 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index e5cd2cf..752a546 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -480,17 +480,13 @@ static int btrfs_get_sb(struct file_system_type *fs_type, int flags,
> fmode_t mode = FMODE_READ;
> int error = 0;
>
> - lock_kernel();
> -
> if (!(flags & MS_RDONLY))
> mode |= FMODE_WRITE;
>
> error = btrfs_parse_early_options(data, mode, fs_type,
> &subvol_name, &fs_devices);
> - if (error) {
> - unlock_kernel();
> + if (error)
> return error;
> - }
>
> error = btrfs_scan_one_device(dev_name, mode, fs_type, &fs_devices);
> if (error)
> @@ -559,7 +555,6 @@ static int btrfs_get_sb(struct file_system_type *fs_type, int flags,
> mnt->mnt_root = root;
>
> kfree(subvol_name);
> - unlock_kernel();
> return 0;
>
> error_s:
> @@ -568,7 +563,6 @@ error_close_devices:
> btrfs_close_devices(fs_devices);
> error_free_subvol_name:
> kfree(subvol_name);
> - unlock_kernel();
> return error;
> }
>
> --
> 1.6.4.2
>
On Mon, Nov 02, Christian Borntraeger wrote:
> Am Montag 02 November 2009 11:04:40 schrieb Jan Blunck:
> > During the realtime preemption mini-summit we discussed the entire removal
> > of the big kernel lock. I've started working on this for some filesystems.
> > My plan is to push the BKL down to the implementations first and remove it
> > from there later.
> >
> > This series is pushing the BKL from do_new_mount() down to the filesystems
> > and removes it from ext series of filesystems and one other trivial use:
> > if the BKL is only used in get_sb/fill_super due to the push-down, we just
> > need to make sure that parallel calls to get_sb/fill_super would race
> > against each other.
>
> seems that patch 1 (the pushdown) did not yet made it to the list.
Seems that the CC list was too long because I used get_maintainer.pl together
with git-send-email on this patch ...
> Looking at
> you diffstat it seems that you only touched fs/*
>
> There are filesystems in other places, e.g.
> drivers/isdn/capi/capifs.c,
> arch/powerpc/platforms/cell/spufs/inode.c
> or
> arch/s390/hypfs/inode.c
>
> I am really not an expert in filesystems, so my comment might be bogus: My
> expection was, that a simple pushdown should also affect these filesystems,
> especially if the filesystems dont use simple_fill_super, no?
D'Oh! You are totally correct. Seems that nothing important outside of fs/
actually requires the BKL since my box is still wor
Am Montag 02 November 2009 14:30:42 schrieb Jan Blunck:
> On Mon, Nov 02, Christian Borntraeger wrote:
> > Am Montag 02 November 2009 11:04:40 schrieb Jan Blunck:
> > > During the realtime preemption mini-summit we discussed the entire
> > > removal of the big kernel lock. I've started working on this for some
> > > filesystems. My plan is to push the BKL down to the implementations
> > > first and remove it from there later.
> > >
> > > This series is pushing the BKL from do_new_mount() down to the
> > > filesystems and removes it from ext series of filesystems and one other
> > > trivial use: if the BKL is only used in get_sb/fill_super due to the
> > > push-down, we just need to make sure that parallel calls to
> > > get_sb/fill_super would race against each other.
> >
> > seems that patch 1 (the pushdown) did not yet made it to the list.
>
> Seems that the CC list was too long because I used get_maintainer.pl
> together with git-send-email on this patch ...
>
> > Looking at
> > you diffstat it seems that you only touched fs/*
> >
> > There are filesystems in other places, e.g.
> > drivers/isdn/capi/capifs.c,
> > arch/powerpc/platforms/cell/spufs/inode.c
> > or
> > arch/s390/hypfs/inode.c
> >
> > I am really not an expert in filesystems, so my comment might be bogus:
> > My expection was, that a simple pushdown should also affect these
> > filesystems, especially if the filesystems dont use simple_fill_super,
> > no?
>
> D'Oh! You are totally correct. Seems that nothing important outside of fs/
> actually requires the BKL since my box is still wor
>
Am Montag 02 November 2009 14:30:42 schrieb Jan Blunck:
> > Looking at
> > you diffstat it seems that you only touched fs/*
> >
> > There are filesystems in other places, e.g.
> > drivers/isdn/capi/capifs.c,
> > arch/powerpc/platforms/cell/spufs/inode.c
> > or
> > arch/s390/hypfs/inode.c
> >
> > I am really not an expert in filesystems, so my comment might be bogus:
> > My expection was, that a simple pushdown should also affect these
> > filesystems, especially if the filesystems dont use simple_fill_super,
> > no?
>
> D'Oh! You are totally correct. Seems that nothing important outside of fs/
> actually requires the BKL since my box is still wor
Can you change your series to do the full pushdown anyway? grep for
register_filesystem find several places that probably dont get much testing and
if something goes wrong a full pushdown + blk removal would allow to find the
problem with bisect later on.
On Mon, Nov 02, 2009 at 11:05:05AM +0100, Jan Blunck wrote:
> BKL is only used in fill_super and get_sb_single() is used. It is safe to
> remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Is this going through your tree to Linus, or do you want me to take this
patch through mine?
thanks,
greg k-h
On Mon, Nov 02, Andi Kleen wrote:
> > @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > sbi->s_sb_block = sb_block;
> >
> > /*
> > + * mutex for protection of modifications of the superblock while being
> > + * write out by ext2_write_super() or ext2_sync_fs().
> > + */
> > + mutex_init(&sbi->s_mutex);
>
> I didn't go over all the code paths in detail, but if you replace
> the BKL with a mutex that is hold over a longer write-out sleep
> period you potentially limit IO parallelism a lot.
Right. I converted it to be a spinlock and unlock before calling
ext2_sync_super().
What do you think?
Thanks,
Jan
> Right. I converted it to be a spinlock and unlock before calling
> ext2_sync_super().
>
> What do you think?
Again didn't go through all the code paths, but it seems like a good
approach.
-Andi
On Mon, Nov 02, Greg KH wrote:
> On Mon, Nov 02, 2009 at 11:05:05AM +0100, Jan Blunck wrote:
> > BKL is only used in fill_super and get_sb_single() is used. It is safe to
> > remove it.
> >
> > Signed-off-by: Jan Blunck <[email protected]>
>
> Acked-by: Greg Kroah-Hartman <[email protected]>
>
> Is this going through your tree to Linus, or do you want me to take this
> patch through mine?
I guess this should go through tip since there are already other bkl removal
patches in per-topic branches there.
Thanks,
Jan
On Mon, Nov 02, 2009 at 11:04:53AM +0100, Jan Blunck wrote:
> BKL is only used in fill_super. It is safe to remove it.
>
> Since this filesystem uses get_sb_single(), fill_super is only called once.
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: Joel Becker <[email protected]>
--
"There is no sincerer love than the love of food."
- George Bernard Shaw
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Mon, Nov 02, 2009 at 06:35:43PM +0100, Jan Blunck wrote:
> On Mon, Nov 02, Greg KH wrote:
>
> > On Mon, Nov 02, 2009 at 11:05:05AM +0100, Jan Blunck wrote:
> > > BKL is only used in fill_super and get_sb_single() is used. It is safe to
> > > remove it.
> > >
> > > Signed-off-by: Jan Blunck <[email protected]>
> >
> > Acked-by: Greg Kroah-Hartman <[email protected]>
> >
> > Is this going through your tree to Linus, or do you want me to take this
> > patch through mine?
>
> I guess this should go through tip since there are already other bkl removal
> patches in per-topic branches there.
Ok, no objection from me.
thanks,
greg k-h
On Mon, 2009-11-02 at 11:05 +0100, Jan Blunck wrote:
> BKL is only used in get_sb. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: Artem Bityutskiy <[email protected]>
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Mon, Nov 02, 2009 at 11:05:01AM +0100, Jan Blunck wrote:
> BKL is only used in fill_super. It is safe to remove it.
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/omfs/inode.c | 7 +------
> 1 files changed, 1 insertions(+), 6 deletions(-)
This patch and omfs parts of 1:
Acked-by: Bob Copeland <[email protected]>
--
Bob Copeland %% http://www.bobcopeland.com
On Mon 02-11-09 11:04:46, Jan Blunck wrote:
> The BKL is protecting nothing than two memory allocations here.
The patch looks good.
Acked-by: Jan Kara <[email protected]>
Should I merge it or will you do it with other patches?
Honza
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/ext3/super.c | 13 +------------
> 1 files changed, 1 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 38261a5..4b635b7 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -1568,19 +1568,14 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
> __le32 features;
> int err;
>
> - lock_kernel();
> -
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> - if (!sbi) {
> - unlock_kernel();
> + if (!sbi)
> return -ENOMEM;
> - }
>
> sbi->s_blockgroup_lock =
> kzalloc(sizeof(struct blockgroup_lock), GFP_KERNEL);
> if (!sbi->s_blockgroup_lock) {
> kfree(sbi);
> - unlock_kernel();
> return -ENOMEM;
> }
> sb->s_fs_info = sbi;
> @@ -1589,8 +1584,6 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
> sbi->s_resgid = EXT3_DEF_RESGID;
> sbi->s_sb_block = sb_block;
>
> - unlock_kernel();
> -
> blocksize = sb_min_blocksize(sb, EXT3_MIN_BLOCK_SIZE);
> if (!blocksize) {
> printk(KERN_ERR "EXT3-fs: unable to set blocksize\n");
> @@ -1996,8 +1989,6 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)
> test_opt(sb,DATA_FLAGS) == EXT3_MOUNT_ORDERED_DATA ? "ordered":
> "writeback");
>
> - lock_kernel();
> - unlock_kernel();
> return 0;
>
> cantfind_ext3:
> @@ -2027,8 +2018,6 @@ out_fail:
> sb->s_fs_info = NULL;
> kfree(sbi->s_blockgroup_lock);
> kfree(sbi);
> - lock_kernel();
> - unlock_kernel();
> return ret;
> }
>
> --
> 1.6.4.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 02-11-09 11:04:47, Jan Blunck wrote:
> The BKL lock is protecting the remounting against a potential call to
> ext3_put_super(). This could not happen, since this is protected by the
> s_umount rw semaphore of struct super_block.
>
> Therefore I think the BKL is protecting nothing here.
Yes, I'm also not aware of anything BKL would really protect. So I'm fine
with removing it here.
Acked-by: Jan Kara <[email protected]>
I can merge this patch as well if you want.
Honza
>
> Signed-off-by: Jan Blunck <[email protected]>
> ---
> fs/ext3/super.c | 8 --------
> 1 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 4b635b7..3ddce03 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -398,8 +398,6 @@ static void ext3_put_super (struct super_block * sb)
> struct ext3_super_block *es = sbi->s_es;
> int i, err;
>
> - lock_kernel();
> -
> ext3_xattr_put_super(sb);
> err = journal_destroy(sbi->s_journal);
> sbi->s_journal = NULL;
> @@ -448,8 +446,6 @@ static void ext3_put_super (struct super_block * sb)
> sb->s_fs_info = NULL;
> kfree(sbi->s_blockgroup_lock);
> kfree(sbi);
> -
> - unlock_kernel();
> }
>
> static struct kmem_cache *ext3_inode_cachep;
> @@ -2495,8 +2491,6 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
> int i;
> #endif
>
> - lock_kernel();
> -
> /* Store the original options */
> lock_super(sb);
> old_sb_flags = sb->s_flags;
> @@ -2607,7 +2601,6 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data)
> kfree(old_opts.s_qf_names[i]);
> #endif
> unlock_super(sb);
> - unlock_kernel();
> return 0;
> restore_opts:
> sb->s_flags = old_sb_flags;
> @@ -2625,7 +2618,6 @@ restore_opts:
> }
> #endif
> unlock_super(sb);
> - unlock_kernel();
> return err;
> }
>
> --
> 1.6.4.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 02-11-09 11:04:45, Jan Blunck wrote:
> The BKL is still used in ext2_put_super(), ext2_fill_super(), ext2_sync_fs()
> ext2_remount() and ext2_write_inode(). From these calls ext2_put_super(),
> ext2_fill_super() and ext2_remount() are protected against each other by
> the struct super_block s_umount rw semaphore. The call in ext2_write_inode()
> could only protect the modification of the ext2_sb_info through
> ext2_update_dynamic_rev() against concurrent ext2_sync_fs() or ext2_remount().
> ext2_fill_super() and ext2_put_super() can be left out because you need a
> valid filesystem reference in all three cases, which you do not have when
> you are one of these functions.
This is not quite true. Actually, ext2_sync_fs() is called also with
s_umount held (for reading) so it cannot race with any of the mounting /
umounting functions. But it should probably be guarded against running two
instances of ext2_sync_fs() in parallel.
Looking into the code, we probably have a race in ext2_write_inode that
two threads can write one inode in parallel. But that's a separe issue from
BKL removal.
Honza
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 057074e..8b51416 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -22,7 +22,6 @@
> * Assorted race fixes, rewrite of ext2_get_block() by Al Viro, 2000
> */
>
> -#include <linux/smp_lock.h>
> #include <linux/time.h>
> #include <linux/highuid.h>
> #include <linux/pagemap.h>
> @@ -1400,13 +1399,11 @@ int ext2_write_inode(struct inode *inode, int do_sync)
> /* If this is the first large file
> * created, add a flag to the superblock.
> */
> - lock_kernel();
> mutex_lock(&EXT2_SB(sb)->s_mutex);
> ext2_update_dynamic_rev(sb);
> EXT2_SET_RO_COMPAT_FEATURE(sb,
> EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> mutex_unlock(&EXT2_SB(sb)->s_mutex);
> - unlock_kernel();
> ext2_write_super(sb);
> }
> }
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index d610eb9..7e819ac 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -26,7 +26,6 @@
> #include <linux/random.h>
> #include <linux/buffer_head.h>
> #include <linux/exportfs.h>
> -#include <linux/smp_lock.h>
> #include <linux/vfs.h>
> #include <linux/seq_file.h>
> #include <linux/mount.h>
> @@ -118,8 +117,6 @@ static void ext2_put_super (struct super_block * sb)
> int i;
> struct ext2_sb_info *sbi = EXT2_SB(sb);
>
> - lock_kernel();
> -
> if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
> ext2_sync_fs(sb, 1);
>
> @@ -143,8 +140,6 @@ static void ext2_put_super (struct super_block * sb)
> sb->s_fs_info = NULL;
> kfree(sbi->s_blockgroup_lock);
> kfree(sbi);
> -
> - unlock_kernel();
> }
>
> static struct kmem_cache * ext2_inode_cachep;
> @@ -750,8 +745,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> __le32 features;
> int err;
>
> - lock_kernel();
> -
> err = -ENOMEM;
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> if (!sbi)
> @@ -1077,7 +1070,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> ext2_warning(sb, __func__,
> "mounting ext3 filesystem as ext2");
> ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
> - unlock_kernel();
> return 0;
>
> cantfind_ext2:
> @@ -1102,7 +1094,6 @@ failed_sbi:
> kfree(sbi->s_blockgroup_lock);
> kfree(sbi);
> failed_unlock:
> - unlock_kernel();
> return ret;
> }
>
> @@ -1140,7 +1131,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
> {
> struct ext2_super_block *es = EXT2_SB(sb)->s_es;
>
> - lock_kernel();
> if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
> ext2_debug("setting valid to 0\n");
> es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
> @@ -1154,7 +1144,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
> ext2_commit_super(sb, es);
> }
> sb->s_dirt = 0;
> - unlock_kernel();
>
> return 0;
> }
> @@ -1181,7 +1170,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> unsigned long old_sb_flags;
> int err;
>
> - lock_kernel();
> mutex_lock(&sbi->s_mutex);
>
> /* Store the old options */
> @@ -1221,14 +1209,12 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> }
> if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY)) {
> mutex_unlock(&sbi->s_mutex);
> - unlock_kernel();
> return 0;
> }
> if (*flags & MS_RDONLY) {
> if (le16_to_cpu(es->s_state) & EXT2_VALID_FS ||
> !(sbi->s_mount_state & EXT2_VALID_FS)) {
> mutex_unlock(&sbi->s_mutex);
> - unlock_kernel();
> return 0;
> }
> /*
> @@ -1258,7 +1244,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> }
> ext2_sync_super(sb, es);
> mutex_unlock(&sbi->s_mutex);
> - unlock_kernel();
> return 0;
> restore_opts:
> sbi->s_mount_opt = old_opts.s_mount_opt;
> @@ -1266,7 +1251,6 @@ restore_opts:
> sbi->s_resgid = old_opts.s_resgid;
> sb->s_flags = old_sb_flags;
> mutex_unlock(&sbi->s_mutex);
> - unlock_kernel();
> return err;
> }
>
> --
> 1.6.4.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Nov 05, Jan Kara wrote:
> On Mon 02-11-09 11:04:45, Jan Blunck wrote:
> > The BKL is still used in ext2_put_super(), ext2_fill_super(), ext2_sync_fs()
> > ext2_remount() and ext2_write_inode(). From these calls ext2_put_super(),
> > ext2_fill_super() and ext2_remount() are protected against each other by
> > the struct super_block s_umount rw semaphore. The call in ext2_write_inode()
> > could only protect the modification of the ext2_sb_info through
> > ext2_update_dynamic_rev() against concurrent ext2_sync_fs() or ext2_remount().
> > ext2_fill_super() and ext2_put_super() can be left out because you need a
> > valid filesystem reference in all three cases, which you do not have when
> > you are one of these functions.
> This is not quite true. Actually, ext2_sync_fs() is called also with
> s_umount held (for reading) so it cannot race with any of the mounting /
> umounting functions. But it should probably be guarded against running two
> instances of ext2_sync_fs() in parallel.
> Looking into the code, we probably have a race in ext2_write_inode that
> two threads can write one inode in parallel. But that's a separe issue from
> BKL removal.
Have you actually seen that the previous patch introduces s_mutex? Meanwhile
that mutex has been replaced by a spinlock due to Andy's feedback.
Anyway, thanks for the review.
Cheers,
Jan
> Honza
>
> > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > index 057074e..8b51416 100644
> > --- a/fs/ext2/inode.c
> > +++ b/fs/ext2/inode.c
> > @@ -22,7 +22,6 @@
> > * Assorted race fixes, rewrite of ext2_get_block() by Al Viro, 2000
> > */
> >
> > -#include <linux/smp_lock.h>
> > #include <linux/time.h>
> > #include <linux/highuid.h>
> > #include <linux/pagemap.h>
> > @@ -1400,13 +1399,11 @@ int ext2_write_inode(struct inode *inode, int do_sync)
> > /* If this is the first large file
> > * created, add a flag to the superblock.
> > */
> > - lock_kernel();
> > mutex_lock(&EXT2_SB(sb)->s_mutex);
> > ext2_update_dynamic_rev(sb);
> > EXT2_SET_RO_COMPAT_FEATURE(sb,
> > EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> > mutex_unlock(&EXT2_SB(sb)->s_mutex);
> > - unlock_kernel();
> > ext2_write_super(sb);
> > }
> > }
> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > index d610eb9..7e819ac 100644
> > --- a/fs/ext2/super.c
> > +++ b/fs/ext2/super.c
> > @@ -26,7 +26,6 @@
> > #include <linux/random.h>
> > #include <linux/buffer_head.h>
> > #include <linux/exportfs.h>
> > -#include <linux/smp_lock.h>
> > #include <linux/vfs.h>
> > #include <linux/seq_file.h>
> > #include <linux/mount.h>
> > @@ -118,8 +117,6 @@ static void ext2_put_super (struct super_block * sb)
> > int i;
> > struct ext2_sb_info *sbi = EXT2_SB(sb);
> >
> > - lock_kernel();
> > -
> > if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
> > ext2_sync_fs(sb, 1);
> >
> > @@ -143,8 +140,6 @@ static void ext2_put_super (struct super_block * sb)
> > sb->s_fs_info = NULL;
> > kfree(sbi->s_blockgroup_lock);
> > kfree(sbi);
> > -
> > - unlock_kernel();
> > }
> >
> > static struct kmem_cache * ext2_inode_cachep;
> > @@ -750,8 +745,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > __le32 features;
> > int err;
> >
> > - lock_kernel();
> > -
> > err = -ENOMEM;
> > sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> > if (!sbi)
> > @@ -1077,7 +1070,6 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > ext2_warning(sb, __func__,
> > "mounting ext3 filesystem as ext2");
> > ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
> > - unlock_kernel();
> > return 0;
> >
> > cantfind_ext2:
> > @@ -1102,7 +1094,6 @@ failed_sbi:
> > kfree(sbi->s_blockgroup_lock);
> > kfree(sbi);
> > failed_unlock:
> > - unlock_kernel();
> > return ret;
> > }
> >
> > @@ -1140,7 +1131,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
> > {
> > struct ext2_super_block *es = EXT2_SB(sb)->s_es;
> >
> > - lock_kernel();
> > if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
> > ext2_debug("setting valid to 0\n");
> > es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
> > @@ -1154,7 +1144,6 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
> > ext2_commit_super(sb, es);
> > }
> > sb->s_dirt = 0;
> > - unlock_kernel();
> >
> > return 0;
> > }
> > @@ -1181,7 +1170,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> > unsigned long old_sb_flags;
> > int err;
> >
> > - lock_kernel();
> > mutex_lock(&sbi->s_mutex);
> >
> > /* Store the old options */
> > @@ -1221,14 +1209,12 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> > }
> > if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY)) {
> > mutex_unlock(&sbi->s_mutex);
> > - unlock_kernel();
> > return 0;
> > }
> > if (*flags & MS_RDONLY) {
> > if (le16_to_cpu(es->s_state) & EXT2_VALID_FS ||
> > !(sbi->s_mount_state & EXT2_VALID_FS)) {
> > mutex_unlock(&sbi->s_mutex);
> > - unlock_kernel();
> > return 0;
> > }
> > /*
> > @@ -1258,7 +1244,6 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
> > }
> > ext2_sync_super(sb, es);
> > mutex_unlock(&sbi->s_mutex);
> > - unlock_kernel();
> > return 0;
> > restore_opts:
> > sbi->s_mount_opt = old_opts.s_mount_opt;
> > @@ -1266,7 +1251,6 @@ restore_opts:
> > sbi->s_resgid = old_opts.s_resgid;
> > sb->s_flags = old_sb_flags;
> > mutex_unlock(&sbi->s_mutex);
> > - unlock_kernel();
> > return err;
> > }
> >
> > --
> > 1.6.4.2
> >
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On Mon 02-11-09 17:57:52, Jan Blunck wrote:
> On Mon, Nov 02, Andi Kleen wrote:
>
> > > @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > > sbi->s_sb_block = sb_block;
> > >
> > > /*
> > > + * mutex for protection of modifications of the superblock while being
> > > + * write out by ext2_write_super() or ext2_sync_fs().
> > > + */
> > > + mutex_init(&sbi->s_mutex);
> >
> > I didn't go over all the code paths in detail, but if you replace
> > the BKL with a mutex that is hold over a longer write-out sleep
> > period you potentially limit IO parallelism a lot.
>
> Right. I converted it to be a spinlock and unlock before calling
> ext2_sync_super().
>
> What do you think?
The patch is generally fine. I have just a few minor comments below:
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 5af1775..70c326c 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -52,8 +52,10 @@ void ext2_error (struct super_block * sb, const char * function,
> struct ext2_super_block *es = sbi->s_es;
>
> if (!(sb->s_flags & MS_RDONLY)) {
> + spin_lock(&sbi->s_lock);
> sbi->s_mount_state |= EXT2_ERROR_FS;
> es->s_state |= cpu_to_le16(EXT2_ERROR_FS);
> + /* drops sbi->s_lock */
> ext2_sync_super(sb, es);
I don't like this dropping of spinlock inside ext2_sync_super. Can we
just drop it here and retake it in ext2_sync_super? It's by far not a
performance critical path so it should not really matter.
> diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
> index 1cdb663..0d20278 100644
> --- a/include/linux/ext2_fs_sb.h
> +++ b/include/linux/ext2_fs_sb.h
> @@ -106,6 +106,8 @@ struct ext2_sb_info {
> spinlock_t s_rsv_window_lock;
> struct rb_root s_rsv_window_root;
> struct ext2_reserve_window_node s_rsv_window_head;
> + /* protect against concurrent modifications of this structure */
> + spinlock_t s_lock;
> };
As I'm reading the code s_lock protects some of the fieds but definitely
not all. I'd say it protects s_mount_state, s_blocks_last, s_overhead_last,
and a content of superblock's buffer pointed to by sbi->s_es. The rest just
either does not change during lifetime of the filesystem or has different
locks (either s_umount semaphore or other spinlocks).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu 05-11-09 14:06:35, Jan Blunck wrote:
> On Thu, Nov 05, Jan Kara wrote:
>
> > On Mon 02-11-09 11:04:45, Jan Blunck wrote:
> > > The BKL is still used in ext2_put_super(), ext2_fill_super(), ext2_sync_fs()
> > > ext2_remount() and ext2_write_inode(). From these calls ext2_put_super(),
> > > ext2_fill_super() and ext2_remount() are protected against each other by
> > > the struct super_block s_umount rw semaphore. The call in ext2_write_inode()
> > > could only protect the modification of the ext2_sb_info through
> > > ext2_update_dynamic_rev() against concurrent ext2_sync_fs() or ext2_remount().
> > > ext2_fill_super() and ext2_put_super() can be left out because you need a
> > > valid filesystem reference in all three cases, which you do not have when
> > > you are one of these functions.
> > This is not quite true. Actually, ext2_sync_fs() is called also with
> > s_umount held (for reading) so it cannot race with any of the mounting /
> > umounting functions. But it should probably be guarded against running two
> > instances of ext2_sync_fs() in parallel.
> > Looking into the code, we probably have a race in ext2_write_inode that
> > two threads can write one inode in parallel. But that's a separe issue from
> > BKL removal.
>
> Have you actually seen that the previous patch introduces s_mutex? Meanwhile
> that mutex has been replaced by a spinlock due to Andy's feedback.
Ops, I wanted to comment on that patch as well but then decided it might
be better to write my version and didn't get to it yet... Thinking more
about it, probably go ahead with these two patches (modulo some changes
I've now suggested in a reply to your second ext2 patch) and I can cleanup
ext2 code after that.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Nov 02, 2009 at 11:04:48AM +0100, Jan Blunck wrote:
> The BKL is still used in ext4_put_super(), ext4_fill_super() and
> ext4_remount(). All three calles are protected against concurrent calls by
> the s_umount rw semaphore of struct super_block.
>
> Therefore the BKL is protecting nothing in this case.
>
> Signed-off-by: Jan Blunck <[email protected]>
Acked-by: "Theodore Ts'o" <[email protected]>
- Ted