[PATCH 0/6] BKL ioctl pushdown

[PATCH 0/6] BKL ioctl pushdown

[John Kacur]

These patches convert ioctls to unlocked_ioctls by pushing down the bkl into
them. I had to go outside of the files that we agreed on, because logically the
ioctls in each kind of file system belong together.

Frederic and Arnd, you can pull from:
git.kernel.org//pub/scm/linux/kernel/git/jkacur/jk-2.6.git linus-bkl-pushdown

John Kacur (6):
  coda: BKL ioctl pushdown
  coda: Clean-up whitespace problems in pioctl.c
  fat: BKL ioctl pushdown
  ncpfs: BKL ioctl pushdown
  smbfs: BKL ioctl pushdown
  udf: BKL ioctl pushdown

 fs/coda/pioctl.c       |   76 ++++++++++++++++++++++++++----------------------
 fs/fat/dir.c           |   36 +++++++++++++++-------
 fs/fat/fat.h           |    3 +-
 fs/fat/file.c          |   22 ++++++++++----
 fs/ncpfs/dir.c         |    2 +-
 fs/ncpfs/file.c        |    2 +-
 fs/ncpfs/ioctl.c       |   27 ++++++++++-------
 fs/smbfs/dir.c         |    2 +-
 fs/smbfs/file.c        |    2 +-
 fs/smbfs/ioctl.c       |   10 ++++--
 fs/smbfs/proto.h       |    2 +-
 fs/udf/dir.c           |    2 +-
 fs/udf/file.c          |   43 +++++++++++++++++----------
 fs/udf/udfdecl.h       |    3 +-
 include/linux/ncp_fs.h |    2 +-
 15 files changed, 140 insertions(+), 94 deletions(-)

[PATCH 1/6] coda: BKL ioctl pushdown

[John Kacur]

Convert coda_pioctl to an unlocked_ioctl pushing down the BKL into it.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 fs/coda/pioctl.c |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/fs/coda/pioctl.c b/fs/coda/pioctl.c
index 773f2ce..96ace2c 100644
--- a/fs/coda/pioctl.c
+++ b/fs/coda/pioctl.c
@@ -23,10 +23,12 @@
 #include <linux/coda_fs_i.h>
 #include <linux/coda_psdev.h>
 
+#include <linux/smp_lock.h>
+
 /* pioctl ops */
 static int coda_ioctl_permission(struct inode *inode, int mask);
-static int coda_pioctl(struct inode * inode, struct file * filp, 
-                       unsigned int cmd, unsigned long user_data);
+static long coda_pioctl(struct file *filp, unsigned int cmd,
+			unsigned long user_data);
 
 /* exported from this file */
 const struct inode_operations coda_ioctl_inode_operations =
@@ -37,7 +39,7 @@ const struct inode_operations coda_ioctl_inode_operations =
 
 const struct file_operations coda_ioctl_operations = {
 	.owner		= THIS_MODULE,
-	.ioctl		= coda_pioctl,
+	.unlocked_ioctl	= coda_pioctl,
 };
 
 /* the coda pioctl inode ops */
@@ -46,40 +48,43 @@ static int coda_ioctl_permission(struct inode *inode, int mask)
 	return (mask & MAY_EXEC) ? -EACCES : 0;
 }
 
-static int coda_pioctl(struct inode * inode, struct file * filp, 
-                       unsigned int cmd, unsigned long user_data)
+static long coda_pioctl(struct file *filp, unsigned int cmd,
+			unsigned long user_data)
 {
 	struct path path;
         int error;
 	struct PioctlData data;
+	struct inode *inode = filp->f_dentry->d_inode;
         struct inode *target_inode = NULL;
         struct coda_inode_info *cnp;
 
+	lock_kernel();
+
         /* get the Pioctl data arguments from user space */
         if (copy_from_user(&data, (void __user *)user_data, sizeof(data))) {
-	    return -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
        
         /* 
          * Look up the pathname. Note that the pathname is in 
          * user memory, and namei takes care of this
          */
-        if (data.follow) {
+	if (data.follow)
                 error = user_path(data.path, &path);
-	} else {
+	else
 	        error = user_lpath(data.path, &path);
-	}
-		
-	if ( error ) {
-		return error;
-        } else {
+
+	if (error)
+		goto out;
+	else
 		target_inode = path.dentry->d_inode;
-	}
-	
+
 	/* return if it is not a Coda inode */
 	if ( target_inode->i_sb != inode->i_sb ) {
 		path_put(&path);
-	        return  -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
 
 	/* now proceed to make the upcall */
@@ -88,6 +93,8 @@ static int coda_pioctl(struct inode * inode, struct file * filp,
 	error = venus_pioctl(inode->i_sb, &(cnp->c_fid), cmd, &data);
 
 	path_put(&path);
+
+out:
+	unlock_kernel();
         return error;
 }
-
-- 
1.6.6.1

Re: [PATCH 1/6] coda: BKL ioctl pushdown

[Frederic Weisbecker]

On Wed, May 05, 2010 at 03:15:34PM +0200, John Kacur wrote:
> Convert coda_pioctl to an unlocked_ioctl pushing down the BKL into it.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---


Applied, thanks!

[PATCH 2/6] coda: Clean-up whitespace problems in pioctl.c

[John Kacur]

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 fs/coda/pioctl.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/coda/pioctl.c b/fs/coda/pioctl.c
index 96ace2c..ca25d96 100644
--- a/fs/coda/pioctl.c
+++ b/fs/coda/pioctl.c
@@ -1,6 +1,6 @@
 /*
  * Pioctl operations for Coda.
- * Original version: (C) 1996 Peter Braam 
+ * Original version: (C) 1996 Peter Braam
  * Rewritten for Linux 2.1: (C) 1997 Carnegie Mellon University
  *
  * Carnegie Mellon encourages users of this code to contribute improvements
@@ -31,8 +31,7 @@ static long coda_pioctl(struct file *filp, unsigned int cmd,
 			unsigned long user_data);
 
 /* exported from this file */
-const struct inode_operations coda_ioctl_inode_operations =
-{
+const struct inode_operations coda_ioctl_inode_operations = {
 	.permission	= coda_ioctl_permission,
 	.setattr	= coda_setattr,
 };
@@ -52,28 +51,28 @@ static long coda_pioctl(struct file *filp, unsigned int cmd,
 			unsigned long user_data)
 {
 	struct path path;
-        int error;
+	int error;
 	struct PioctlData data;
 	struct inode *inode = filp->f_dentry->d_inode;
-        struct inode *target_inode = NULL;
-        struct coda_inode_info *cnp;
+	struct inode *target_inode = NULL;
+	struct coda_inode_info *cnp;
 
 	lock_kernel();
 
-        /* get the Pioctl data arguments from user space */
-        if (copy_from_user(&data, (void __user *)user_data, sizeof(data))) {
+	/* get the Pioctl data arguments from user space */
+	if (copy_from_user(&data, (void __user *)user_data, sizeof(data))) {
 		error = -EINVAL;
 		goto out;
 	}
-       
-        /* 
-         * Look up the pathname. Note that the pathname is in 
-         * user memory, and namei takes care of this
-         */
+
+	/*
+	 * Look up the pathname. Note that the pathname is in
+	 * user memory, and namei takes care of this
+	 */
 	if (data.follow)
-                error = user_path(data.path, &path);
+		error = user_path(data.path, &path);
 	else
-	        error = user_lpath(data.path, &path);
+		error = user_lpath(data.path, &path);
 
 	if (error)
 		goto out;
@@ -81,14 +80,14 @@ static long coda_pioctl(struct file *filp, unsigned int cmd,
 		target_inode = path.dentry->d_inode;
 
 	/* return if it is not a Coda inode */
-	if ( target_inode->i_sb != inode->i_sb ) {
+	if (target_inode->i_sb != inode->i_sb) {
 		path_put(&path);
 		error = -EINVAL;
 		goto out;
 	}
 
 	/* now proceed to make the upcall */
-        cnp = ITOC(target_inode);
+	cnp = ITOC(target_inode);
 
 	error = venus_pioctl(inode->i_sb, &(cnp->c_fid), cmd, &data);
 
@@ -96,5 +95,5 @@ static long coda_pioctl(struct file *filp, unsigned int cmd,
 
 out:
 	unlock_kernel();
-        return error;
+	return error;
 }
-- 
1.6.6.1

Re: [PATCH 2/6] coda: Clean-up whitespace problems in pioctl.c

[Frederic Weisbecker]

On Wed, May 05, 2010 at 03:15:35PM +0200, John Kacur wrote:
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---


Applied too, thanks.

[PATCH 3/6] fat: BKL ioctl pushdown

[John Kacur]

Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
and push down the bkl into those functions.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 fs/fat/dir.c  |   36 ++++++++++++++++++++++++------------
 fs/fat/fat.h  |    3 +--
 fs/fat/file.c |   22 ++++++++++++++++------
 3 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 530b4ca..1b73e60 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -19,6 +19,7 @@
 #include <linux/buffer_head.h>
 #include <linux/compat.h>
 #include <asm/uaccess.h>
+#include <linux/smp_lock.h>
 #include "fat.h"
 
 /*
@@ -737,10 +738,11 @@ efault:									   \
 
 FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent)
 
-static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
+static long fat_ioctl_readdir(struct file *filp,
 			     void __user *dirent, filldir_t filldir,
 			     int short_only, int both)
 {
+	struct inode *inode = filp->f_dentry->d_inode;
 	struct fat_ioctl_filldir_callback buf;
 	int ret;
 
@@ -758,9 +760,10 @@ static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
 	return ret;
 }
 
-static int fat_dir_ioctl(struct inode *inode, struct file *filp,
-			 unsigned int cmd, unsigned long arg)
+static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
+			unsigned long arg)
 {
+	long error;
 	struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
 	int short_only, both;
 
@@ -774,21 +777,31 @@ static int fat_dir_ioctl(struct inode *inode, struct file *filp,
 		both = 1;
 		break;
 	default:
-		return fat_generic_ioctl(inode, filp, cmd, arg);
+		lock_kernel();
+		error = fat_generic_ioctl(filp, cmd, arg);
+		goto out;
 	}
 
-	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
-		return -EFAULT;
+	lock_kernel();
+	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2]))) {
+		error = -EFAULT;
+		goto out;
+	}
 	/*
 	 * Yes, we don't need this put_user() absolutely. However old
 	 * code didn't return the right value. So, app use this value,
 	 * in order to check whether it is EOF.
 	 */
-	if (put_user(0, &d1->d_reclen))
-		return -EFAULT;
+	if (put_user(0, &d1->d_reclen)) {
+		error = -EFAULT;
+		goto out;
+	}
 
-	return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir,
+	error = fat_ioctl_readdir(filp, d1, fat_ioctl_filldir,
 				 short_only, both);
+out:
+	unlock_kernel();
+	return error;
 }
 
 #ifdef CONFIG_COMPAT
@@ -800,7 +813,6 @@ FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent)
 static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
 				 unsigned long arg)
 {
-	struct inode *inode = filp->f_path.dentry->d_inode;
 	struct compat_dirent __user *d1 = compat_ptr(arg);
 	int short_only, both;
 
@@ -827,7 +839,7 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
 	if (put_user(0, &d1->d_reclen))
 		return -EFAULT;
 
-	return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir,
+	return fat_ioctl_readdir(filp, d1, fat_compat_ioctl_filldir,
 				 short_only, both);
 }
 #endif /* CONFIG_COMPAT */
@@ -836,7 +848,7 @@ const struct file_operations fat_dir_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= fat_readdir,
-	.ioctl		= fat_dir_ioctl,
+	.unlocked_ioctl	= fat_dir_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= fat_compat_dir_ioctl,
 #endif
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index e6efdfa..23f9b2a 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -298,8 +298,7 @@ extern int fat_free_clusters(struct inode *inode, int cluster);
 extern int fat_count_free_clusters(struct super_block *sb);
 
 /* fat/file.c */
-extern int fat_generic_ioctl(struct inode *inode, struct file *filp,
-			     unsigned int cmd, unsigned long arg);
+extern long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 extern const struct file_operations fat_file_operations;
 extern const struct inode_operations fat_file_inode_operations;
 extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index e8c159d..4f3044f 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -16,6 +16,7 @@
 #include <linux/blkdev.h>
 #include <linux/fsnotify.h>
 #include <linux/security.h>
+#include <linux/smp_lock.h>
 #include "fat.h"
 
 static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
@@ -114,19 +115,28 @@ out:
 	return err;
 }
 
-int fat_generic_ioctl(struct inode *inode, struct file *filp,
-		      unsigned int cmd, unsigned long arg)
+long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	long error;
+	struct inode *inode = filp->f_dentry->d_inode;
 	u32 __user *user_attr = (u32 __user *)arg;
 
+	lock_kernel();
+
 	switch (cmd) {
 	case FAT_IOCTL_GET_ATTRIBUTES:
-		return fat_ioctl_get_attributes(inode, user_attr);
+		error = fat_ioctl_get_attributes(inode, user_attr);
+		break;
 	case FAT_IOCTL_SET_ATTRIBUTES:
-		return fat_ioctl_set_attributes(filp, user_attr);
+		error = fat_ioctl_set_attributes(filp, user_attr);
+		break;
 	default:
-		return -ENOTTY;	/* Inappropriate ioctl for device */
+		error = -ENOTTY;	/* Inappropriate ioctl for device */
+		break;
 	}
+
+	unlock_kernel();
+	return error;
 }
 
 static int fat_file_release(struct inode *inode, struct file *filp)
@@ -159,7 +169,7 @@ const struct file_operations fat_file_operations = {
 	.aio_write	= generic_file_aio_write,
 	.mmap		= generic_file_mmap,
 	.release	= fat_file_release,
-	.ioctl		= fat_generic_ioctl,
+	.unlocked_ioctl = fat_generic_ioctl,
 	.fsync		= fat_file_fsync,
 	.splice_read	= generic_file_splice_read,
 };
-- 
1.6.6.1

Re: [PATCH 3/6] fat: BKL ioctl pushdown

[OGAWA Hirofumi]

John Kacur <jkacur@redhat.com> writes:

> Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
> and push down the bkl into those functions.

I guess this is the part of batch ioctl conversion stuff though, those
ioctl of FAT don't need BKL at all. Because all of those should already
be protected by inode->i_mutex.

Removing BKL and then cleanup after this patch would be almost same with
reverting this patch. So, could you just convert to unlocked_ioctl
instead?

Thanks.

> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  fs/fat/dir.c  |   36 ++++++++++++++++++++++++------------
>  fs/fat/fat.h  |    3 +--
>  fs/fat/file.c |   22 ++++++++++++++++------
>  3 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 530b4ca..1b73e60 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -19,6 +19,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/compat.h>
>  #include <asm/uaccess.h>
> +#include <linux/smp_lock.h>
>  #include "fat.h"
>  
>  /*
> @@ -737,10 +738,11 @@ efault:									   \
>  
>  FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent)
>  
> -static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
> +static long fat_ioctl_readdir(struct file *filp,
>  			     void __user *dirent, filldir_t filldir,
>  			     int short_only, int both)
>  {
> +	struct inode *inode = filp->f_dentry->d_inode;
>  	struct fat_ioctl_filldir_callback buf;
>  	int ret;
>  
> @@ -758,9 +760,10 @@ static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
>  	return ret;
>  }
>  
> -static int fat_dir_ioctl(struct inode *inode, struct file *filp,
> -			 unsigned int cmd, unsigned long arg)
> +static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
> +			unsigned long arg)
>  {
> +	long error;
>  	struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
>  	int short_only, both;
>  
> @@ -774,21 +777,31 @@ static int fat_dir_ioctl(struct inode *inode, struct file *filp,
>  		both = 1;
>  		break;
>  	default:
> -		return fat_generic_ioctl(inode, filp, cmd, arg);
> +		lock_kernel();
> +		error = fat_generic_ioctl(filp, cmd, arg);
> +		goto out;
>  	}
>  
> -	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
> -		return -EFAULT;
> +	lock_kernel();
> +	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2]))) {
> +		error = -EFAULT;
> +		goto out;
> +	}
>  	/*
>  	 * Yes, we don't need this put_user() absolutely. However old
>  	 * code didn't return the right value. So, app use this value,
>  	 * in order to check whether it is EOF.
>  	 */
> -	if (put_user(0, &d1->d_reclen))
> -		return -EFAULT;
> +	if (put_user(0, &d1->d_reclen)) {
> +		error = -EFAULT;
> +		goto out;
> +	}
>  
> -	return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir,
> +	error = fat_ioctl_readdir(filp, d1, fat_ioctl_filldir,
>  				 short_only, both);
> +out:
> +	unlock_kernel();
> +	return error;
>  }
>  
>  #ifdef CONFIG_COMPAT
> @@ -800,7 +813,6 @@ FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent)
>  static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
>  				 unsigned long arg)
>  {
> -	struct inode *inode = filp->f_path.dentry->d_inode;
>  	struct compat_dirent __user *d1 = compat_ptr(arg);
>  	int short_only, both;
>  
> @@ -827,7 +839,7 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
>  	if (put_user(0, &d1->d_reclen))
>  		return -EFAULT;
>  
> -	return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir,
> +	return fat_ioctl_readdir(filp, d1, fat_compat_ioctl_filldir,
>  				 short_only, both);
>  }
>  #endif /* CONFIG_COMPAT */
> @@ -836,7 +848,7 @@ const struct file_operations fat_dir_operations = {
>  	.llseek		= generic_file_llseek,
>  	.read		= generic_read_dir,
>  	.readdir	= fat_readdir,
> -	.ioctl		= fat_dir_ioctl,
> +	.unlocked_ioctl	= fat_dir_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl	= fat_compat_dir_ioctl,
>  #endif
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index e6efdfa..23f9b2a 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -298,8 +298,7 @@ extern int fat_free_clusters(struct inode *inode, int cluster);
>  extern int fat_count_free_clusters(struct super_block *sb);
>  
>  /* fat/file.c */
> -extern int fat_generic_ioctl(struct inode *inode, struct file *filp,
> -			     unsigned int cmd, unsigned long arg);
> +extern long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>  extern const struct file_operations fat_file_operations;
>  extern const struct inode_operations fat_file_inode_operations;
>  extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index e8c159d..4f3044f 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -16,6 +16,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/fsnotify.h>
>  #include <linux/security.h>
> +#include <linux/smp_lock.h>
>  #include "fat.h"
>  
>  static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
> @@ -114,19 +115,28 @@ out:
>  	return err;
>  }
>  
> -int fat_generic_ioctl(struct inode *inode, struct file *filp,
> -		      unsigned int cmd, unsigned long arg)
> +long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	long error;
> +	struct inode *inode = filp->f_dentry->d_inode;
>  	u32 __user *user_attr = (u32 __user *)arg;
>  
> +	lock_kernel();
> +
>  	switch (cmd) {
>  	case FAT_IOCTL_GET_ATTRIBUTES:
> -		return fat_ioctl_get_attributes(inode, user_attr);
> +		error = fat_ioctl_get_attributes(inode, user_attr);
> +		break;
>  	case FAT_IOCTL_SET_ATTRIBUTES:
> -		return fat_ioctl_set_attributes(filp, user_attr);
> +		error = fat_ioctl_set_attributes(filp, user_attr);
> +		break;
>  	default:
> -		return -ENOTTY;	/* Inappropriate ioctl for device */
> +		error = -ENOTTY;	/* Inappropriate ioctl for device */
> +		break;
>  	}
> +
> +	unlock_kernel();
> +	return error;
>  }
>  
>  static int fat_file_release(struct inode *inode, struct file *filp)
> @@ -159,7 +169,7 @@ const struct file_operations fat_file_operations = {
>  	.aio_write	= generic_file_aio_write,
>  	.mmap		= generic_file_mmap,
>  	.release	= fat_file_release,
> -	.ioctl		= fat_generic_ioctl,
> +	.unlocked_ioctl = fat_generic_ioctl,
>  	.fsync		= fat_file_fsync,
>  	.splice_read	= generic_file_splice_read,
>  };

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 3/6] fat: BKL ioctl pushdown

[John Kacur]

On Thu, 6 May 2010, OGAWA Hirofumi wrote:

> John Kacur <jkacur@redhat.com> writes:
> 
> > Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
> > and push down the bkl into those functions.
> 
> I guess this is the part of batch ioctl conversion stuff though, those
> ioctl of FAT don't need BKL at all. Because all of those should already
> be protected by inode->i_mutex.
> 
> Removing BKL and then cleanup after this patch would be almost same with
> reverting this patch. So, could you just convert to unlocked_ioctl
> instead?

That's probably not a good idea, without a little bit more analysis, 
otherwise it's quite easy to introduce subtle bugs.

> 
> Thanks.
> 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  fs/fat/dir.c  |   36 ++++++++++++++++++++++++------------
> >  fs/fat/fat.h  |    3 +--
> >  fs/fat/file.c |   22 ++++++++++++++++------
> >  3 files changed, 41 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> > index 530b4ca..1b73e60 100644
> > --- a/fs/fat/dir.c
> > +++ b/fs/fat/dir.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/buffer_head.h>
> >  #include <linux/compat.h>
> >  #include <asm/uaccess.h>
> > +#include <linux/smp_lock.h>
> >  #include "fat.h"
> >  
> >  /*
> > @@ -737,10 +738,11 @@ efault:									   \
> >  
> >  FAT_IOCTL_FILLDIR_FUNC(fat_ioctl_filldir, __fat_dirent)
> >  
> > -static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
> > +static long fat_ioctl_readdir(struct file *filp,
> >  			     void __user *dirent, filldir_t filldir,
> >  			     int short_only, int both)
> >  {
> > +	struct inode *inode = filp->f_dentry->d_inode;
> >  	struct fat_ioctl_filldir_callback buf;
> >  	int ret;
> >  
> > @@ -758,9 +760,10 @@ static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
> >  	return ret;
> >  }
> >  
> > -static int fat_dir_ioctl(struct inode *inode, struct file *filp,
> > -			 unsigned int cmd, unsigned long arg)
> > +static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
> > +			unsigned long arg)
> >  {
> > +	long error;
> >  	struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
> >  	int short_only, both;
> >  
> > @@ -774,21 +777,31 @@ static int fat_dir_ioctl(struct inode *inode, struct file *filp,
> >  		both = 1;
> >  		break;
> >  	default:
> > -		return fat_generic_ioctl(inode, filp, cmd, arg);
> > +		lock_kernel();
> > +		error = fat_generic_ioctl(filp, cmd, arg);
> > +		goto out;
> >  	}
> >  
> > -	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
> > -		return -EFAULT;
> > +	lock_kernel();
> > +	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2]))) {
> > +		error = -EFAULT;
> > +		goto out;
> > +	}
> >  	/*
> >  	 * Yes, we don't need this put_user() absolutely. However old
> >  	 * code didn't return the right value. So, app use this value,
> >  	 * in order to check whether it is EOF.
> >  	 */
> > -	if (put_user(0, &d1->d_reclen))
> > -		return -EFAULT;
> > +	if (put_user(0, &d1->d_reclen)) {
> > +		error = -EFAULT;
> > +		goto out;
> > +	}
> >  
> > -	return fat_ioctl_readdir(inode, filp, d1, fat_ioctl_filldir,
> > +	error = fat_ioctl_readdir(filp, d1, fat_ioctl_filldir,
> >  				 short_only, both);
> > +out:
> > +	unlock_kernel();
> > +	return error;
> >  }
> >  
> >  #ifdef CONFIG_COMPAT
> > @@ -800,7 +813,6 @@ FAT_IOCTL_FILLDIR_FUNC(fat_compat_ioctl_filldir, compat_dirent)
> >  static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
> >  				 unsigned long arg)
> >  {
> > -	struct inode *inode = filp->f_path.dentry->d_inode;
> >  	struct compat_dirent __user *d1 = compat_ptr(arg);
> >  	int short_only, both;
> >  
> > @@ -827,7 +839,7 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
> >  	if (put_user(0, &d1->d_reclen))
> >  		return -EFAULT;
> >  
> > -	return fat_ioctl_readdir(inode, filp, d1, fat_compat_ioctl_filldir,
> > +	return fat_ioctl_readdir(filp, d1, fat_compat_ioctl_filldir,
> >  				 short_only, both);
> >  }
> >  #endif /* CONFIG_COMPAT */
> > @@ -836,7 +848,7 @@ const struct file_operations fat_dir_operations = {
> >  	.llseek		= generic_file_llseek,
> >  	.read		= generic_read_dir,
> >  	.readdir	= fat_readdir,
> > -	.ioctl		= fat_dir_ioctl,
> > +	.unlocked_ioctl	= fat_dir_ioctl,
> >  #ifdef CONFIG_COMPAT
> >  	.compat_ioctl	= fat_compat_dir_ioctl,
> >  #endif
> > diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> > index e6efdfa..23f9b2a 100644
> > --- a/fs/fat/fat.h
> > +++ b/fs/fat/fat.h
> > @@ -298,8 +298,7 @@ extern int fat_free_clusters(struct inode *inode, int cluster);
> >  extern int fat_count_free_clusters(struct super_block *sb);
> >  
> >  /* fat/file.c */
> > -extern int fat_generic_ioctl(struct inode *inode, struct file *filp,
> > -			     unsigned int cmd, unsigned long arg);
> > +extern long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> >  extern const struct file_operations fat_file_operations;
> >  extern const struct inode_operations fat_file_inode_operations;
> >  extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
> > diff --git a/fs/fat/file.c b/fs/fat/file.c
> > index e8c159d..4f3044f 100644
> > --- a/fs/fat/file.c
> > +++ b/fs/fat/file.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/blkdev.h>
> >  #include <linux/fsnotify.h>
> >  #include <linux/security.h>
> > +#include <linux/smp_lock.h>
> >  #include "fat.h"
> >  
> >  static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
> > @@ -114,19 +115,28 @@ out:
> >  	return err;
> >  }
> >  
> > -int fat_generic_ioctl(struct inode *inode, struct file *filp,
> > -		      unsigned int cmd, unsigned long arg)
> > +long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  {
> > +	long error;
> > +	struct inode *inode = filp->f_dentry->d_inode;
> >  	u32 __user *user_attr = (u32 __user *)arg;
> >  
> > +	lock_kernel();
> > +
> >  	switch (cmd) {
> >  	case FAT_IOCTL_GET_ATTRIBUTES:
> > -		return fat_ioctl_get_attributes(inode, user_attr);
> > +		error = fat_ioctl_get_attributes(inode, user_attr);
> > +		break;
> >  	case FAT_IOCTL_SET_ATTRIBUTES:
> > -		return fat_ioctl_set_attributes(filp, user_attr);
> > +		error = fat_ioctl_set_attributes(filp, user_attr);
> > +		break;
> >  	default:
> > -		return -ENOTTY;	/* Inappropriate ioctl for device */
> > +		error = -ENOTTY;	/* Inappropriate ioctl for device */
> > +		break;
> >  	}
> > +
> > +	unlock_kernel();
> > +	return error;
> >  }
> >  
> >  static int fat_file_release(struct inode *inode, struct file *filp)
> > @@ -159,7 +169,7 @@ const struct file_operations fat_file_operations = {
> >  	.aio_write	= generic_file_aio_write,
> >  	.mmap		= generic_file_mmap,
> >  	.release	= fat_file_release,
> > -	.ioctl		= fat_generic_ioctl,
> > +	.unlocked_ioctl = fat_generic_ioctl,
> >  	.fsync		= fat_file_fsync,
> >  	.splice_read	= generic_file_splice_read,
> >  };
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/6] fat: BKL ioctl pushdown

[OGAWA Hirofumi]

John Kacur <jkacur@redhat.com> writes:

> On Thu, 6 May 2010, OGAWA Hirofumi wrote:
>
>> John Kacur <jkacur@redhat.com> writes:
>> 
>> > Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
>> > and push down the bkl into those functions.
>> 
>> I guess this is the part of batch ioctl conversion stuff though, those
>> ioctl of FAT don't need BKL at all. Because all of those should already
>> be protected by inode->i_mutex.
>> 
>> Removing BKL and then cleanup after this patch would be almost same with
>> reverting this patch. So, could you just convert to unlocked_ioctl
>> instead?
>
> That's probably not a good idea, without a little bit more analysis, 
> otherwise it's quite easy to introduce subtle bugs.

What analysis? Who do it? I thought about removing BKL of FAT from
several years ago. I was reviewing FAT multiple times, and I'm always
testing FAT without BKL.

If you are going to do, could you do it instead of this patch?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH] fat: convert to unlocked_ioctl

[Arnd Bergmann]

FAT does not require the BKL in its ioctl function, which is already serialized
through a mutex. Since we're already touching the ioctl code, also fix the
missing handling of FAT_IOCTL_GET_ATTRIBUTES in the compat code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
On Wednesday 05 May 2010 20:30:07 OGAWA Hirofumi wrote:
> John Kacur <jkacur@redhat.com> writes:
> > That's probably not a good idea, without a little bit more analysis, 
> > otherwise it's quite easy to introduce subtle bugs.

The chances are rather low if the maintainer confirms that the BKL
is not needed. I double-checked and found a small bug in related
code, so this does both.

> What analysis? Who do it? I thought about removing BKL of FAT from
> several years ago. I was reviewing FAT multiple times, and I'm always
> testing FAT without BKL.
> 
> If you are going to do, could you do it instead of this patch?

The only thing the series really needs to do is to remove the
usage of the deprecated ->ioctl() operation. This patch makes
FAT simply use unlocked_ioctl but does not reintroduce the
BKL.

Does that look better?

 fs/fat/dir.c  |   11 ++++++-----
 fs/fat/fat.h  |    4 ++--
 fs/fat/file.c |   19 ++++++++++++++++---
 3 files changed, 24 insertions(+), 10 deletions(-)


diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 530b4ca..d9bf865 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -758,9 +758,10 @@ static int fat_ioctl_readdir(struct inode *inode, struct file *filp,
 	return ret;
 }
 
-static int fat_dir_ioctl(struct inode *inode, struct file *filp,
-			 unsigned int cmd, unsigned long arg)
+static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
+			  unsigned long arg)
 {
+	struct inode *inode = filp->f_path.dentry->d_inode;
 	struct __fat_dirent __user *d1 = (struct __fat_dirent __user *)arg;
 	int short_only, both;
 
@@ -774,7 +775,7 @@ static int fat_dir_ioctl(struct inode *inode, struct file *filp,
 		both = 1;
 		break;
 	default:
-		return fat_generic_ioctl(inode, filp, cmd, arg);
+		return fat_generic_ioctl(filp, cmd, arg);
 	}
 
 	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct __fat_dirent[2])))
@@ -814,7 +815,7 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
 		both = 1;
 		break;
 	default:
-		return -ENOIOCTLCMD;
+		return fat_generic_ioctl(filp, cmd, (unsigned long)arg);
 	}
 
 	if (!access_ok(VERIFY_WRITE, d1, sizeof(struct compat_dirent[2])))
@@ -836,7 +837,7 @@ const struct file_operations fat_dir_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= fat_readdir,
-	.ioctl		= fat_dir_ioctl,
+	.unlocked_ioctl	= fat_dir_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= fat_compat_dir_ioctl,
 #endif
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index e6efdfa..eb821ee 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -298,8 +298,8 @@ extern int fat_free_clusters(struct inode *inode, int cluster);
 extern int fat_count_free_clusters(struct super_block *sb);
 
 /* fat/file.c */
-extern int fat_generic_ioctl(struct inode *inode, struct file *filp,
-			     unsigned int cmd, unsigned long arg);
+extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
+			      unsigned long arg);
 extern const struct file_operations fat_file_operations;
 extern const struct inode_operations fat_file_inode_operations;
 extern int fat_setattr(struct dentry * dentry, struct iattr * attr);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index e8c159d..a14c2f6 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -8,6 +8,7 @@
 
 #include <linux/capability.h>
 #include <linux/module.h>
+#include <linux/compat.h>
 #include <linux/mount.h>
 #include <linux/time.h>
 #include <linux/buffer_head.h>
@@ -114,9 +115,9 @@ out:
 	return err;
 }
 
-int fat_generic_ioctl(struct inode *inode, struct file *filp,
-		      unsigned int cmd, unsigned long arg)
+long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = filp->f_path.dentry->d_inode;
 	u32 __user *user_attr = (u32 __user *)arg;
 
 	switch (cmd) {
@@ -129,6 +130,15 @@ int fat_generic_ioctl(struct inode *inode, struct file *filp,
 	}
 }
 
+#ifdef CONFIG_COMPAT
+static long fat_generic_compat_ioctl(struct file *filp, unsigned int cmd,
+				      unsigned long arg)
+
+{
+	return fat_generic_ioctl(filp, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
 static int fat_file_release(struct inode *inode, struct file *filp)
 {
 	if ((filp->f_mode & FMODE_WRITE) &&
@@ -159,7 +169,10 @@ const struct file_operations fat_file_operations = {
 	.aio_write	= generic_file_aio_write,
 	.mmap		= generic_file_mmap,
 	.release	= fat_file_release,
-	.ioctl		= fat_generic_ioctl,
+	.unlocked_ioctl	= fat_generic_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= fat_generic_compat_ioctl,
+#endif
 	.fsync		= fat_file_fsync,
 	.splice_read	= generic_file_splice_read,
 };

Re: [PATCH] fat: convert to unlocked_ioctl

[OGAWA Hirofumi]

Arnd Bergmann <arnd@arndb.de> writes:

> FAT does not require the BKL in its ioctl function, which is already serialized
> through a mutex. Since we're already touching the ioctl code, also fix the
> missing handling of FAT_IOCTL_GET_ATTRIBUTES in the compat code.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

[...]

> The only thing the series really needs to do is to remove the
> usage of the deprecated ->ioctl() operation. This patch makes
> FAT simply use unlocked_ioctl but does not reintroduce the
> BKL.
>
> Does that look better?

Looks good to me. I'll apply this for next window after some tests.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 3/6] fat: BKL ioctl pushdown

[John Kacur]

On Thu, 6 May 2010, OGAWA Hirofumi wrote:

> John Kacur <jkacur@redhat.com> writes:
> 
> > On Thu, 6 May 2010, OGAWA Hirofumi wrote:
> >
> >> John Kacur <jkacur@redhat.com> writes:
> >> 
> >> > Convert fat_generic_ioctl and fat_dir_ioctl to unlocked_ioctls
> >> > and push down the bkl into those functions.
> >> 
> >> I guess this is the part of batch ioctl conversion stuff though, those
> >> ioctl of FAT don't need BKL at all. Because all of those should already
> >> be protected by inode->i_mutex.
> >> 
> >> Removing BKL and then cleanup after this patch would be almost same with
> >> reverting this patch. So, could you just convert to unlocked_ioctl
> >> instead?
> >
> > That's probably not a good idea, without a little bit more analysis, 
> > otherwise it's quite easy to introduce subtle bugs.
> 
> What analysis? Who do it? I thought about removing BKL of FAT from
> several years ago. I was reviewing FAT multiple times, and I'm always
> testing FAT without BKL.

This patch just makes explicit the hidden BKLs that FAT is already using 
related to ioctl. We would like to get this step done in time for the next
merge window, because then we can remove that hidden source.

This step is preparation for removing the BKL from the individual 
functions we've pushed it down into. In other words, we'll do the analysis 
to remove it later if you don't want to.

Thanks.

> 
> If you are going to do, could you do it instead of this patch?
> 

[PATCH 4/6] ncpfs: BKL ioctl pushdown

[John Kacur]

Convert ncp_ioctl to an unlocked_ioctl and push down the bkl into it.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 fs/ncpfs/dir.c         |    2 +-
 fs/ncpfs/file.c        |    2 +-
 fs/ncpfs/ioctl.c       |   27 ++++++++++++++++-----------
 include/linux/ncp_fs.h |    2 +-
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 7edfcd4..92dde6f 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -51,7 +51,7 @@ const struct file_operations ncp_dir_operations =
 {
 	.read		= generic_read_dir,
 	.readdir	= ncp_readdir,
-	.ioctl		= ncp_ioctl,
+	.unlocked_ioctl	= ncp_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ncp_compat_ioctl,
 #endif
diff --git a/fs/ncpfs/file.c b/fs/ncpfs/file.c
index 1daabb9..b938708 100644
--- a/fs/ncpfs/file.c
+++ b/fs/ncpfs/file.c
@@ -295,7 +295,7 @@ const struct file_operations ncp_file_operations =
 	.llseek 	= ncp_remote_llseek,
 	.read		= ncp_file_read,
 	.write		= ncp_file_write,
-	.ioctl		= ncp_ioctl,
+	.unlocked_ioctl	= ncp_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= ncp_compat_ioctl,
 #endif
diff --git a/fs/ncpfs/ioctl.c b/fs/ncpfs/ioctl.c
index 60a5e28..023c03d 100644
--- a/fs/ncpfs/ioctl.c
+++ b/fs/ncpfs/ioctl.c
@@ -20,6 +20,7 @@
 #include <linux/smp_lock.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 
 #include <linux/ncp_fs.h>
 
@@ -261,9 +262,9 @@ ncp_get_charsets(struct ncp_server* server, struct ncp_nls_ioctl __user *arg)
 }
 #endif /* CONFIG_NCPFS_NLS */
 
-static int __ncp_ioctl(struct inode *inode, struct file *filp,
-	      unsigned int cmd, unsigned long arg)
+static long __ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = filp->f_dentry->d_inode;
 	struct ncp_server *server = NCP_SERVER(inode);
 	int result;
 	struct ncp_ioctl_request request;
@@ -841,11 +842,11 @@ static int ncp_ioctl_need_write(unsigned int cmd)
 	}
 }
 
-int ncp_ioctl(struct inode *inode, struct file *filp,
-	      unsigned int cmd, unsigned long arg)
+long ncp_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	int ret;
+	long ret;
 
+	lock_kernel();
 	if (ncp_ioctl_need_write(cmd)) {
 		/*
 		 * inside the ioctl(), any failures which
@@ -853,24 +854,28 @@ int ncp_ioctl(struct inode *inode, struct file *filp,
 		 * -EACCESS, so it seems consistent to keep
 		 *  that here.
 		 */
-		if (mnt_want_write(filp->f_path.mnt))
-			return -EACCES;
+		if (mnt_want_write(filp->f_path.mnt)) {
+			ret = -EACCES;
+			goto out;
+		}
 	}
-	ret = __ncp_ioctl(inode, filp, cmd, arg);
+	ret = __ncp_ioctl(filp, cmd, arg);
 	if (ncp_ioctl_need_write(cmd))
 		mnt_drop_write(filp->f_path.mnt);
+
+out:
+	unlock_kernel();
 	return ret;
 }
 
 #ifdef CONFIG_COMPAT
 long ncp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	struct inode *inode = file->f_path.dentry->d_inode;
-	int ret;
+	long ret;
 
 	lock_kernel();
 	arg = (unsigned long) compat_ptr(arg);
-	ret = ncp_ioctl(inode, file, cmd, arg);
+	ret = ncp_ioctl(file, cmd, arg);
 	unlock_kernel();
 	return ret;
 }
diff --git a/include/linux/ncp_fs.h b/include/linux/ncp_fs.h
index 30b06c8..4522aed 100644
--- a/include/linux/ncp_fs.h
+++ b/include/linux/ncp_fs.h
@@ -210,7 +210,7 @@ int ncp_date_dos2unix(__le16 time, __le16 date);
 void ncp_date_unix2dos(int unix_date, __le16 * time, __le16 * date);
 
 /* linux/fs/ncpfs/ioctl.c */
-int ncp_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
+long ncp_ioctl(struct file *, unsigned int, unsigned long);
 long ncp_compat_ioctl(struct file *, unsigned int, unsigned long);
 
 /* linux/fs/ncpfs/sock.c */
-- 
1.6.6.1

Re: [PATCH 4/6] ncpfs: BKL ioctl pushdown

[Frederic Weisbecker]

2010/5/5 John Kacur <jkacur@redhat.com>:
> Convert ncp_ioctl to an unlocked_ioctl and push down the bkl into it.
>
> Signed-off-by: John Kacur <jkacur@redhat.com>


Applied, thanks.

[PATCH 5/6] smbfs: BKL ioctl pushdown

[John Kacur]

Convert smb_ioctl to an unlocked ioctl and push down the bkl into it.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 fs/smbfs/dir.c   |    2 +-
 fs/smbfs/file.c  |    2 +-
 fs/smbfs/ioctl.c |   10 +++++++---
 fs/smbfs/proto.h |    2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/smbfs/dir.c b/fs/smbfs/dir.c
index 3e4803b..6c97842 100644
--- a/fs/smbfs/dir.c
+++ b/fs/smbfs/dir.c
@@ -39,7 +39,7 @@ const struct file_operations smb_dir_operations =
 {
 	.read		= generic_read_dir,
 	.readdir	= smb_readdir,
-	.ioctl		= smb_ioctl,
+	.unlocked_ioctl	= smb_ioctl,
 	.open		= smb_dir_open,
 };
 
diff --git a/fs/smbfs/file.c b/fs/smbfs/file.c
index dbf6548..84ecf0e 100644
--- a/fs/smbfs/file.c
+++ b/fs/smbfs/file.c
@@ -437,7 +437,7 @@ const struct file_operations smb_file_operations =
 	.aio_read	= smb_file_aio_read,
 	.write		= do_sync_write,
 	.aio_write	= smb_file_aio_write,
-	.ioctl		= smb_ioctl,
+	.unlocked_ioctl	= smb_ioctl,
 	.mmap		= smb_file_mmap,
 	.open		= smb_file_open,
 	.release	= smb_file_release,
diff --git a/fs/smbfs/ioctl.c b/fs/smbfs/ioctl.c
index dbae1f8..dfc3a94 100644
--- a/fs/smbfs/ioctl.c
+++ b/fs/smbfs/ioctl.c
@@ -19,17 +19,19 @@
 #include <linux/smb_mount.h>
 
 #include <asm/uaccess.h>
+#include <linux/smp_lock.h>
 
 #include "proto.h"
 
-int
-smb_ioctl(struct inode *inode, struct file *filp,
-	  unsigned int cmd, unsigned long arg)
+long smb_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = filp->f_dentry->d_inode;
 	struct smb_sb_info *server = server_from_inode(inode);
 	struct smb_conn_opt opt;
 	int result = -EINVAL;
 
+	lock_kernel();
+
 	switch (cmd) {
 		uid16_t uid16;
 		uid_t uid32;
@@ -63,5 +65,7 @@ smb_ioctl(struct inode *inode, struct file *filp,
 		break;
 	}
 
+	unlock_kernel();
+
 	return result;
 }
diff --git a/fs/smbfs/proto.h b/fs/smbfs/proto.h
index 03f456c..05939a6 100644
--- a/fs/smbfs/proto.h
+++ b/fs/smbfs/proto.h
@@ -67,7 +67,7 @@ extern const struct address_space_operations smb_file_aops;
 extern const struct file_operations smb_file_operations;
 extern const struct inode_operations smb_file_inode_operations;
 /* ioctl.c */
-extern int smb_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg);
+extern long smb_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 /* smbiod.c */
 extern void smbiod_wake_up(void);
 extern int smbiod_register_server(struct smb_sb_info *server);
-- 
1.6.6.1

Re: [PATCH 5/6] smbfs: BKL ioctl pushdown

[Frederic Weisbecker]

On Wed, May 05, 2010 at 03:15:38PM +0200, John Kacur wrote:
> Convert smb_ioctl to an unlocked ioctl and push down the bkl into it.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>


Applied, thanks.



> ---
>  fs/smbfs/dir.c   |    2 +-
>  fs/smbfs/file.c  |    2 +-
>  fs/smbfs/ioctl.c |   10 +++++++---
>  fs/smbfs/proto.h |    2 +-
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/smbfs/dir.c b/fs/smbfs/dir.c
> index 3e4803b..6c97842 100644
> --- a/fs/smbfs/dir.c
> +++ b/fs/smbfs/dir.c
> @@ -39,7 +39,7 @@ const struct file_operations smb_dir_operations =
>  {
>  	.read		= generic_read_dir,
>  	.readdir	= smb_readdir,
> -	.ioctl		= smb_ioctl,
> +	.unlocked_ioctl	= smb_ioctl,
>  	.open		= smb_dir_open,
>  };
>  
> diff --git a/fs/smbfs/file.c b/fs/smbfs/file.c
> index dbf6548..84ecf0e 100644
> --- a/fs/smbfs/file.c
> +++ b/fs/smbfs/file.c
> @@ -437,7 +437,7 @@ const struct file_operations smb_file_operations =
>  	.aio_read	= smb_file_aio_read,
>  	.write		= do_sync_write,
>  	.aio_write	= smb_file_aio_write,
> -	.ioctl		= smb_ioctl,
> +	.unlocked_ioctl	= smb_ioctl,
>  	.mmap		= smb_file_mmap,
>  	.open		= smb_file_open,
>  	.release	= smb_file_release,
> diff --git a/fs/smbfs/ioctl.c b/fs/smbfs/ioctl.c
> index dbae1f8..dfc3a94 100644
> --- a/fs/smbfs/ioctl.c
> +++ b/fs/smbfs/ioctl.c
> @@ -19,17 +19,19 @@
>  #include <linux/smb_mount.h>
>  
>  #include <asm/uaccess.h>
> +#include <linux/smp_lock.h>
>  
>  #include "proto.h"
>  
> -int
> -smb_ioctl(struct inode *inode, struct file *filp,
> -	  unsigned int cmd, unsigned long arg)
> +long smb_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	struct inode *inode = filp->f_dentry->d_inode;
>  	struct smb_sb_info *server = server_from_inode(inode);
>  	struct smb_conn_opt opt;
>  	int result = -EINVAL;
>  
> +	lock_kernel();
> +
>  	switch (cmd) {
>  		uid16_t uid16;
>  		uid_t uid32;
> @@ -63,5 +65,7 @@ smb_ioctl(struct inode *inode, struct file *filp,
>  		break;
>  	}
>  
> +	unlock_kernel();
> +
>  	return result;
>  }
> diff --git a/fs/smbfs/proto.h b/fs/smbfs/proto.h
> index 03f456c..05939a6 100644
> --- a/fs/smbfs/proto.h
> +++ b/fs/smbfs/proto.h
> @@ -67,7 +67,7 @@ extern const struct address_space_operations smb_file_aops;
>  extern const struct file_operations smb_file_operations;
>  extern const struct inode_operations smb_file_inode_operations;
>  /* ioctl.c */
> -extern int smb_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg);
> +extern long smb_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>  /* smbiod.c */
>  extern void smbiod_wake_up(void);
>  extern int smbiod_register_server(struct smb_sb_info *server);
> -- 
> 1.6.6.1
> 

Re: [PATCH 5/6] smbfs: BKL ioctl pushdown

[Frederic Weisbecker]

On Wed, May 05, 2010 at 03:15:38PM +0200, John Kacur wrote:
> Convert smb_ioctl to an unlocked ioctl and push down the bkl into it.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>


In fact this one doesn't apply, not a single hunk.

Against which tree have you made this patch?

Re: [PATCH 5/6] smbfs: BKL ioctl pushdown

[John Kacur]

On Mon, 17 May 2010, Frederic Weisbecker wrote:

> On Wed, May 05, 2010 at 03:15:38PM +0200, John Kacur wrote:
> > Convert smb_ioctl to an unlocked ioctl and push down the bkl into it.
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> 
> In fact this one doesn't apply, not a single hunk.
> 
> Against which tree have you made this patch?

It was just Linus's latest, after 2.6.34-rc6
at commit 7ebd467551ed6ae200d7835a84bbda0dcadaa511

[PATCH 6/6] udf: BKL ioctl pushdown

[John Kacur]

Convert udf_ioctl to an unlocked_ioctl and push the BKL down into it.

Signed-off-by: John Kacur <jkacur@redhat.com
---
 fs/udf/dir.c     |    2 +-
 fs/udf/file.c    |   43 +++++++++++++++++++++++++++----------------
 fs/udf/udfdecl.h |    3 +--
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/fs/udf/dir.c b/fs/udf/dir.c
index f0f2a43..3a84455 100644
--- a/fs/udf/dir.c
+++ b/fs/udf/dir.c
@@ -209,6 +209,6 @@ static int udf_readdir(struct file *filp, void *dirent, filldir_t filldir)
 const struct file_operations udf_dir_operations = {
 	.read			= generic_read_dir,
 	.readdir		= udf_readdir,
-	.ioctl			= udf_ioctl,
+	.unlocked_ioctl		= udf_ioctl,
 	.fsync			= simple_fsync,
 };
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 4b6a46c..83092fb 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -37,6 +37,7 @@
 #include <linux/quotaops.h>
 #include <linux/buffer_head.h>
 #include <linux/aio.h>
+#include <linux/smp_lock.h>
 
 #include "udf_i.h"
 #include "udf_sb.h"
@@ -144,50 +145,60 @@ static ssize_t udf_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 	return retval;
 }
 
-int udf_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-	      unsigned long arg)
+long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+	struct inode *inode = filp->f_dentry->d_inode;
 	long old_block, new_block;
 	int result = -EINVAL;
 
+	lock_kernel();
+
 	if (file_permission(filp, MAY_READ) != 0) {
-		udf_debug("no permission to access inode %lu\n",
-			  inode->i_ino);
-		return -EPERM;
+		udf_debug("no permission to access inode %lu\n", inode->i_ino);
+		result = -EPERM;
+		goto out;
 	}
 
 	if (!arg) {
 		udf_debug("invalid argument to udf_ioctl\n");
-		return -EINVAL;
+		result = -EINVAL;
+		goto out;
 	}
 
 	switch (cmd) {
 	case UDF_GETVOLIDENT:
 		if (copy_to_user((char __user *)arg,
 				 UDF_SB(inode->i_sb)->s_volume_ident, 32))
-			return -EFAULT;
+			result = -EFAULT;
 		else
-			return 0;
+			result = 0;
+		goto out;
 	case UDF_RELOCATE_BLOCKS:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		if (get_user(old_block, (long __user *)arg))
-			return -EFAULT;
+		if (!capable(CAP_SYS_ADMIN)) {
+			result = -EACCES;
+			goto out;
+		}
+		if (get_user(old_block, (long __user *)arg)) {
+			result = -EFAULT;
+			goto out;
+		}
 		result = udf_relocate_blocks(inode->i_sb,
 						old_block, &new_block);
 		if (result == 0)
 			result = put_user(new_block, (long __user *)arg);
-		return result;
+		goto out;
 	case UDF_GETEASIZE:
 		result = put_user(UDF_I(inode)->i_lenEAttr, (int __user *)arg);
-		break;
+		goto out;
 	case UDF_GETEABLOCK:
 		result = copy_to_user((char __user *)arg,
 				      UDF_I(inode)->i_ext.i_data,
 				      UDF_I(inode)->i_lenEAttr) ? -EFAULT : 0;
-		break;
+		goto out;
 	}
 
+out:
+	unlock_kernel();
 	return result;
 }
 
@@ -207,7 +218,7 @@ static int udf_release_file(struct inode *inode, struct file *filp)
 const struct file_operations udf_file_operations = {
 	.read			= do_sync_read,
 	.aio_read		= generic_file_aio_read,
-	.ioctl			= udf_ioctl,
+	.unlocked_ioctl		= udf_ioctl,
 	.open			= dquot_file_open,
 	.mmap			= generic_file_mmap,
 	.write			= do_sync_write,
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 702a114..9079ff7 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -130,8 +130,7 @@ extern int udf_write_fi(struct inode *inode, struct fileIdentDesc *,
 			uint8_t *, uint8_t *);
 
 /* file.c */
-extern int udf_ioctl(struct inode *, struct file *, unsigned int,
-		     unsigned long);
+extern long udf_ioctl(struct file *, unsigned int, unsigned long);
 extern int udf_setattr(struct dentry *dentry, struct iattr *iattr);
 /* inode.c */
 extern struct inode *udf_iget(struct super_block *, struct kernel_lb_addr *);
-- 
1.6.6.1

Re: [PATCH 6/6] udf: BKL ioctl pushdown

[Jan Kara]

On Wed 05-05-10 15:15:39, John Kacur wrote:
> Convert udf_ioctl to an unlocked_ioctl and push the BKL down into it.
  Thanks. Merged into UDF tree.

								Honza

> 
> Signed-off-by: John Kacur <jkacur@redhat.com
> ---
>  fs/udf/dir.c     |    2 +-
>  fs/udf/file.c    |   43 +++++++++++++++++++++++++++----------------
>  fs/udf/udfdecl.h |    3 +--
>  3 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/udf/dir.c b/fs/udf/dir.c
> index f0f2a43..3a84455 100644
> --- a/fs/udf/dir.c
> +++ b/fs/udf/dir.c
> @@ -209,6 +209,6 @@ static int udf_readdir(struct file *filp, void *dirent, filldir_t filldir)
>  const struct file_operations udf_dir_operations = {
>  	.read			= generic_read_dir,
>  	.readdir		= udf_readdir,
> -	.ioctl			= udf_ioctl,
> +	.unlocked_ioctl		= udf_ioctl,
>  	.fsync			= simple_fsync,
>  };
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index 4b6a46c..83092fb 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -37,6 +37,7 @@
>  #include <linux/quotaops.h>
>  #include <linux/buffer_head.h>
>  #include <linux/aio.h>
> +#include <linux/smp_lock.h>
>  
>  #include "udf_i.h"
>  #include "udf_sb.h"
> @@ -144,50 +145,60 @@ static ssize_t udf_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
>  	return retval;
>  }
>  
> -int udf_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
> -	      unsigned long arg)
> +long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> +	struct inode *inode = filp->f_dentry->d_inode;
>  	long old_block, new_block;
>  	int result = -EINVAL;
>  
> +	lock_kernel();
> +
>  	if (file_permission(filp, MAY_READ) != 0) {
> -		udf_debug("no permission to access inode %lu\n",
> -			  inode->i_ino);
> -		return -EPERM;
> +		udf_debug("no permission to access inode %lu\n", inode->i_ino);
> +		result = -EPERM;
> +		goto out;
>  	}
>  
>  	if (!arg) {
>  		udf_debug("invalid argument to udf_ioctl\n");
> -		return -EINVAL;
> +		result = -EINVAL;
> +		goto out;
>  	}
>  
>  	switch (cmd) {
>  	case UDF_GETVOLIDENT:
>  		if (copy_to_user((char __user *)arg,
>  				 UDF_SB(inode->i_sb)->s_volume_ident, 32))
> -			return -EFAULT;
> +			result = -EFAULT;
>  		else
> -			return 0;
> +			result = 0;
> +		goto out;
>  	case UDF_RELOCATE_BLOCKS:
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		if (get_user(old_block, (long __user *)arg))
> -			return -EFAULT;
> +		if (!capable(CAP_SYS_ADMIN)) {
> +			result = -EACCES;
> +			goto out;
> +		}
> +		if (get_user(old_block, (long __user *)arg)) {
> +			result = -EFAULT;
> +			goto out;
> +		}
>  		result = udf_relocate_blocks(inode->i_sb,
>  						old_block, &new_block);
>  		if (result == 0)
>  			result = put_user(new_block, (long __user *)arg);
> -		return result;
> +		goto out;
>  	case UDF_GETEASIZE:
>  		result = put_user(UDF_I(inode)->i_lenEAttr, (int __user *)arg);
> -		break;
> +		goto out;
>  	case UDF_GETEABLOCK:
>  		result = copy_to_user((char __user *)arg,
>  				      UDF_I(inode)->i_ext.i_data,
>  				      UDF_I(inode)->i_lenEAttr) ? -EFAULT : 0;
> -		break;
> +		goto out;
>  	}
>  
> +out:
> +	unlock_kernel();
>  	return result;
>  }
>  
> @@ -207,7 +218,7 @@ static int udf_release_file(struct inode *inode, struct file *filp)
>  const struct file_operations udf_file_operations = {
>  	.read			= do_sync_read,
>  	.aio_read		= generic_file_aio_read,
> -	.ioctl			= udf_ioctl,
> +	.unlocked_ioctl		= udf_ioctl,
>  	.open			= dquot_file_open,
>  	.mmap			= generic_file_mmap,
>  	.write			= do_sync_write,
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 702a114..9079ff7 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -130,8 +130,7 @@ extern int udf_write_fi(struct inode *inode, struct fileIdentDesc *,
>  			uint8_t *, uint8_t *);
>  
>  /* file.c */
> -extern int udf_ioctl(struct inode *, struct file *, unsigned int,
> -		     unsigned long);
> +extern long udf_ioctl(struct file *, unsigned int, unsigned long);
>  extern int udf_setattr(struct dentry *dentry, struct iattr *iattr);
>  /* inode.c */
>  extern struct inode *udf_iget(struct super_block *, struct kernel_lb_addr *);
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/6] BKL ioctl pushdown

[Frederic Weisbecker]

On Wed, May 05, 2010 at 03:15:33PM +0200, John Kacur wrote:
> These patches convert ioctls to unlocked_ioctls by pushing down the bkl into
> them. I had to go outside of the files that we agreed on, because logically the
> ioctls in each kind of file system belong together.
> 
> Frederic and Arnd, you can pull from:
> git.kernel.org//pub/scm/linux/kernel/git/jkacur/jk-2.6.git linus-bkl-pushdown



Thanks John.

I will apply the patches from this set that haven't been applied yet,
for .35



 
> John Kacur (6):
>   coda: BKL ioctl pushdown
>   coda: Clean-up whitespace problems in pioctl.c
>   fat: BKL ioctl pushdown
>   ncpfs: BKL ioctl pushdown
>   smbfs: BKL ioctl pushdown
>   udf: BKL ioctl pushdown
> 
>  fs/coda/pioctl.c       |   76 ++++++++++++++++++++++++++----------------------
>  fs/fat/dir.c           |   36 +++++++++++++++-------
>  fs/fat/fat.h           |    3 +-
>  fs/fat/file.c          |   22 ++++++++++----
>  fs/ncpfs/dir.c         |    2 +-
>  fs/ncpfs/file.c        |    2 +-
>  fs/ncpfs/ioctl.c       |   27 ++++++++++-------
>  fs/smbfs/dir.c         |    2 +-
>  fs/smbfs/file.c        |    2 +-
>  fs/smbfs/ioctl.c       |   10 ++++--
>  fs/smbfs/proto.h       |    2 +-
>  fs/udf/dir.c           |    2 +-
>  fs/udf/file.c          |   43 +++++++++++++++++----------
>  fs/udf/udfdecl.h       |    3 +-
>  include/linux/ncp_fs.h |    2 +-
>  15 files changed, 140 insertions(+), 94 deletions(-)
>