Implement some low hanging BKL removal fruit in fs/*
Subsystems (14)
cover Implement some low hanging BKL removal fruit in fs/*
[Andrew: I believe this is -mm material for .25]
- Convert some more file systems (generally those who don't use the BKL for anything except mount) to use unlocked_bkl.
- Implement BKL less fasync (see patch for the rationale) This is currently a separate entry point, but since the number of fasync users in the tree is relatively small I hope the older entry point can be removed then in the not too far future
[help from other people converting more fasync users to unlocked_fasync would be appreciated]
- Implement BKL less remote_llseek
- While I was at it I also added a few missing compat ioctl handlers
- Fix a few comments
This fixes a lot of relatively trivial BKL users in fs/*. The main remaining non legacy offenders are now locks.c, nfs/nfsd and reiserfs. I believe BKL removal for all of those is being worked on by other people. Also a lot of "legacy" file systems still use it, but converting those does not seem to be very pressing.
-Andi
PATCH 1/18 BKL-removal: Convert ext2 over to use unlocked_ioctl
I checked ext2_ioctl and could not find anything in there that would need the BKL. So convert it over to use unlocked_ioctl
--- fs/ext2/dir.c | 2 +- fs/ext2/ext2.h | 3 +-- fs/ext2/file.c | 4 ++-- fs/ext2/ioctl.c | 12 +++--------- 4 files changed, 7 insertions(+), 14 deletions(-)
fs/ext2/dir.c +1 -1
@@ -703,7 +703,7 @@ const struct file_operations ext2_dir_op .llseek = generic_file_llseek, .read = generic_read_dir, .readdir = ext2_readdir, - .ioctl = ext2_ioctl, + .unlocked_ioctl = ext2_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext2_compat_ioctl,#endif
fs/ext2/ext2.h +1 -2
@@ -139,8 +139,7 @@ int __ext2_write_begin(struct file *file struct page **pagep, void **fsdata);/* ioctl.c */ -extern int ext2_ioctl (struct inode *,struct file *,unsigned int , -unsigned long ); +extern long ext2_ioctl (struct file *,unsigned int ,unsigned long );extern long ext2_compat_ioctl (struct file *,unsigned int ,unsigned long );/* namei.c */
fs/ext2/file.c +2 -2
@@ -48,7 +48,7 @@ const struct file_operations ext2_file_o .write = do_sync_write, .aio_read = generic_file_aio_read, .aio_write = generic_file_aio_write, - .ioctl = ext2_ioctl, + .unlocked_ioctl = ext2_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext2_compat_ioctl,#endif
@@ -65,7 +65,7 @@ const struct file_operations ext2_xip_fi .llseek = generic_file_llseek, .read = xip_file_read, .write = xip_file_write, - .ioctl = ext2_ioctl, + .unlocked_ioctl = ext2_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext2_compat_ioctl,#endif
fs/ext2/ioctl.c +3 -9
@@ -17,9 +17,9 @@#include <asm/uaccess.h> -int ext2_ioctl (struct inode * inode,struct file * filp,unsigned int cmd, -unsigned long arg) +long ext2_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) { +struct inode *inode = filp->f_dentry->d_inode;struct ext2_inode_info *ei =EXT2_I (inode);unsigned int flags;unsigned short rsv_window_size;
@@ -141,9 +141,6 @@ int ext2_ioctl (struct inode * inode, st#ifdef CONFIG_COMPAT long ext2_compat_ioctl (struct file *file,unsigned int cmd,unsigned long arg) { -struct inode *inode = file->f_path.dentry->d_inode; -int ret; -/* These are just misnamed, they actually get/put from/to user an int */ switch (cmd) {case EXT2_IOC32_GETFLAGS :
@@ -161,9 +158,6 @@ long ext2_compat_ioctl(struct file *filedefault : return -ENOIOCTLCMD; } -lock_kernel (); - ret =ext2_ioctl (inode, file, cmd, (unsigned long )compat_ptr (arg)); -unlock_kernel (); -return ret; +return ext2_ioctl (file, cmd, (unsigned long )compat_ptr (arg)); }#endif
PATCH 2/18 BKL-removal: Remove incorrect BKL comment in ext2
In reply to Andi KleenNo BKL used anywhere, so don't mention it.
--- fs/ext2/inode.c | 1 - 1 file changed, 1 deletion(-)
fs/ext2/inode.c +0 -1
@@ -569,7 +569,6 @@ static void ext2_splice_branch(struct in* * `handle' can be NULL if create == 0. * -* The BKL may not be held on entry here. Be sure to take it early. * return > 0, # of blocks mapped or allocated. * return = 0, if plain lookup failed. * return < 0, error case.
PATCH 3/18 BKL-removal: Convert ext3 to use unlocked_ioctl
In reply to Andi KleenI checked ext3_ioctl and it looked largely safe to not be used without BKL. So convert it over to unlocked_ioctl.
The only case where I wasn't quite sure was for the dynamic fs grow ioctls versus umounting -- I kept the BKL for those.
--- fs/ext3/dir.c | 2 +- fs/ext3/file.c | 2 +- fs/ext3/ioctl.c | 21 +++++++++++---------- include/linux/ext3_fs.h | 3 +-- 4 files changed, 14 insertions(+), 14 deletions(-)
fs/ext3/dir.c +1 -1
@@ -42,7 +42,7 @@ const struct file_operations ext3_dir_op .llseek = generic_file_llseek, .read = generic_read_dir, .readdir = ext3_readdir, /* we take BKL. needed?*/ - .ioctl = ext3_ioctl, /* BKL held */ + .unlocked_ioctl = ext3_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext3_compat_ioctl,#endif
fs/ext3/file.c +1 -1
@@ -112,7 +112,7 @@ const struct file_operations ext3_file_o .write = do_sync_write, .aio_read = generic_file_aio_read, .aio_write = ext3_file_write, - .ioctl = ext3_ioctl, + .unlocked_ioctl = ext3_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext3_compat_ioctl,#endif
fs/ext3/ioctl.c +11 -10
@@ -17,9 +17,9 @@#include <linux/smp_lock.h> #include <asm/uaccess.h> -int ext3_ioctl (struct inode * inode,struct file * filp,unsigned int cmd, -unsigned long arg) +long ext3_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) { +struct inode *inode = filp->f_dentry->d_inode;struct ext3_inode_info *ei =EXT3_I (inode);unsigned int flags;unsigned short rsv_window_size;
@@ -224,10 +224,14 @@ flags_err:if (get_user (n_blocks_count, (__u32 __user *)arg))return -EFAULT ; +/* AK: not sure the BKL is needed, but this might prevent +* races against umount */ +lock_kernel (); err =ext3_group_extend (sb,EXT3_SB (sb)->s_es, n_blocks_count);journal_lock_updates (EXT3_SB (sb)->s_journal);journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); +unlock_kernel ();return err; }
@@ -245,11 +249,14 @@ flags_err:if (copy_from_user (&input, (struct ext3_new_group_input __user *)arg,sizeof (input)))return -EFAULT ; - +/* AK: not sure the BKL is needed, but this might prevent +* races against umount */ +lock_kernel (); err =ext3_group_add (sb, &input);journal_lock_updates (EXT3_SB (sb)->s_journal);journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); +unlock_kernel ();return err; }
@@ -263,9 +270,6 @@ flags_err:#ifdef CONFIG_COMPAT long ext3_compat_ioctl (struct file *file,unsigned int cmd,unsigned long arg) { -struct inode *inode = file->f_path.dentry->d_inode; -int ret; -/* These are just misnamed, they actually get/put from/to user an int */ switch (cmd) {case EXT3_IOC32_GETFLAGS :
@@ -305,9 +309,6 @@ long ext3_compat_ioctl(struct file *filedefault : return -ENOIOCTLCMD; } -lock_kernel (); - ret =ext3_ioctl (inode, file, cmd, (unsigned long )compat_ptr (arg)); -unlock_kernel (); -return ret; +return ext3_ioctl (file, cmd, (unsigned long )compat_ptr (arg)); }#endif
include/linux/ext3_fs.h +1 -2
@@ -838,8 +838,7 @@ extern void ext3_get_inode_flags(structextern void ext3_set_aops (struct inode *inode);/* ioctl.c */ -extern int ext3_ioctl (struct inode *,struct file *,unsigned int , -unsigned long ); +extern long ext3_ioctl (struct file *,unsigned int ,unsigned long );extern long ext3_compat_ioctl (struct file *,unsigned int ,unsigned long );/* namei.c */
Re: [PATCH] [3/18] BKL-removal: Convert ext3 to use unlocked_ioctl
Quoted 7 lines · On Sun, 27 Jan 2008 03:17:09 +0100 (CET) Andi Kleen <[email protected]> wrote:
I checked
ext3_ioctland it looked largely safe to not be used without BKL. So convert it over tounlocked_ioctl.The only case where I wasn't quite sure was for the dynamic fs grow ioctls versus umounting -- I kept the BKL for those.
Please cpoy linux-ext4 on ext2/3/4 material.
I skippped a lot of these patches because I just got bored of fixing rejects. Now is a very optimistic time to be raising patches against mainline.
I'm going to work on getting a unified devel tree operating: one which contains everyone's latest stuff and is updated daily. Basically it'll be -mm without a couple of the quilt trees. People can then prepare patches against that, as it seems that most can't be bothered patching against -mm, let alone building and testing it. More later.
Quoted 15 lines
fs/ext3/ioctl.c +4
-0+/* AK: not sure the BKL is needed, but this might prevent +* races against umount */ +lock_kernel (); err =ext3_group_extend (sb,EXT3_SB (sb)->s_es, n_blocks_count);journal_lock_updates (EXT3_SB (sb)->s_journal);journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); +unlock_kernel ();return err; }@@ -245,11 +249,14 @@ flags_err:if (copy_from_user (&input, (struct ext3_new_group_input __user *)arg,sizeof (input)))return -EFAULT ;
fs/ext3/ioctl.c flags_err: · line 260
Quoted 10 lines
fs/ext3/ioctl.c +4
-1- +/* AK: not sure the BKL is needed, but this might prevent +* races against umount */ +lock_kernel (); err =ext3_group_add (sb, &input);journal_lock_updates (EXT3_SB (sb)->s_journal);journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); +unlock_kernel ();
The ext3_ioctl() caller has an open fd against the fs - should be sufficient to keep unmount away?
(gets even more rejects, drops all the fasync patches too)
It's all reached the stage of stupid.
Re: [PATCH] [3/18] BKL-removal: Convert ext3 to use unlocked_ioctl
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Sun, 27 Jan 2008 03:17:09 +0100 (CET) Andi Kleen <[email protected]> wrote:
I checked
ext3_ioctland it looked largely safe to not be used without BKL. So convert it over tounlocked_ioctl.The only case where I wasn't quite sure was for the dynamic fs grow ioctls versus umounting -- I kept the BKL for those.
Please cpoy linux-ext4 on ext2/3/4 material.
Ok I'll resubmit those to tytso/ext4-devel (or perhaps he has already seen them)
Quoted 4 lines
I skippped a lot of these patches because I just got bored of fixing rejects. Now is a very optimistic time to be raising patches against mainline.
JFS and CIFS are already taken care of by the maintainers. This leaves remote_llseek which touches a couple of file systems. Could you perhaps take that one only please? And perhaps Nick's minix patchkit which looks safe to me and is unlikely to cause conflicts.
Quoted 11 lines
fs/ext3/ioctl.c flags_err: · line 259
Quoted 8 lines
fs/ext3/ioctl.c +4
-0+/* AK: not sure the BKL is needed, but this might prevent +* races against umount */ +lock_kernel (); err =ext3_group_add (sb, &input);journal_lock_updates (EXT3_SB (sb)->s_journal);journal_flush (EXT3_SB (sb)->s_journal);journal_unlock_updates (EXT3_SB (sb)->s_journal); +unlock_kernel ();The
ext3_ioctl()caller has an open fd against the fs - should be sufficient to keep unmount away?
True. I am still conservative because group_add is a lot of code which I didn't fully check. But with the open fd it's likely safe to not take the BKL because there is nothing else (except readdir?) in ext* that takes it.
Quoted 1 line
It's all reached the stage of stupid.
I'll resubmit ->fasync_unlocked against -mm.
Also I wanted to recheck the ->f_flags locking. I found one bug in those already and I can extract the bug fix for that one.
-Andi
PATCH 4/18 ext3: Remove incorrect BKL comment
In reply to Andi KleenThere is no BKL held on entry in ->fsync nor in the low level ext3_sync_file.
--- fs/ext3/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext3/dir.c +1 -1
@@ -46,7 +46,7 @@ const struct file_operations ext3_dir_op#ifdef CONFIG_COMPAT .compat_ioctl = ext3_compat_ioctl, #endif - .fsync = ext3_sync_file, /* BKL held */ + .fsync = ext3_sync_file, .release = ext3_release_dir, };
PATCH 5/18 BKL-removal: Remove incorrect comment refering to lock_kernel() from jbd/jbd2
In reply to Andi KleenNone of the callers of this function does actually take the BKL as far as I can see. So remove the comment refering to the BKL.
--- fs/jbd/recovery.c | 2 +- fs/jbd2/recovery.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
fs/jbd/recovery.c +1 -1
@@ -354,7 +354,7 @@ static int do_one_pass(journal_t *journastruct buffer_head * obh;struct buffer_head * nbh; -cond_resched ();/* We're under lock_kernel() */ +cond_resched ();/* If we already know where to stop the log traversal, * check right now that we haven't gone past the end of
fs/jbd2/recovery.c +1 -1
@@ -364,7 +364,7 @@ static int do_one_pass(journal_t *journastruct buffer_head * obh;struct buffer_head * nbh; -cond_resched ();/* We're under lock_kernel() */ +cond_resched ();/* If we already know where to stop the log traversal, * check right now that we haven't gone past the end of
PATCH 6/18 BKL-removal: Convert ext4 to use unlocked_ioctl
In reply to Andi KleenI checked ext4_ioctl and it looked largely safe to not be used without BKL. So convert it over to unlocked_ioctl.
The only case where I wasn't quite sure was for the dynamic fs grow ioctls versus umounting -- I kept the BKL for those.
--- fs/ext4/dir.c | 2 +- fs/ext4/file.c | 2 +- fs/ext4/ioctl.c | 20 +++++++++++--------- include/linux/ext4_fs.h | 3 +-- 4 files changed, 14 insertions(+), 13 deletions(-)
fs/ext4/dir.c +1 -1
@@ -42,7 +42,7 @@ const struct file_operations ext4_dir_op .llseek = generic_file_llseek, .read = generic_read_dir, .readdir = ext4_readdir, /* we take BKL. needed?*/ - .ioctl = ext4_ioctl, /* BKL held */ + .unlocked_ioctl = ext4_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext4_compat_ioctl,#endif
fs/ext4/file.c +1 -1
@@ -112,7 +112,7 @@ const struct file_operations ext4_file_o .write = do_sync_write, .aio_read = generic_file_aio_read, .aio_write = ext4_file_write, - .ioctl = ext4_ioctl, + .unlocked_ioctl = ext4_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ext4_compat_ioctl,#endif
fs/ext4/ioctl.c +11 -9
@@ -17,9 +17,9 @@#include <linux/smp_lock.h> #include <asm/uaccess.h> -int ext4_ioctl (struct inode * inode,struct file * filp,unsigned int cmd, -unsigned long arg) +long ext4_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) { +struct inode *inode = filp->f_dentry->d_inode;struct ext4_inode_info *ei =EXT4_I (inode);unsigned int flags;unsigned short rsv_window_size;
@@ -224,10 +224,14 @@ flags_err:if (get_user (n_blocks_count, (__u32 __user *)arg))return -EFAULT ; +/* AK: not sure the BKL is needed, but this might prevent +* races against umount. */ +lock_kernel (); err =ext4_group_extend (sb,EXT4_SB (sb)->s_es, n_blocks_count);jbd2_journal_lock_updates (EXT4_SB (sb)->s_journal);jbd2_journal_flush (EXT4_SB (sb)->s_journal);jbd2_journal_unlock_updates (EXT4_SB (sb)->s_journal); +unlock_kernel ();return err; }
@@ -246,10 +250,14 @@ flags_err: sizeof(input)))return -EFAULT ; +/* AK: not sure the BKL is needed, but this might prevent +* races against umount. */ +lock_kernel (); err =ext4_group_add (sb, &input);jbd2_journal_lock_updates (EXT4_SB (sb)->s_journal);jbd2_journal_flush (EXT4_SB (sb)->s_journal);jbd2_journal_unlock_updates (EXT4_SB (sb)->s_journal); +unlock_kernel ();return err; }
@@ -262,9 +270,6 @@ flags_err:#ifdef CONFIG_COMPAT long ext4_compat_ioctl (struct file *file,unsigned int cmd,unsigned long arg) { -struct inode *inode = file->f_path.dentry->d_inode; -int ret; -/* These are just misnamed, they actually get/put from/to user an int */ switch (cmd) {case EXT4_IOC32_GETFLAGS :
@@ -304,9 +309,6 @@ long ext4_compat_ioctl(struct file *filedefault : return -ENOIOCTLCMD; } -lock_kernel (); - ret =ext4_ioctl (inode, file, cmd, (unsigned long )compat_ptr (arg)); -unlock_kernel (); -return ret; +return ext4_ioctl (file, cmd, (unsigned long )compat_ptr (arg)); }#endif
include/linux/ext4_fs.h +1 -2
@@ -939,8 +939,7 @@ extern int ext4_block_truncate_page(hand struct address_space *mapping, loff_t from);/* ioctl.c */ -extern int ext4_ioctl (struct inode *,struct file *,unsigned int , -unsigned long ); +extern long ext4_ioctl (struct file *,unsigned int ,unsigned long );extern long ext4_compat_ioctl (struct file *,unsigned int ,unsigned long );/* namei.c */
PATCH 7/18 BKL-removal: Remove incorrect comments refering to BKL from ext4
In reply to Andi KleenBKL is not hold in any of those
--- fs/ext4/dir.c | 2 +- fs/ext4/inode.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
fs/ext4/dir.c +1 -1
@@ -46,7 +46,7 @@ const struct file_operations ext4_dir_op#ifdef CONFIG_COMPAT .compat_ioctl = ext4_compat_ioctl, #endif - .fsync = ext4_sync_file, /* BKL held */ + .fsync = ext4_sync_file, .release = ext4_release_dir, };
fs/ext4/inode.c +0 -1
@@ -778,7 +778,6 @@ err_out:* * `handle' can be NULL if create == 0. * -* The BKL may not be held on entry here. Be sure to take it early. * return > 0, # of blocks mapped or allocated. * return = 0, if plain lookup failed. * return < 0, error case.
PATCH 8/18 BKL-removal: Remove BKL from remote_llseek
In reply to Andi Kleen- Replace remote_llseek with remote_llseek_unlocked (to force compilation failures in all users)
- Change all users to either use remote_llseek directly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.
I moved them all over in a single patch to avoid unbisectable sections.
--- fs/cifs/cifsfs.c | 2 +- fs/gfs2/ops_file.c | 4 ++-- fs/ncpfs/file.c | 12 +++++++++++- fs/nfs/file.c | 6 +++++- fs/read_write.c | 7 +++---- fs/smbfs/file.c | 11 ++++++++++- include/linux/fs.h | 3 ++- 7 files changed, 34 insertions(+), 11 deletions(-)
fs/cifs/cifsfs.c +1 -1
@@ -549,7 +549,7 @@ static loff_t cifs_llseek(struct file *fif (retval <0 )return (loff_t )retval; } -return remote_llseek (file, offset, origin); +return remote_llseek_unlocked (file, offset, origin); }static struct file_system_type cifs_fs_type = {
fs/gfs2/ops_file.c +2 -2
@@ -107,11 +107,11 @@ static loff_t gfs2_llseek(struct file *f error =gfs2_glock_nq_init (ip->i_gl,LM_ST_SHARED ,LM_FLAG_ANY , &i_gh);if (!error) { - error =remote_llseek (file, offset, origin); + error =remote_llseek_unlocked (file, offset, origin);gfs2_glock_dq_uninit (&i_gh); } }else - error =remote_llseek (file, offset, origin); + error =remote_llseek_unlocked (file, offset, origin);return error; }
fs/ncpfs/file.c +11 -1
@@ -18,6 +18,7 @@#include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/sched.h> +#include <linux/smp_lock.h> #include <linux/ncp_fs.h> #include "ncplib_kernel.h"
@@ -281,9 +282,18 @@ static int ncp_release(struct inode *inoreturn 0 ; } +static loff_t ncp_remote_llseek (struct file *file,loff_t offset,int origin) +{ +loff_t ret; +lock_kernel (); + ret =remote_llseek_unlocked (file, offset, origin); +unlock_kernel (); +return ret; +} +const struct file_operations ncp_file_operations = { - .llseek = remote_llseek, + .llseek = ncp_remote_llseek, .read = ncp_file_read, .write = ncp_file_write, .ioctl = ncp_ioctl,
fs/read_write.c +3 -4
@@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *EXPORT_SYMBOL (generic_file_llseek); -loff_t remote_llseek (struct file *file,loff_t offset,int origin) +loff_t remote_llseek_unlocked (struct file *file,loff_t offset,int origin) {long long retval; -lock_kernel ();switch (origin) {case SEEK_END : offset +=i_size_read (file->f_path.dentry->d_inode);
@@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file, retval = -EINVAL ;if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {if (offset != file->f_pos) { +/* AK: do we need a lock for those? */ file->f_pos = offset; file->f_version =0 ; } retval = offset; } -unlock_kernel ();return retval; } -EXPORT_SYMBOL (remote_llseek); +EXPORT_SYMBOL (remote_llseek_unlocked);loff_t no_llseek (struct file *file,loff_t offset,int origin) {
fs/smbfs/file.c +10 -1
@@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode,return error; } +static loff_t smb_remote_llseek (struct file *file,loff_t offset,int origin) +{ +loff_t ret; +lock_kernel (); + ret =remote_llseek_unlocked (file, offset, origin); +unlock_kernel (); +return ret; +} +const struct file_operations smb_file_operations = { - .llseek = remote_llseek, + .llseek = smb_remote_llseek, .read = do_sync_read, .aio_read = smb_file_aio_read, .write = do_sync_write,
include/linux/fs.h +2 -1
@@ -1825,7 +1825,8 @@ extern void file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);extern loff_t no_llseek (struct file *file,loff_t offset,int origin);extern loff_t generic_file_llseek (struct file *file,loff_t offset,int origin); -extern loff_t remote_llseek (struct file *file,loff_t offset,int origin); +extern loff_t remote_llseek_unlocked (struct file *file,loff_t offset, +int origin);extern int generic_file_open (struct inode * inode,struct file * filp);extern int nonseekable_open (struct inode * inode,struct file * filp);
fs/nfs/file.c +5 -1
@@ -166,6 +166,7 @@ force_reval:static loff_t nfs_file_llseek (struct file *filp,loff_t offset,int origin) { +loff_t loff;/* origin == SEEK_END => we must revalidate the cached file length */ if (origin ==SEEK_END ) {struct inode *inode = filp->f_mapping->host;
@@ -173,7 +174,10 @@ static loff_t nfs_file_llseek(struct filif (retval <0 )return (loff_t )retval; } -return remote_llseek (filp, offset, origin); +lock_kernel ();/* BKL needed? */ + loff =remote_llseek_unlocked (filp, offset, origin); +unlock_kernel (); +return loff; }/*
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from f_pos in fs/read_write.c in remote_llseek with your patch since the reads/writes from that field are not necessarily atomic and threads could be racing in seek on the same file struct?
Quoted 188 lines · Andi Kleen
- Replace
remote_llseekwithremote_llseek_unlocked(to force compilation failures in all users)
- Change all users to either useremote_llseekdirectly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.I moved them all over in a single patch to avoid unbisectable sections.
Signed-off-by Andi Kleen <[email protected]>--- fs/cifs/cifsfs.c | 2 +- fs/gfs2/ops_file.c | 4 ++-- fs/ncpfs/file.c | 12 +++++++++++- fs/nfs/file.c | 6 +++++- fs/read_write.c | 7 +++---- fs/smbfs/file.c | 11 ++++++++++- include/linux/fs.h | 3 ++- 7 files changed, 34 insertions(+), 11 deletions(-)Index:
linux/fs/cifs/cifsfs.c
===================================================================fs/cifs/cifsfs.c +1
-1@@ -549,7 +549,7 @@ static loff_t cifs_llseek(struct file *fif (retval <0 )return (loff_t )retval; } -return remote_llseek (file, offset, origin); +return remote_llseek_unlocked (file, offset, origin); }static struct file_system_type cifs_fs_type = {Index:
linux/fs/gfs2/ops_file.c
===================================================================fs/gfs2/ops_file.c +2
-2@@ -107,11 +107,11 @@ static loff_t gfs2_llseek(struct file *f error =gfs2_glock_nq_init (ip->i_gl,LM_ST_SHARED ,LM_FLAG_ANY , &i_gh);if (!error) { - error =remote_llseek (file, offset, origin); + error =remote_llseek_unlocked (file, offset, origin);gfs2_glock_dq_uninit (&i_gh); } }else - error =remote_llseek (file, offset, origin); + error =remote_llseek_unlocked (file, offset, origin);return error; }Index:
linux/fs/ncpfs/file.c
===================================================================fs/ncpfs/file.c +11
-1@@ -18,6 +18,7 @@#include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/sched.h> +#include <linux/smp_lock.h> #include <linux/ncp_fs.h> #include "ncplib_kernel.h" @@ -281,9 +282,18 @@ static int ncp_release(struct inode *inoreturn 0 ; } +static loff_t ncp_remote_llseek (struct file *file,loff_t offset,int origin) +{ +loff_t ret; +lock_kernel (); + ret =remote_llseek_unlocked (file, offset, origin); +unlock_kernel (); +return ret; +} +const struct file_operations ncp_file_operations = { - .llseek = remote_llseek, + .llseek = ncp_remote_llseek, .read = ncp_file_read, .write = ncp_file_write, .ioctl = ncp_ioctl,Index:
linux/fs/read_write.c
===================================================================fs/read_write.c +3
-4@@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *EXPORT_SYMBOL (generic_file_llseek); -loff_t remote_llseek (struct file *file,loff_t offset,int origin) +loff_t remote_llseek_unlocked (struct file *file,loff_t offset,int origin) {long long retval; -lock_kernel ();switch (origin) {case SEEK_END : offset +=i_size_read (file->f_path.dentry->d_inode);@@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file, retval = -EINVAL ;if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {if (offset != file->f_pos) { +/* AK: do we need a lock for those? */ file->f_pos = offset; file->f_version =0 ; } retval = offset; } -unlock_kernel ();return retval; } -EXPORT_SYMBOL (remote_llseek); +EXPORT_SYMBOL (remote_llseek_unlocked);loff_t no_llseek (struct file *file,loff_t offset,int origin) {Index:
linux/fs/smbfs/file.c
===================================================================fs/smbfs/file.c +10
-1@@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode,return error; } +static loff_t smb_remote_llseek (struct file *file,loff_t offset,int origin) +{ +loff_t ret; +lock_kernel (); + ret =remote_llseek_unlocked (file, offset, origin); +unlock_kernel (); +return ret; +} +const struct file_operations smb_file_operations = { - .llseek = remote_llseek, + .llseek = smb_remote_llseek, .read = do_sync_read, .aio_read = smb_file_aio_read, .write = do_sync_write,Index:
linux/include/linux/fs.h
===================================================================include/linux/fs.h +2
-1@@ -1825,7 +1825,8 @@ extern void file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping);extern loff_t no_llseek (struct file *file,loff_t offset,int origin);extern loff_t generic_file_llseek (struct file *file,loff_t offset,int origin); -extern loff_t remote_llseek (struct file *file,loff_t offset,int origin); +extern loff_t remote_llseek_unlocked (struct file *file,loff_t offset, +int origin);extern int generic_file_open (struct inode * inode,struct file * filp);extern int nonseekable_open (struct inode * inode,struct file * filp);Index:
linux/fs/nfs/file.c
===================================================================fs/nfs/file.c +5
-1@@ -166,6 +166,7 @@ force_reval:static loff_t nfs_file_llseek (struct file *filp,loff_t offset,int origin) { +loff_t loff;/* origin == SEEK_END => we must revalidate the cached file length */ if (origin ==SEEK_END ) {struct inode *inode = filp->f_mapping->host;@@ -173,7 +174,10 @@ static loff_t nfs_file_llseek(struct filif (retval <0 )return (loff_t )retval; } -return remote_llseek (filp, offset, origin); +lock_kernel ();/* BKL needed? */ + loff =remote_llseek_unlocked (filp, offset, origin); +unlock_kernel (); +return loff; }/*
Signature · 3 lines
Thanks, Steve
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 5 lines · Steve French
Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from
f_posinfs/read_write.cinremote_llseekwith your patch since the reads/writes from that field are not necessarily atomic and threads could be racing in seek on the same file struct?
Where does is state in POSIX or SUS that we need to cater to that kind of application?
In any case, the current behaviour of f_pos if two threads are sharing the file struct is undefined no matter whether you spinlock or not, since there is no special locking around sys_read() or sys_write().
Trond
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
If two seeks overlap, can't you end up with an f_pos value that is different than what either thread seeked to? or if you have a seek and a read overlap can't you end up with the read occurring in the midst of an update of f_pos (which takes more than one instruction on various architectures), e.g. reading an f_pos, which has only the
lower half of a 64 bit field updated? I agree that you shouldn't
have seeks racing in parallel but I think it is preferable to get either the updated f_pos or the earlier f_pos not something 1/2 updated.
Quoted 16 lines · Trond Myklebust
Quoted 5 lines · Steve French
Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from
f_posinfs/read_write.cinremote_llseekwith your patch since the reads/writes from that field are not necessarily atomic and threads could be racing in seek on the same file struct?Where does is state in POSIX or SUS that we need to cater to that kind of application?
In any case, the current behaviour off_posif two threads are sharing the file struct is undefined no matter whether you spinlock or not, since there is no special locking aroundsys_read()orsys_write().Trond
Signature · 3 lines
Thanks, Steve
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 9 lines · Steve French
If two seeks overlap, can't you end up with an
f_posvalue that is different than what either thread seeked to? or if you have a seek and a read overlap can't you end up with the read occurring in the midst of an update off_pos(which takes more than one instruction on various architectures), e.g. reading anf_pos, which has only the
lower half of a 64 bit field updated? I agree that you shouldn't
have seeks racing in parallel but I think it is preferable to get either the updatedf_posor the earlierf_posnot something 1/2 updated.
Why? The threads are doing something inherently liable to corrupt data anyway. If they can race over the seek, why wouldn't they race over the read or write too?
The race in lseek() should probably be the least of your worries in this case.
Trond
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 12 lines · On Monday 28 January 2008 00:08:56 Trond Myklebust wrote:
Quoted 9 lines · Steve French
If two seeks overlap, can't you end up with an
f_posvalue that is different than what either thread seeked to? or if you have a seek and a read overlap can't you end up with the read occurring in the midst of an update off_pos(which takes more than one instruction on various architectures), e.g. reading anf_pos, which has only the
lower half of a 64 bit field updated? I agree that you shouldn't
have seeks racing in parallel but I think it is preferable to get either the updatedf_posor the earlierf_posnot something 1/2 updated.
Quoted 5 lines
Why? The threads are doing something inherently liable to corrupt data anyway. If they can race over the seek, why wouldn't they race over the read or write too?
The race inlseek()should probably be the least of your worries in this case.
The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt f_pos when it happens with two racing relative seeks or read/write()s
I would consider that a bug.
Fixes would be either to always take a spinlock to update this (nasty on platforms where spinlocks are expensive like P4) or define some architecture specific way to read/write 64bit values consistently. In theory also some lazy locking seqlock like mechanism could be used, but that had the disadvantage of being theoretically starvable.
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 6 lines · Andi Kleen
The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt
f_poswhen it happens with two racing relative seeks or read/write()sI would consider that a bug.
I disagree. The corruption occurs because this isn't a situation that is allowed by either POSIX or SUSv2/v3. Exactly what spec are you referring to here?
Trond
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 7 lines · On Monday 28 January 2008 05:13:09 Trond Myklebust wrote:
Quoted 5 lines · Andi Kleen
The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt
f_poswhen it happens with two racing relative seeks or read/write()s
Quoted 5 lines
Quoted 1 line
I would consider that a bug.
I disagree. The corruption occurs because this isn't a situation that is allowed by either POSIX or SUSv2/v3. Exactly what spec are you referring to here?
No specific spec, just general quality of implementation. We normally don't have non thread safe system calls even if it was in theory allowed by some specification.
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 8 lines · Andi Kleen
Quoted 7 lines · On Monday 28 January 2008 05:13:09 Trond Myklebust wrote:
Quoted 5 lines · Andi Kleen
The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt
f_poswhen it happens with two racing relative seeks or read/write()s
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
I would consider that a bug.
I disagree. The corruption occurs because this isn't a situation that is allowed by either POSIX or SUSv2/v3. Exactly what spec are you referring to here?
No specific spec, just general quality of implementation. We normally don't have non thread safe system calls even if it was in theory allowed by some specification.
We've had the existing implementation for quite some time. The arguments against changing it have been the same all along: if your application wants to share files between threads, the portability argument implies that you should either use pread/pwrite or use a mutex or some other form of synchronisation primitive in order to ensure that lseek()/read()/write() do not overlap.
Cheers
Trond
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
In reply to Andi KleenQuoted 8 lines · On Mon, 28 Jan 2008 05:38:25 +0100 Andi Kleen <[email protected]> wrote:
Quoted 7 lines · On Monday 28 January 2008 05:13:09 Trond Myklebust wrote:
Quoted 5 lines · Andi Kleen
The problem is that it's not a race in who gets to do its thing first, but a parallel reader can actually see a corrupted value from the two independent words on 32bit (e.g. during a 4GB). And this could actually completely corrupt
f_poswhen it happens with two racing relative seeks or read/write()s
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I would consider that a bug.
I disagree. The corruption occurs because this isn't a situation that is allowed by either POSIX or SUSv2/v3. Exactly what spec are you referring to here?
No specific spec, just general quality of implementation.
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A & 0xffffffff00000000) | (B & 0xffffffff))
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 4 lines
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A &
0xffffffff00000000) | (B &0xffffffff))
The problem is pretty nasty unfortunately. To solve it properly I think the file_operations->read/write prototypes would need to be changed because otherwise it is not possible to do atomic relative updates of f_pos. Right now the actual update is burrowed deeply in the low level read/write implementation. But that would be a huge impact all over the tree :/
Or maybe define a new read/write64 and keep the default as 32bit only-- i suppose most users don't really need 64bit. Still would be a nasty API change.
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 10 lines · Andi Kleen
Quoted 3 lines
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A &
0xffffffff00000000) | (B &0xffffffff))The problem is pretty nasty unfortunately. To solve it properly I think the file_operations->read/write prototypes would need to be changed because otherwise it is not possible to do atomic relative updates of
f_pos. Right now the actual update is burrowed deeply in the low level read/write implementation. But that would be a huge impact all over the tree :/
If there were a wrapper around reads and writes of f_pos as there is for i_size e.g. it would hit a lot of code, but not as many as I had originally thought. the most important ones are in the vfs itself, where
there are only 59 uses of the field (not all need to be changed). ext3
has fewer (25), and cifs only 12 uses.
Signature · 3 lines
Thanks, Steve
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 12 lines · Steve French
Quoted 10 lines · Andi Kleen
Quoted 3 lines
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A &
0xffffffff00000000) | (B &0xffffffff))The problem is pretty nasty unfortunately. To solve it properly I think the file_operations->read/write prototypes would need to be changed because otherwise it is not possible to do atomic relative updates of
f_pos. Right now the actual update is burrowed deeply in the low level read/write implementation. But that would be a huge impact all over the tree :/
Quoted 5 lines
If there were a wrapper around reads and writes of
f_posas there is fori_sizee.g. it would hit a lot of code, but not as many as I had originally thought. the most important ones are in the vfs itself, where
there are only 59 uses of the field (not all need to be changed). ext3
has fewer (25), and cifs only 12 uses.
Most of the uses in ext3 and cifs deal with a directory's f_pos in readdir, which is protected by i_mutex, so I don't think we need to worry about them at all.
Signature · 2 lines
David Kleikamp IBM Linux Technology Center
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
In reply to Andrew MortonQuoted 5 lines
Quoted 1 line
No specific spec, just general quality of implementation.
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A &
0xffffffff00000000) | (B &0xffffffff))
Agree entirely: the spec doesn't allow for random scribbling in the wrong place. It doesn't cover which goes first or who "wins" the race but provides pwrite/pread for that situation. Writing somewhere unrelated is definitely not to spec and not good.
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 10 lines · On Monday 28 January 2008 13:56:05 Alan Cox wrote:
Quoted 5 lines
Quoted 1 line
No specific spec, just general quality of implementation.
I completely agree. If one thread writes A and another writes B then the kernel should record either A or B, not ((A &
0xffffffff00000000) | (B &0xffffffff))Agree entirely: the spec doesn't allow for random scribbling in the wrong place. It doesn't cover which goes first or who "wins" the race but provides pwrite/pread for that situation. Writing somewhere unrelated is definitely not to spec
Actually it would probably -- i guess it's undefined and in undefined country such things can happen.
Also to be fair I think it's only a problem for the 4GB wrapping case which is presumably rare (otherwise we would have heard about it)
Also worse really fixing it would be a major change to the VFS because of the way ->read/write are defined :/
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 2 lines
Also worse really fixing it would be a major change to the VFS because of the way ->read/write are defined :/
I don't see a problem there. ->read and ->write update the passed pointer which is not the real f_pos anyway. Just the copies need fixing.
Alan
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 5 lines · On Monday 28 January 2008 14:38:57 Alan Cox wrote:
Quoted 2 lines
Also worse really fixing it would be a major change to the VFS because of the way ->read/write are defined :/
I don't see a problem there. ->read and ->write update the passed pointer which is not the real
f_posanyway. Just the copies need fixing.
They are effectually doing a decoupled read/modify/write cycle. e.g.:
A B
read fpos
read fpos
fpos += A fpos += B
write fpos
write fpos
So you get overlapping reads. Probably not good.
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
Quoted 15 lines · On Mon, 28 Jan 2008 15:10:34 +0100 Andi Kleen <[email protected]> wrote:
Quoted 5 lines · On Monday 28 January 2008 14:38:57 Alan Cox wrote:
Quoted 2 lines
Also worse really fixing it would be a major change to the VFS because of the way ->read/write are defined :/
I don't see a problem there. ->read and ->write update the passed pointer which is not the real
f_posanyway. Just the copies need fixing.They are effectually doing a decoupled read/modify/write cycle. e.g.:
A B
read fpos
read fpos
Quoted 7 lines
fpos += A fpos += B
write fposwrite fpos
So you get overlapping reads. Probably not good.
No unix system I'm aware of cares about the read/write positioning during parallel simultaneous reads or writes, with the exception of O_APPEND which is strictly defined. The problem case is getting fpos != either valid value.
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
In reply to Andi KleenQuoted 1 line · Andi Kleen
So you get overlapping reads. Probably not good.
This was discussed in the past i think ->
http://lkml.org/lkml/2006/4/13/124
http://lkml.org/lkml/2006/4/13/130
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
In reply to Steve FrenchQuoted 2 lines · On Sunday 27 January 2008 23:18:26 Steve French wrote:
If two seeks overlap, can't you end up with an
f_posvalue that is different than what either thread seeked to?
Yes you can on 32bit. Especially during the 4GB wrap
-Andi
Re: [PATCH] [8/18] BKL-removal: Remove BKL from remote_llseek
In reply to Steve FrenchQuoted 5 lines · On Sunday 27 January 2008 17:57:14 Steve French wrote:
Don't you need to a spinlock/spinunlock(i_lock) or something similar (there isn't a spinlock in the file struct unfortunately) around the reads and writes from
f_posinfs/read_write.cinremote_llseekwith your patch since the reads/writes from that field are not necessarily atomic and threads could be racing in seek on the same file struct?
Funny that you mention it. I actually noticed this too while working on this, but noticed that it is wrong everywhere (as in even plain sys_write/read gets it wrong). So I decided to not address it because it is already broken.
I did actually send email to a few people about this, but no answer yet.
I agree it's probably all broken on 32bit platforms, but I'm not sure how to best address this. When it is comprehensively addressed remote_llseek can use that new method too.
-Andi
PATCH 9/18 BKL-removal: Use unlocked_ioctl for jfs
In reply to Andi KleenConvert jfs_ioctl over to not use the BKL. The only potential race I could see was with two ioctls in parallel changing the flags and losing the updates. Use the i_mutex to protect against this.
--- fs/jfs/file.c | 2 +- fs/jfs/ioctl.c | 13 ++++++++++--- fs/jfs/jfs_inode.h | 3 +-- fs/jfs/namei.c | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-)
fs/jfs/ioctl.c +10 -3
@@ -51,9 +51,9 @@ static long jfs_map_ext2(unsigned long f } -int jfs_ioctl (struct inode * inode,struct file * filp,unsigned int cmd, -unsigned long arg) +long jfs_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) { +struct inode *inode = filp->f_dentry->d_inode;struct jfs_inode_info *jfs_inode =JFS_IP (inode);unsigned int flags;
@@ -82,6 +82,10 @@ int jfs_ioctl(struct inode * inode, stru/* Is it quota file? Do not allow user to mess with it */ if (IS_NOQUOTA (inode))return -EPERM ; + +/* Lock against other parallel changes of flags */ +mutex_lock (&inode->i_mutex); +jfs_get_inode_flags (jfs_inode); oldflags = jfs_inode->mode2;
@@ -92,8 +96,10 @@ int jfs_ioctl(struct inode * inode, struif ((oldflags &JFS_IMMUTABLE_FL ) || ((flags ^ oldflags) & (JFS_APPEND_FL |JFS_IMMUTABLE_FL ))) { -if (!capable (CAP_LINUX_IMMUTABLE )) +if (!capable (CAP_LINUX_IMMUTABLE )) { +mutex_unlock (&inode->i_mutex);return -EPERM ; + } } flags = flags &JFS_FL_USER_MODIFIABLE ;
@@ -101,6 +107,7 @@ int jfs_ioctl(struct inode * inode, stru jfs_inode->mode2 = flags;jfs_set_inode_flags (inode); +mutex_unlock (&inode->i_mutex); inode->i_ctime =CURRENT_TIME_SEC ;mark_inode_dirty (inode);return 0 ;
fs/jfs/file.c +1 -1
@@ -112,5 +112,5 @@ const struct file_operations jfs_file_op .splice_write = generic_file_splice_write, .fsync = jfs_fsync, .release = jfs_release, - .ioctl = jfs_ioctl, + .unlocked_ioctl = jfs_ioctl, };
fs/jfs/jfs_inode.h +1 -2
@@ -22,8 +22,7 @@ struct fid;extern struct inode *ialloc (struct inode *,umode_t );extern int jfs_fsync (struct file *,struct dentry *,int ); -extern int jfs_ioctl (struct inode *,struct file *, -unsigned int ,unsigned long ); +extern long jfs_ioctl (struct file *,unsigned int ,unsigned long );extern void jfs_read_inode (struct inode *);extern int jfs_commit_inode (struct inode *,int );extern int jfs_write_inode (struct inode *,int );
fs/jfs/namei.c +1 -1
@@ -1562,7 +1562,7 @@ const struct file_operations jfs_dir_ope .read = generic_read_dir, .readdir = jfs_readdir, .fsync = jfs_fsync, - .ioctl = jfs_ioctl, + .unlocked_ioctl = jfs_ioctl, };static int jfs_ci_hash (struct dentry *dir,struct qstr *this)
Re: [PATCH] [9/18] BKL-removal: Use unlocked_ioctl for jfs
Quoted 7 lines · Andi Kleen
Convert
jfs_ioctlover to not use the BKL. The only potential race I could see was with two ioctls in parallel changing the flags and losing the updates. Use thei_mutexto protect against this.Signed-off-by Andi Kleen <[email protected]>
Added to the jfs git tree.
Thanks,
Shaggy
Signature · 2 lines
David Kleikamp IBM Linux Technology Center
PATCH 10/18 BKL-removal: Implement a compat_ioctl handler for JFS
In reply to Andi KleenThe ioctls were already compatible except for the actual values so this was fairly easy to do.
--- fs/jfs/file.c | 3 +++ fs/jfs/ioctl.c | 18 ++++++++++++++++++ fs/jfs/jfs_dinode.h | 2 ++ fs/jfs/jfs_inode.h | 1 + fs/jfs/namei.c | 3 +++ 5 files changed, 27 insertions(+)
fs/jfs/file.c +3 -0
@@ -113,4 +113,7 @@ const struct file_operations jfs_file_op .fsync = jfs_fsync, .release = jfs_release, .unlocked_ioctl = jfs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = jfs_compat_ioctl, +#endif };
fs/jfs/ioctl.c +18 -0
@@ -117,3 +117,21 @@ long jfs_ioctl(struct file *filp, unsign } } +#ifdef CONFIG_COMPAT +long jfs_compat_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) +{ +/* While these ioctl numbers defined with 'long' and have different +* numbers than the 64bit ABI, +* the actual implementation only deals with ints and is compatible. +*/ +switch (cmd) { +case JFS_IOC_GETFLAGS32 : + cmd =JFS_IOC_GETFLAGS ; +break ; +case JFS_IOC_SETFLAGS32 : + cmd =JFS_IOC_SETFLAGS ; +break ; + } +return jfs_ioctl (filp, cmd, arg); +} +#endif
fs/jfs/jfs_inode.h +1 -0
@@ -23,6 +23,7 @@ struct fid;extern struct inode *ialloc (struct inode *,umode_t );extern int jfs_fsync (struct file *,struct dentry *,int );extern long jfs_ioctl (struct file *,unsigned int ,unsigned long ); +extern long jfs_compat_ioctl (struct file *,unsigned int ,unsigned long );extern void jfs_read_inode (struct inode *);extern int jfs_commit_inode (struct inode *,int );extern int jfs_write_inode (struct inode *,int );
fs/jfs/namei.c +3 -0
@@ -1563,6 +1563,9 @@ const struct file_operations jfs_dir_ope .readdir = jfs_readdir, .fsync = jfs_fsync, .unlocked_ioctl = jfs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = jfs_compat_ioctl, +#endif };static int jfs_ci_hash (struct dentry *dir,struct qstr *this)
fs/jfs/jfs_dinode.h +2 -0
@@ -170,5 +170,7 @@ struct dinode {#define JFS_IOC_GETFLAGS _IOR('f', 1, long)#define JFS_IOC_SETFLAGS _IOW('f', 2, long) +#define JFS_IOC_GETFLAGS32 _IOR('f', 1, int) +#define JFS_IOC_SETFLAGS32 _IOW('f', 2, int)#endif /*_H_JFS_DINODE */
Re: [PATCH] [10/18] BKL-removal: Implement a compat_ioctl handler for JFS
Quoted 6 lines · Andi Kleen
The ioctls were already compatible except for the actual values so this was fairly easy to do.
Signed-off-by Andi Kleen <[email protected]>
Added to the jfs git tree.
Thanks,
Shaggy
Signature · 2 lines
David Kleikamp IBM Linux Technology Center
PATCH 11/18 BKL-removal: Convert ocfs2 over to unlocked_ioctl
In reply to Andi KleenAs far as I can see there is nothing in ocfs2_ioctl that requires the BKL, so use unlocked_ioctl
--- fs/ocfs2/file.c | 4 ++-- fs/ocfs2/ioctl.c | 12 +++--------- fs/ocfs2/ioctl.h | 3 +-- 3 files changed, 6 insertions(+), 13 deletions(-)
fs/ocfs2/ioctl.c +3 -9
@@ -111,9 +111,9 @@ bail:return status; } -int ocfs2_ioctl (struct inode * inode,struct file * filp, -unsigned int cmd,unsigned long arg) +long ocfs2_ioctl (struct file *filp,unsigned int cmd,unsigned long arg) { +struct inode *inode = filp->f_path.dentry->d_inode;unsigned int flags;int status;struct ocfs2_space_resv sr;
@@ -148,9 +148,6 @@ int ocfs2_ioctl(struct inode * inode, st#ifdef CONFIG_COMPAT long ocfs2_compat_ioctl (struct file *file,unsigned cmd,unsigned long arg) { -struct inode *inode = file->f_path.dentry->d_inode; -int ret; -switch (cmd) {case OCFS2_IOC32_GETFLAGS : cmd =OCFS2_IOC_GETFLAGS ;
@@ -167,9 +164,6 @@ long ocfs2_compat_ioctl(struct file *fil return -ENOIOCTLCMD; } -lock_kernel (); - ret =ocfs2_ioctl (inode, file, cmd, arg); -unlock_kernel (); -return ret; +return ocfs2_ioctl (file, cmd, arg); }#endif
fs/ocfs2/ioctl.h +1 -2
@@ -10,8 +10,7 @@#ifndef OCFS2_IOCTL_H #define OCFS2_IOCTL_H -int ocfs2_ioctl (struct inode * inode,struct file * filp, -unsigned int cmd,unsigned long arg); +long ocfs2_ioctl (struct file *filp,unsigned int cmd,unsigned long arg);long ocfs2_compat_ioctl (struct file *file,unsigned cmd,unsigned long arg);#endif /* OCFS2_IOCTL_H */
fs/ocfs2/file.c +2 -2
@@ -2212,7 +2212,7 @@ const struct file_operations ocfs2_fops .open = ocfs2_file_open, .aio_read = ocfs2_file_aio_read, .aio_write = ocfs2_file_aio_write, - .ioctl = ocfs2_ioctl, + .unlocked_ioctl = ocfs2_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ocfs2_compat_ioctl,#endif
@@ -2224,7 +2224,7 @@ const struct file_operations ocfs2_dops .read = generic_read_dir, .readdir = ocfs2_readdir, .fsync = ocfs2_sync_file, - .ioctl = ocfs2_ioctl, + .unlocked_ioctl = ocfs2_ioctl,#ifdef CONFIG_COMPAT .compat_ioctl = ocfs2_compat_ioctl,#endif
PATCH 11/18 BKL-removal: Convert ocfs2 over to unlocked_ioctl II
Quoted 2 lines · Andi Kleen
As far as I can see there is nothing in
ocfs2_ioctlthat requires the BKL, so useunlocked_ioctl
Mark, can you please ack/nack this patch and ideally take it into your tree? You're the only file system maintainer who hasn't acknowledged yet.
Thanks,
-Andi
[Ocfs2-devel] Re: [PATCH] [11/18] BKL-removal: Convert ocfs2 over to unlocked_ioctl II
Quoted 2 lines · Andi Kleen
Mark, can you please ack/nack this patch and ideally take it into your tree?
ACK.
I'll gladly take it into Ocfs2.git. It doesn't merge cleanly right now, but I think I can handle that tommorrow.
--Mark
--
Mark Fasheh
Principal Software Developer, Oracle
[email protected]
PATCH 12/18 BKL-removal: Convert CIFS over to unlocked_ioctl
In reply to Andi Kleencifs_ioctl doesn't seem to need the BKL for anything, so convert it over to use unlocked_ioctl.
--- fs/cifs/cifsfs.c | 10 +++++----- fs/cifs/cifsfs.h | 4 ++-- fs/cifs/ioctl.c | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-)
fs/cifs/cifsfs.c +5 -5
@@ -625,7 +625,7 @@ const struct file_operations cifs_file_o .splice_read = generic_file_splice_read, .llseek = cifs_llseek,#ifdef CONFIG_CIFS_POSIX - .ioctl = cifs_ioctl, + .unlocked_ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */ #ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -645,7 +645,7 @@ const struct file_operations cifs_file_d .flush = cifs_flush, .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX - .ioctl = cifs_ioctl, + .unlocked_ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */ .llseek = cifs_llseek,#ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -665,7 +665,7 @@ const struct file_operations cifs_file_n .splice_read = generic_file_splice_read, .llseek = cifs_llseek,#ifdef CONFIG_CIFS_POSIX - .ioctl = cifs_ioctl, + .unlocked_ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */ #ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -684,7 +684,7 @@ const struct file_operations cifs_file_d .flush = cifs_flush, .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX - .ioctl = cifs_ioctl, + .unlocked_ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */ .llseek = cifs_llseek,#ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -699,7 +699,7 @@ const struct file_operations cifs_dir_op#ifdef CONFIG_CIFS_EXPERIMENTAL .dir_notify = cifs_dir_notify,#endif /* CONFIG_CIFS_EXPERIMENTAL */ - .ioctl = cifs_ioctl, + .unlocked_ioctl = cifs_ioctl, }; static void
fs/cifs/cifsfs.h +2 -2
@@ -99,8 +99,8 @@ extern int cifs_setxattr(struct dentry size_t, int);extern ssize_t cifs_getxattr (struct dentry *,const char *,void *,size_t );extern ssize_t cifs_listxattr (struct dentry *,char *,size_t ); -extern int cifs_ioctl (struct inode *inode,struct file *filep, -unsigned int command,unsigned long arg); +extern long cifs_ioctl (struct file *filep,unsigned int command, +unsigned long arg);#ifdef CONFIG_CIFS_EXPERIMENTAL extern const struct export_operations cifs_export_ops;
fs/cifs/ioctl.c +2 -2
@@ -30,9 +30,9 @@#define CIFS_IOC_CHECKUMOUNT _IO(0xCF, 2) -int cifs_ioctl (struct inode *inode,struct file *filep, -unsigned int command,unsigned long arg) +long cifs_ioctl (struct file *filep,unsigned int command,unsigned long arg) { +struct inode *inode = filep->f_dentry->d_inode;int rc = -ENOTTY ;/* strange error - but the precedent */ int xid;struct cifs_sb_info *cifs_sb;
PATCH 13/18 BKL-removal: Add compat_ioctl for cifs
In reply to Andi KleenSimilar to the compat handlers of other file systems. The ioctls are compatible except that they have different numbers.
--- fs/cifs/cifsfs.c | 15 +++++++++++++++ fs/cifs/cifsfs.h | 2 ++ fs/cifs/ioctl.c | 19 +++++++++++++++++++ 3 files changed, 36 insertions(+)
fs/cifs/cifsfs.c +15 -0
@@ -626,6 +626,9 @@ const struct file_operations cifs_file_o .llseek = cifs_llseek,#ifdef CONFIG_CIFS_POSIX .unlocked_ioctl = cifs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = cifs_compat_ioctl, +#endif #endif /* CONFIG_CIFS_POSIX */ #ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -646,6 +649,9 @@ const struct file_operations cifs_file_d .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX .unlocked_ioctl = cifs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = cifs_compat_ioctl, +#endif #endif /* CONFIG_CIFS_POSIX */ .llseek = cifs_llseek,#ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -666,6 +672,9 @@ const struct file_operations cifs_file_n .llseek = cifs_llseek,#ifdef CONFIG_CIFS_POSIX .unlocked_ioctl = cifs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = cifs_compat_ioctl, +#endif #endif /* CONFIG_CIFS_POSIX */ #ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -685,6 +694,9 @@ const struct file_operations cifs_file_d .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX .unlocked_ioctl = cifs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = cifs_compat_ioctl, +#endif #endif /* CONFIG_CIFS_POSIX */ .llseek = cifs_llseek,#ifdef CONFIG_CIFS_EXPERIMENTAL
@@ -700,6 +712,9 @@ const struct file_operations cifs_dir_op .dir_notify = cifs_dir_notify,#endif /* CONFIG_CIFS_EXPERIMENTAL */ .unlocked_ioctl = cifs_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = cifs_compat_ioctl, +#endif }; static void
fs/cifs/cifsfs.h +2 -0
@@ -101,6 +101,8 @@ extern ssize_t cifs_getxattr(struct dentextern ssize_t cifs_listxattr (struct dentry *,char *,size_t );extern long cifs_ioctl (struct file *filep,unsigned int command,unsigned long arg); +extern long cifs_compat_ioctl (struct file *filep,unsigned int command, +unsigned long arg);#ifdef CONFIG_CIFS_EXPERIMENTAL extern const struct export_operations cifs_export_ops;
fs/cifs/ioctl.c +19 -0
@@ -108,3 +108,22 @@ long cifs_ioctl(struct file *filep, unsiFreeXid (xid);return rc; } + +#ifdef CONFIG_COMPAT +#define FS_IOC_GETFLAGS32 _IOR('f', 1, int) +#define FS_IOC_SETFLAGS32 _IOR('f', 2, int) + +long cifs_compat_ioctl (struct file *f,unsigned int command,unsigned long arg) +{ +/* Native ioctl is just mistyped, the real type is always int */ +switch (command) { +case FS_IOC_GETFLAGS32 : + command =FS_IOC_GETFLAGS ; +break ; +case FS_IOC_SETFLAGS32 : + command =FS_IOC_SETFLAGS ; +break ; + } +return cifs_ioctl (f, command, arg); +} +#endif
PATCH 14/18 BKL-removal: Add unlocked_fasync
In reply to Andi KleenAdd a new fops entry point to allow fasync without BKL. While it's arguably unclear this entry point is called often enough for it really matters it was still relatively easy to do. And there are far less async users in the tree than ioctls so it's likely they can be all converted eventually and then the non unlocked async entry point could be dropped.
--- Documentation/filesystems/vfs.txt | 5 ++++- fs/fcntl.c | 6 +++++- fs/ioctl.c | 5 ++++- include/linux/fs.h | 1 + 4 files changed, 14 insertions(+), 3 deletions(-)
fs/fcntl.c +5 -1
@@ -240,11 +240,15 @@ static int setfl(int fd, struct file * flock_kernel ();if ((arg ^ filp->f_flags) &FASYNC ) { -if (filp->f_op && filp->f_op->fasync) { +if (filp->f_op && filp->f_op->unlocked_fasync) + error = filp->f_op->unlocked_fasync (fd, filp, + !!(arg &FASYNC )); +else if (filp->f_op && filp->f_op->fasync) { error = filp->f_op->fasync (fd, filp, (arg &FASYNC ) !=0 );if (error <0 ) goto out; } +/* AK: no else error = -EINVAL here? */ } filp->f_flags = (arg &SETFL_MASK ) | (filp->f_flags & ~SETFL_MASK );
fs/ioctl.c +4 -1
@@ -118,7 +118,10 @@ int vfs_ioctl(struct file *filp, unsigne/* Did FASYNC state change ? */ if ((flag ^ filp->f_flags) &FASYNC ) { -if (filp->f_op && filp->f_op->fasync) { +if (filp->f_op && filp->f_op->unlocked_fasync) + error = filp->f_op->unlocked_fasync (fd, + filp, on); +else if (filp->f_op && filp->f_op->fasync) {lock_kernel (); error = filp->f_op->fasync (fd, filp, on);unlock_kernel ();
include/linux/fs.h +1 -0
@@ -1179,6 +1179,7 @@ struct file_operations {int (*fsync) (struct file *,struct dentry *,int datasync);int (*aio_fsync) (struct kiocb *,int datasync);int (*fasync) (int ,struct file *,int ); +int (*unlocked_fasync) (int ,struct file *,int );int (*lock) (struct file *,int ,struct file_lock *);ssize_t (*sendpage) (struct file *,struct page *,int ,size_t ,loff_t *,int );unsigned long (*get_unmapped_area)(struct file *,unsigned long ,unsigned long ,unsigned long ,unsigned long );
Documentation/filesystems/vfs.txt +4 -1
@@ -769,6 +769,7 @@ struct file_operations { int (*fsync) (struct file *, struct dentry *, int datasync); int (*aio_fsync) (struct kiocb *, int datasync); int (*fasync) (int, struct file *, int); + int (*unlocked_fasync) (int, struct file *, int); int (*lock) (struct file *, int, struct file_lock *); ssize_t (*readv) (struct file *, const struct iovec *, unsigned long, loff_t *); ssize_t (*writev) (struct file *, const struct iovec *, unsigned long, loff_t *);
@@ -828,7 +829,9 @@ otherwise noted. fsync: called by the fsync(2) system call fasync: called by the fcntl(2) system call when asynchronous - (non-blocking) mode is enabled for a file + (non-blocking) mode is enabled for a file. BKL hold + + unlocked_fasync: like fasync, but without BKL lock: called by the fcntl(2) system call for F_GETLK, F_SETLK, and F_SETLKW commands
Re: [PATCH] [14/18] BKL-removal: Add unlocked_fasync
Quoted 5 lines
Add a new fops entry point to allow fasync without BKL. While it's arguably unclear this entry point is called often enough for it really matters it was still relatively easy to do. And there are far less async users in the tree than ioctls so it's likely they can be all converted eventually and then the non unlocked async entry point could be dropped.
Excellent!
I like this patch :)
PATCH 15/18 BKL-removal: Convert pipe over to unlocked_fasync
In reply to Andi Kleen--- fs/pipe.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
fs/pipe.c +6 -6
@@ -788,7 +788,7 @@ const struct file_operations read_fifo_f .ioctl = pipe_ioctl, .open = pipe_read_open, .release = pipe_read_release, - .fasync = pipe_read_fasync, + .unlocked_fasync = pipe_read_fasync, };const struct file_operations write_fifo_fops = {
@@ -800,7 +800,7 @@ const struct file_operations write_fifo_ .ioctl = pipe_ioctl, .open = pipe_write_open, .release = pipe_write_release, - .fasync = pipe_write_fasync, + .unlocked_fasync = pipe_write_fasync, };const struct file_operations rdwr_fifo_fops = {
@@ -813,7 +813,7 @@ const struct file_operations rdwr_fifo_f .ioctl = pipe_ioctl, .open = pipe_rdwr_open, .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync, + .unlocked_fasync = pipe_rdwr_fasync, };static const struct file_operations read_pipe_fops = {
@@ -825,7 +825,7 @@ static const struct file_operations read .ioctl = pipe_ioctl, .open = pipe_read_open, .release = pipe_read_release, - .fasync = pipe_read_fasync, + .unlocked_fasync = pipe_read_fasync, };static const struct file_operations write_pipe_fops = {
@@ -837,7 +837,7 @@ static const struct file_operations writ .ioctl = pipe_ioctl, .open = pipe_write_open, .release = pipe_write_release, - .fasync = pipe_write_fasync, + .unlocked_fasync = pipe_write_fasync, };static const struct file_operations rdwr_pipe_fops = {
@@ -850,7 +850,7 @@ static const struct file_operations rdwr .ioctl = pipe_ioctl, .open = pipe_rdwr_open, .release = pipe_rdwr_release, - .fasync = pipe_rdwr_fasync, + .unlocked_fasync = pipe_rdwr_fasync, };struct pipe_inode_info *alloc_pipe_info (struct inode *inode)
PATCH 16/18 BKL-removal: Convert socket fasync to unlocked_fasync
In reply to Andi Kleen--- net/socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/socket.c +1 -1
@@ -131,7 +131,7 @@ static const struct file_operations sock .mmap = sock_mmap, .open = sock_no_open, /* special open code to disallow open via /proc */ .release = sock_close, - .fasync = sock_fasync, + .unlocked_fasync = sock_fasync, .sendpage = sock_sendpage, .splice_write = generic_splice_sendpage, };
PATCH 17/18 BKL-removal: Convert fuse to unlocked_fasync
In reply to Andi Kleen--- fs/fuse/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/fuse/dev.c +1 -1
@@ -1040,7 +1040,7 @@ const struct file_operations fuse_dev_op .aio_write = fuse_dev_write, .poll = fuse_dev_poll, .release = fuse_dev_release, - .fasync = fuse_dev_fasync, + .unlocked_fasync = fuse_dev_fasync, };static struct miscdevice fuse_miscdevice = {
PATCH 18/18 BKL-removal: Convert bad_inode to unlocked_fasync
In reply to Andi KleenNot that it matters much, but it was easy.
--- fs/bad_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/bad_inode.c +1 -1
@@ -174,7 +174,7 @@ static const struct file_operations bad_ .release = bad_file_release, .fsync = bad_file_fsync, .aio_fsync = bad_file_aio_fsync, - .fasync = bad_file_fasync, + .unlocked_fasync = bad_file_fasync, .lock = bad_file_lock, .sendpage = bad_file_sendpage, .get_unmapped_area = bad_file_get_unmapped_area,
Quoted 14 lines · Andi Kleen
[Andrew: I believe this is -mm material for .25]
- Convert some more file systems (generally those who don't use the BKL for anything except mount) to use
unlocked_bkl.
- Implement BKL less fasync (see patch for the rationale) This is currently a separate entry point, but since the number of fasync users in the tree is relatively small I hope the older entry point can be removed then in the not too far future
[help from other people converting more fasync users tounlocked_fasyncwould be appreciated]
- Implement BKL lessremote_llseek
- While I was at it I also added a few missing compat ioctl handlers
- Fix a few comments
Quoted 5 lines
This fixes a lot of relatively trivial BKL users in
fs/*. The main remaining non legacy offenders are nowlocks.c, nfs/nfsd and reiserfs. I believe BKL removal for all of those is being worked on by other people. Also a lot of "legacy" file systems still use it, but converting those does not seem to be very pressing.
BTW. here is a patch I did a while back for minix. I know it isn't a big deal, but the work is done so I guess I should send it along.
The minix filesystem uses bkl to protect access to metadata. Switch to a per-superblock mutex.
fs/minix/bitmap.c +11 -11
@@ -69,11 +69,11 @@ void minix_free_block(struct inode *inodreturn ; } bh = sbi->s_zmap[zone]; -lock_kernel (); +mutex_lock (&sbi->s_mutex);if (!minix_test_and_clear_bit (bit, bh->b_data))printk ("minix_free_block (%s:%lu): bit already cleared\n" , sb->s_id, block); -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);mark_buffer_dirty (bh);return ; }
@@ -88,18 +88,18 @@ int minix_new_block(struct inode * inodestruct buffer_head *bh = sbi->s_zmap[i];int j; -lock_kernel (); +mutex_lock (&sbi->s_mutex); j =minix_find_first_zero_bit (bh->b_data, bits_per_zone);if (j < bits_per_zone) {minix_set_bit (j, bh->b_data); -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);mark_buffer_dirty (bh); j += i * bits_per_zone + sbi->s_firstdatazone-1 ;if (j < sbi->s_firstdatazone || j >= sbi->s_nzones)break ;return j; } -unlock_kernel (); +mutex_unlock (&sbi->s_mutex); }return 0 ; }
@@ -211,10 +211,10 @@ void minix_free_inode(struct inode * inominix_clear_inode (inode);/* clear on-disk copy */ bh = sbi->s_imap[ino]; -lock_kernel (); +mutex_lock (&sbi->s_mutex);if (!minix_test_and_clear_bit (bit, bh->b_data))printk ("minix_free_inode: bit %lu already cleared\n" , bit); -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);mark_buffer_dirty (bh); out:clear_inode (inode);/* clear in-memory copy */
@@ -237,7 +237,7 @@ struct inode * minix_new_inode(const str j = bits_per_zone; bh =NULL ; *error = -ENOSPC ; -lock_kernel (); +mutex_lock (&sbi->s_mutex);for (i =0 ; i < sbi->s_imap_blocks; i++) { bh = sbi->s_imap[i]; j =minix_find_first_zero_bit (bh->b_data, bits_per_zone);
@@ -245,17 +245,17 @@ struct inode * minix_new_inode(const strbreak ; }if (!bh || j >= bits_per_zone) { -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);iput (inode);return NULL ; }if (minix_test_and_set_bit (j, bh->b_data)) {/* shouldn't happen */ -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);printk ("minix_new_inode: bit already set\n" );iput (inode);return NULL ; } -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);mark_buffer_dirty (bh); j += i * bits_per_zone;if (!j || j > sbi->s_ninodes) {
fs/minix/dir.c +2 -2
@@ -102,7 +102,7 @@ static int minix_readdir(struct file * fchar *name;__u32 inumber; -lock_kernel (); +mutex_lock (&sbi->s_mutex); pos = (pos + chunk_size-1 ) & ~(chunk_size-1 );if (pos >= inode->i_size)
@@ -146,7 +146,7 @@ static int minix_readdir(struct file * f done: filp->f_pos = (n <<PAGE_CACHE_SHIFT ) | offset; -unlock_kernel (); +mutex_unlock (&sbi->s_mutex);return 0 ; }
fs/minix/inode.c +1 -0
@@ -174,6 +174,7 @@ static int minix_fill_super(struct super sbi->s_firstdatazone = ms->s_firstdatazone; sbi->s_log_zone_size = ms->s_log_zone_size; sbi->s_max_size = ms->s_max_size; +mutex_init (&sbi->s_mutex); s->s_magic = ms->s_magic;if (s->s_magic ==MINIX_SUPER_MAGIC ) { sbi->s_version =MINIX_V1 ;
fs/minix/minix.h +3 -0
@@ -1,6 +1,7 @@#include <linux/fs.h> #include <linux/pagemap.h> #include <linux/minix_fs.h> +#include <linux/mutex.h> /* * change the define below to 0 if you want names > info->s_namelen chars to be
@@ -43,6 +44,8 @@ struct minix_sb_info {struct minix_super_block * s_ms;unsigned short s_mount_state;unsigned short s_version; + +struct mutex s_mutex; };extern struct inode *minix_iget (struct super_block *,unsigned long );
Quoted 3 lines
BTW. here is a patch I did a while back for minix. I know it isn't a big deal, but the work is done so I guess I should send it along.
Looks safe, although I'm surprised it actually gets around with such little locking in general.
-Andi