[PATCH] [0/11] REPOST: Early exception fixes

[PATCH] [0/11] REPOST: Early exception fixes

[Andi Kleen]

Mostly saves some code size and cleans up the code after Roland's 
recent changes.

Please ack or nack.

-Andi

[PATCH] [1/11] Remove BKL from remote_llseek v2

[Andi Kleen]

- Replace remote_llseek with generic_file_llseek_unlocked (to force compilation 
failures in all users)
- Change all users to either use generic_file_llseek_unlocked 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.

Open problem: 32bit kernels can corrupt fpos because its modification
is not atomic, but they can do that anyways because there's other paths who 
modify it without BKL.

Do we need a special lock for the pos/f_version = 0 checks? 

Trond says the NFS BKL is likely not needed, but keep it for now
until his full audit.

v2: Use generic_file_llseek_unlocked instead of remote_llseek_unlocked
    and factor duplicated code (suggested by hch)

Cc: Trond.Myklebust@netapp.com
Cc: swhiteho@redhat.com
Cc: sfrench@samba.org
Cc: vandrove@vc.cvut.cz

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 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    |   38 +++++++++++---------------------------
 fs/smbfs/file.c    |   11 ++++++++++-
 include/linux/fs.h |    3 ++-
 7 files changed, 42 insertions(+), 34 deletions(-)

Index: linux/fs/cifs/cifsfs.c
===================================================================
--- linux.orig/fs/cifs/cifsfs.c
+++ linux/fs/cifs/cifsfs.c
@@ -581,7 +581,7 @@ static loff_t cifs_llseek(struct file *f
 		if (retval < 0)
 			return (loff_t)retval;
 	}
-	return remote_llseek(file, offset, origin);
+	return generic_file_llseek_unlocked(file, offset, origin);
 }
 
 struct file_system_type cifs_fs_type = {
Index: linux/fs/gfs2/ops_file.c
===================================================================
--- linux.orig/fs/gfs2/ops_file.c
+++ linux/fs/gfs2/ops_file.c
@@ -62,11 +62,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 = generic_file_llseek_unlocked(file, offset, origin);
 			gfs2_glock_dq_uninit(&i_gh);
 		}
 	} else
-		error = remote_llseek(file, offset, origin);
+		error = generic_file_llseek_unlocked(file, offset, origin);
 
 	return error;
 }
Index: linux/fs/ncpfs/file.c
===================================================================
--- linux.orig/fs/ncpfs/file.c
+++ linux/fs/ncpfs/file.c
@@ -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 *ino
 	return 0;
 }
 
+static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
+{
+	loff_t ret;
+	lock_kernel();
+	ret = generic_file_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
===================================================================
--- linux.orig/fs/read_write.c
+++ linux/fs/read_write.c
@@ -31,12 +31,12 @@ const struct file_operations generic_ro_
 
 EXPORT_SYMBOL(generic_ro_fops);
 
-loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
+loff_t
+generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
 {
 	loff_t retval;
 	struct inode *inode = file->f_mapping->host;
 
-	mutex_lock(&inode->i_mutex);
 	switch (origin) {
 		case SEEK_END:
 			offset += inode->i_size;
@@ -46,42 +46,26 @@ loff_t generic_file_llseek(struct file *
 	}
 	retval = -EINVAL;
 	if (offset>=0 && offset<=inode->i_sb->s_maxbytes) {
+		/* Special lock needed here? */
 		if (offset != file->f_pos) {
 			file->f_pos = offset;
 			file->f_version = 0;
 		}
 		retval = offset;
 	}
-	mutex_unlock(&inode->i_mutex);
 	return retval;
 }
+EXPORT_SYMBOL(generic_file_llseek_unlocked);
 
-EXPORT_SYMBOL(generic_file_llseek);
-
-loff_t remote_llseek(struct file *file, loff_t offset, int origin)
+loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
 {
-	loff_t retval;
-
-	lock_kernel();
-	switch (origin) {
-		case SEEK_END:
-			offset += i_size_read(file->f_path.dentry->d_inode);
-			break;
-		case SEEK_CUR:
-			offset += file->f_pos;
-	}
-	retval = -EINVAL;
-	if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
-		if (offset != file->f_pos) {
-			file->f_pos = offset;
-			file->f_version = 0;
-		}
-		retval = offset;
-	}
-	unlock_kernel();
-	return retval;
+	loff_t n;
+	mutex_lock(&file->f_dentry->d_inode->i_mutex);
+	n = generic_file_llseek_unlocked(file, offset, origin);
+	mutex_unlock(&file->f_dentry->d_inode->i_mutex);
+	return n;
 }
-EXPORT_SYMBOL(remote_llseek);
+EXPORT_SYMBOL(generic_file_llseek);
 
 loff_t no_llseek(struct file *file, loff_t offset, int origin)
 {
Index: linux/fs/smbfs/file.c
===================================================================
--- linux.orig/fs/smbfs/file.c
+++ linux/fs/smbfs/file.c
@@ -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 = generic_file_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
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -1871,7 +1871,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 generic_file_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
===================================================================
--- linux.orig/fs/nfs/file.c
+++ linux/fs/nfs/file.c
@@ -170,6 +170,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;
@@ -177,7 +178,10 @@ static loff_t nfs_file_llseek(struct fil
 		if (retval < 0)
 			return (loff_t)retval;
 	}
-	return remote_llseek(filp, offset, origin);
+	lock_kernel();	/* BKL needed? */
+	loff = generic_file_llseek_unlocked(filp, offset, origin);
+	unlock_kernel();
+	return loff;
 }
 
 /*

[PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

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.

There was still the problem of the actual flags change being
protected against other setters of flags. Instead of using BKL
for this use the i_mutex now.

I also added a mutex_lock against one other flags change
that was lockless and could potentially lose updates.

There are a couple of potential problems I added comments about on.

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 Documentation/filesystems/vfs.txt |    5 ++++-
 fs/fcntl.c                        |   22 +++++++++++++++-------
 fs/ioctl.c                        |   13 ++++++++++++-
 include/linux/fs.h                |    1 +
 4 files changed, 32 insertions(+), 9 deletions(-)

Index: linux/fs/fcntl.c
===================================================================
--- linux.orig/fs/fcntl.c
+++ linux/fs/fcntl.c
@@ -227,18 +227,26 @@ static int setfl(int fd, struct file * f
 	if (error)
 		return error;
 
-	lock_kernel();
+	/* AK: potential race here. Since test is unlocked fasync could
+	   be called several times in a row with on. We hope the drivers
+	   deal with it. */
 	if ((arg ^ filp->f_flags) & FASYNC) {
-		if (filp->f_op && filp->f_op->fasync) {
-			error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
-			if (error < 0)
-				goto out;
+		int on = !!(arg & 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();
 		}
+		/* AK: no else error = -EINVAL here? */
+		if (error < 0)
+			return error;
 	}
 
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
- out:
-	unlock_kernel();
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
Index: linux/fs/ioctl.c
===================================================================
--- linux.orig/fs/ioctl.c
+++ linux/fs/ioctl.c
@@ -103,10 +103,13 @@ static int ioctl_fionbio(struct file *fi
 	if (O_NONBLOCK != O_NDELAY)
 		flag |= O_NDELAY;
 #endif
+	/* Protect f_flags */
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	if (on)
 		filp->f_flags |= flag;
 	else
 		filp->f_flags &= ~flag;
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
@@ -122,8 +125,13 @@ static int ioctl_fioasync(unsigned int f
 	flag = on ? FASYNC : 0;
 
 	/* Did FASYNC state change ? */
+	/* AK: potential race here: ->fasync could be called with on two times
+	   in a row. We hope the drivers deal with it. */
 	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();
@@ -133,10 +141,13 @@ static int ioctl_fioasync(unsigned int f
 	if (error)
 		return error;
 
+	/* Protect f_flags */
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	if (on)
 		filp->f_flags |= FASYNC;
 	else
 		filp->f_flags &= ~FASYNC;
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -1237,6 +1237,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);
Index: linux/Documentation/filesystems/vfs.txt
===================================================================
--- linux.orig/Documentation/filesystems/vfs.txt
+++ linux/Documentation/filesystems/vfs.txt
@@ -755,6 +755,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 *);
@@ -814,7 +815,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

[PATCH] [3/11] Convert pipe over to unlocked_fasync

[Andi Kleen]

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 fs/pipe.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux/fs/pipe.c
===================================================================
--- linux.orig/fs/pipe.c
+++ linux/fs/pipe.c
@@ -787,7 +787,7 @@ const struct file_operations read_fifo_f
 	.unlocked_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 = {
@@ -799,7 +799,7 @@ const struct file_operations write_fifo_
 	.unlocked_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 = {
@@ -812,7 +812,7 @@ const struct file_operations rdwr_fifo_f
 	.unlocked_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 = {
@@ -824,7 +824,7 @@ static const struct file_operations read
 	.unlocked_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 = {
@@ -836,7 +836,7 @@ static const struct file_operations writ
 	.unlocked_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 = {
@@ -849,7 +849,7 @@ static const struct file_operations rdwr
 	.unlocked_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] [4/11] Convert socket fasync to unlocked_fasync

[Andi Kleen]

Pretty sure the socket layer has no BKL requirements.

Cc: davem@davemloft.net

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 net/socket.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/net/socket.c
===================================================================
--- linux.orig/net/socket.c
+++ linux/net/socket.c
@@ -134,7 +134,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,
 	.splice_read =	sock_splice_read,

[PATCH] [5/11] Convert fuse to unlocked_fasync

[Andi Kleen]

Cc: miklos@szeredi.hu

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 fs/fuse/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/fs/fuse/dev.c
===================================================================
--- linux.orig/fs/fuse/dev.c
+++ linux/fs/fuse/dev.c
@@ -1082,7 +1082,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] [6/11] Convert bad_inode to unlocked_fasync

[Andi Kleen]

Not that it matters much, but it was easy.

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 fs/bad_inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/fs/bad_inode.c
===================================================================
--- linux.orig/fs/bad_inode.c
+++ linux/fs/bad_inode.c
@@ -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,

[PATCH] [7/11] Convert DRM to unlocked_fasync

[Andi Kleen]

Doesn't need BKL because it just uses fasync_helper and minor->dev is 
protected by the file reference count.

Cc: airlied@linux.ie

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 drivers/char/drm/i810_dma.c   |    2 +-
 drivers/char/drm/i810_drv.c   |    2 +-
 drivers/char/drm/i830_dma.c   |    2 +-
 drivers/char/drm/i830_drv.c   |    2 +-
 drivers/char/drm/i915_drv.c   |    2 +-
 drivers/char/drm/mga_drv.c    |    2 +-
 drivers/char/drm/r128_drv.c   |    2 +-
 drivers/char/drm/radeon_drv.c |    2 +-
 drivers/char/drm/savage_drv.c |    2 +-
 drivers/char/drm/sis_drv.c    |    2 +-
 drivers/char/drm/tdfx_drv.c   |    2 +-
 drivers/char/drm/via_drv.c    |    2 +-
 12 files changed, 12 insertions(+), 12 deletions(-)

Index: linux/drivers/char/drm/i810_dma.c
===================================================================
--- linux.orig/drivers/char/drm/i810_dma.c
+++ linux/drivers/char/drm/i810_dma.c
@@ -117,7 +117,7 @@ static const struct file_operations i810
 	.release = drm_release,
 	.ioctl = drm_ioctl,
 	.mmap = i810_mmap_buffers,
-	.fasync = drm_fasync,
+	.unlocked_fasync = drm_fasync,
 };
 
 static int i810_map_buffer(struct drm_buf * buf, struct drm_file *file_priv)
Index: linux/drivers/char/drm/i810_drv.c
===================================================================
--- linux.orig/drivers/char/drm/i810_drv.c
+++ linux/drivers/char/drm/i810_drv.c
@@ -62,7 +62,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 
 	.pci_driver = {
Index: linux/drivers/char/drm/i830_dma.c
===================================================================
--- linux.orig/drivers/char/drm/i830_dma.c
+++ linux/drivers/char/drm/i830_dma.c
@@ -119,7 +119,7 @@ static const struct file_operations i830
 	.release = drm_release,
 	.ioctl = drm_ioctl,
 	.mmap = i830_mmap_buffers,
-	.fasync = drm_fasync,
+	.unlocked_fasync = drm_fasync,
 };
 
 static int i830_map_buffer(struct drm_buf * buf, struct drm_file *file_priv)
Index: linux/drivers/char/drm/i830_drv.c
===================================================================
--- linux.orig/drivers/char/drm/i830_drv.c
+++ linux/drivers/char/drm/i830_drv.c
@@ -73,7 +73,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 
 	.pci_driver = {
Index: linux/drivers/char/drm/i915_drv.c
===================================================================
--- linux.orig/drivers/char/drm/i915_drv.c
+++ linux/drivers/char/drm/i915_drv.c
@@ -559,7 +559,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 #ifdef CONFIG_COMPAT
 		 .compat_ioctl = i915_compat_ioctl,
 #endif
Index: linux/drivers/char/drm/mga_drv.c
===================================================================
--- linux.orig/drivers/char/drm/mga_drv.c
+++ linux/drivers/char/drm/mga_drv.c
@@ -71,7 +71,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 #ifdef CONFIG_COMPAT
 		 .compat_ioctl = mga_compat_ioctl,
 #endif
Index: linux/drivers/char/drm/r128_drv.c
===================================================================
--- linux.orig/drivers/char/drm/r128_drv.c
+++ linux/drivers/char/drm/r128_drv.c
@@ -66,7 +66,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 #ifdef CONFIG_COMPAT
 		 .compat_ioctl = r128_compat_ioctl,
 #endif
Index: linux/drivers/char/drm/radeon_drv.c
===================================================================
--- linux.orig/drivers/char/drm/radeon_drv.c
+++ linux/drivers/char/drm/radeon_drv.c
@@ -88,7 +88,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 #ifdef CONFIG_COMPAT
 		 .compat_ioctl = radeon_compat_ioctl,
 #endif
Index: linux/drivers/char/drm/savage_drv.c
===================================================================
--- linux.orig/drivers/char/drm/savage_drv.c
+++ linux/drivers/char/drm/savage_drv.c
@@ -53,7 +53,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 
 	.pci_driver = {
Index: linux/drivers/char/drm/sis_drv.c
===================================================================
--- linux.orig/drivers/char/drm/sis_drv.c
+++ linux/drivers/char/drm/sis_drv.c
@@ -83,7 +83,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 	.pci_driver = {
 		 .name = DRIVER_NAME,
Index: linux/drivers/char/drm/tdfx_drv.c
===================================================================
--- linux.orig/drivers/char/drm/tdfx_drv.c
+++ linux/drivers/char/drm/tdfx_drv.c
@@ -51,7 +51,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 	.pci_driver = {
 		 .name = DRIVER_NAME,
Index: linux/drivers/char/drm/via_drv.c
===================================================================
--- linux.orig/drivers/char/drm/via_drv.c
+++ linux/drivers/char/drm/via_drv.c
@@ -67,7 +67,7 @@ static struct drm_driver driver = {
 		 .ioctl = drm_ioctl,
 		 .mmap = drm_mmap,
 		 .poll = drm_poll,
-		 .fasync = drm_fasync,
+		 .unlocked_fasync = drm_fasync,
 	},
 	.pci_driver = {
 		 .name = DRIVER_NAME,

[PATCH] [8/11] Use unlocked_fasync in random.c

[Andi Kleen]

Just uses fasync_helper which does its own locking

Cc: mpm@selenic.com

Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 drivers/char/random.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/drivers/char/random.c
===================================================================
--- linux.orig/drivers/char/random.c
+++ linux/drivers/char/random.c
@@ -1121,7 +1121,7 @@ const struct file_operations random_fops
 	.write = random_write,
 	.poll  = random_poll,
 	.unlocked_ioctl = random_ioctl,
-	.fasync = random_fasync,
+	.unlocked_fasync = random_fasync,
 	.release = random_release,
 };
 
@@ -1129,7 +1129,7 @@ const struct file_operations urandom_fop
 	.read  = urandom_read,
 	.write = random_write,
 	.unlocked_ioctl = random_ioctl,
-	.fasync = random_fasync,
+	.unlocked_fasync = random_fasync,
 	.release = random_release,
 };
 

[PATCH] [9/11] Convert hpet to unlocked_fasync

[Andi Kleen]

Just calls fasync_helper and private structure is protected by 
file reference count.

Cc: clemens@ladisch.de
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 drivers/char/hpet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/char/hpet.c
===================================================================
--- linux.orig/drivers/char/hpet.c
+++ linux/drivers/char/hpet.c
@@ -585,7 +585,7 @@ static const struct file_operations hpet
 	.ioctl = hpet_ioctl,
 	.open = hpet_open,
 	.release = hpet_release,
-	.fasync = hpet_fasync,
+	.unlocked_fasync = hpet_fasync,
 	.mmap = hpet_mmap,
 };
 

[PATCH] [10/11] Use unlocked_fasync in RTC

[Andi Kleen]

Just uses fasync_helper
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 drivers/char/rtc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/char/rtc.c
===================================================================
--- linux.orig/drivers/char/rtc.c
+++ linux/drivers/char/rtc.c
@@ -913,7 +913,7 @@ static const struct file_operations rtc_
 	.ioctl		= rtc_ioctl,
 	.open		= rtc_open,
 	.release	= rtc_release,
-	.fasync		= rtc_fasync,
+	.unlocked_fasync = rtc_fasync,
 };
 
 static struct miscdevice rtc_dev = {

[PATCH] [11/11] Convert uio to fasync_unlocked

[Andi Kleen]

Just calls fasync_helper

Cc: hjk@linutronix.de
---
 drivers/uio/uio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/uio/uio.c
===================================================================
--- linux.orig/drivers/uio/uio.c
+++ linux/drivers/uio/uio.c
@@ -541,7 +541,7 @@ static const struct file_operations uio_
 	.read		= uio_read,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
-	.fasync		= uio_fasync,
+	.unlocked_fasync = uio_fasync,
 };
 
 static int uio_major_init(void)

Re: [PATCH] [2/11] Add unlocked_fasync

[Arjan van de Ven]

On Tue, 20 May 2008 17:28:43 +0200 (CEST)
Andi Kleen <andi@firstfloor.org> wrote:

> 
> Add a new fops entry point to allow fasync without BKL. 

I (again) really don't like having another entry point for this...
it'll stay around forever rather than doing this as one step and 
move on.

Re: [PATCH] [2/11] Add unlocked_fasync

[Randy Dunlap]

On Tue, 20 May 2008 17:28:43 +0200 (CEST) Andi Kleen wrote:

> 
> 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.
> 
> There was still the problem of the actual flags change being
> protected against other setters of flags. Instead of using BKL
> for this use the i_mutex now.
> 
> I also added a mutex_lock against one other flags change
> that was lockless and could potentially lose updates.
> 
> There are a couple of potential problems I added comments about on.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
>  Documentation/filesystems/vfs.txt |    5 ++++-
>  fs/fcntl.c                        |   22 +++++++++++++++-------
>  fs/ioctl.c                        |   13 ++++++++++++-
>  include/linux/fs.h                |    1 +
>  4 files changed, 32 insertions(+), 9 deletions(-)

> Index: linux/Documentation/filesystems/vfs.txt
> ===================================================================
> --- linux.orig/Documentation/filesystems/vfs.txt
> +++ linux/Documentation/filesystems/vfs.txt
> @@ -755,6 +755,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 *);
> @@ -814,7 +815,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

	                                           BKL held.
so is that BKL must be held by the caller or this function
holds the BKL?

> +
> +  unlocked_fasync: like fasync, but without BKL
>  
>    lock: called by the fcntl(2) system call for F_GETLK, F_SETLK, and F_SETLKW
>    	commands


---
~Randy

Re: [PATCH] [2/11] Add unlocked_fasync

[Jonathan Corbet]

I guess my one concern with this mirrors what other people have said:
might not it be better to just push the BKL down into the fasync()
implementations and avoid adding yet another file operation?  A quick
grep shows 43 drivers defining fasync() functions - and many of those
use the same one.  fs/ has a few more.  Obnoxious but doable, unless
there's something I'm missing?

Thanks,

jon

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

Arjan van de Ven wrote:
> On Tue, 20 May 2008 17:28:43 +0200 (CEST)
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>> Add a new fops entry point to allow fasync without BKL. 
> 
> I (again) really don't like having another entry point for this...
> it'll stay around forever rather than doing this as one step and 
> move on.

Yes the goal is for it staying around forever, correct. And ->fasync()
will go instead.

Advantage is that out of tree drivers will be compile broken which I
consider an advantage. Yes I know Linus said earlier that's not
important to him, but in this case my standards are higher than his.

Also BTW if you're that worried about the audit not getting
finished then the result would be just that lots of lock_kernel()s
would stay around. Hardly better.

But cannot do that many drivers in one step.

My goal is to just audit the remaining ones and then remove ->fasync()
and unlocked_fasync stays. Will be hopefully not that far away, since
fasync is relatively easy. The conversions are mostly for me to keep
track which ones I audited.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

Jonathan Corbet wrote:
> I guess my one concern with this mirrors what other people have said:
> might not it be better to just push the BKL down into the fasync()
> implementations and avoid adding yet another file operation?  A quick
> grep shows 43 drivers defining fasync() functions - and many of those
> use the same one.  fs/ has a few more.  Obnoxious but doable, unless
> there's something I'm missing?

See my reply to Arjan.  While for complicated stuff pushing down first
is better, fasync is not that complicated and I think my strategy
with the new entry point, with the old one going away is better in this
case.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Alan Cox]

On Tue, 20 May 2008 08:58:30 -0700
Arjan van de Ven <arjan@infradead.org> wrote:

> On Tue, 20 May 2008 17:28:43 +0200 (CEST)
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > 
> > Add a new fops entry point to allow fasync without BKL. 
> 
> I (again) really don't like having another entry point for this...
> it'll stay around forever rather than doing this as one step and 
> move on.

Agreed entirely - all our unlocked_foo methods we got stuck with, all our
push down everyone cases we got most cases promptly fixed and the other
lock/unlocks show up clearly in grep and help embarass the
maintainer/author 8)

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

> Agreed entirely - all our unlocked_foo methods we got stuck with,

That's the goal actually to "get stuck with it" and ->fasync going
away.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Arjan van de Ven]

On Tue, 20 May 2008 20:30:06 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Arjan van de Ven wrote:
> > On Tue, 20 May 2008 17:28:43 +0200 (CEST)
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> >> Add a new fops entry point to allow fasync without BKL. 
> > 
> > I (again) really don't like having another entry point for this...
> > it'll stay around forever rather than doing this as one step and 
> > move on.
> 
> Yes the goal is for it staying around forever, correct. And ->fasync()
> will go instead.
> 
> Advantage is that out of tree drivers will be compile broken which I
> consider an advantage. Yes I know Linus said earlier that's not
> important to him, but in this case my standards are higher than his.

I'd say it's just different standards.
My concern is that the new API as long lived is ugly and not the right
thing. I assume Linus and others have similar concerns, and weigh that
over the "some obscure out of tree driver might break".

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

> My concern is that the new API as long lived is ugly and not the right
> thing. I assume Linus and others have similar concerns, and weigh that
> over the "some obscure out of tree driver might break".

What do you find ugly exactly? The prefix "unlocked_" ? Other than
that there is no difference.

Duplicated entry points are not great, but they will be going away.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Arjan van de Ven]

On Wed, 21 May 2008 01:35:21 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> 
> > My concern is that the new API as long lived is ugly and not the
> > right thing. I assume Linus and others have similar concerns, and
> > weigh that over the "some obscure out of tree driver might break".
> 
> What do you find ugly exactly? The prefix "unlocked_" ?

yes exactly

Re: [PATCH] [7/11] Convert DRM to unlocked_fasync

[Dave Airlie]

> Doesn't need BKL because it just uses fasync_helper and minor->dev is 
> protected by the file reference count.
> 
> Cc: airlied@linux.ie
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: Dave Airlie <airlied@linux.ie>

unless you want me to take it via my tree.

Dave.

> 
> ---
>  drivers/char/drm/i810_dma.c   |    2 +-
>  drivers/char/drm/i810_drv.c   |    2 +-
>  drivers/char/drm/i830_dma.c   |    2 +-
>  drivers/char/drm/i830_drv.c   |    2 +-
>  drivers/char/drm/i915_drv.c   |    2 +-
>  drivers/char/drm/mga_drv.c    |    2 +-
>  drivers/char/drm/r128_drv.c   |    2 +-
>  drivers/char/drm/radeon_drv.c |    2 +-
>  drivers/char/drm/savage_drv.c |    2 +-
>  drivers/char/drm/sis_drv.c    |    2 +-
>  drivers/char/drm/tdfx_drv.c   |    2 +-
>  drivers/char/drm/via_drv.c    |    2 +-
>  12 files changed, 12 insertions(+), 12 deletions(-)
> 
> Index: linux/drivers/char/drm/i810_dma.c
> ===================================================================
> --- linux.orig/drivers/char/drm/i810_dma.c
> +++ linux/drivers/char/drm/i810_dma.c
> @@ -117,7 +117,7 @@ static const struct file_operations i810
>  	.release = drm_release,
>  	.ioctl = drm_ioctl,
>  	.mmap = i810_mmap_buffers,
> -	.fasync = drm_fasync,
> +	.unlocked_fasync = drm_fasync,
>  };
>  
>  static int i810_map_buffer(struct drm_buf * buf, struct drm_file *file_priv)
> Index: linux/drivers/char/drm/i810_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/i810_drv.c
> +++ linux/drivers/char/drm/i810_drv.c
> @@ -62,7 +62,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  
>  	.pci_driver = {
> Index: linux/drivers/char/drm/i830_dma.c
> ===================================================================
> --- linux.orig/drivers/char/drm/i830_dma.c
> +++ linux/drivers/char/drm/i830_dma.c
> @@ -119,7 +119,7 @@ static const struct file_operations i830
>  	.release = drm_release,
>  	.ioctl = drm_ioctl,
>  	.mmap = i830_mmap_buffers,
> -	.fasync = drm_fasync,
> +	.unlocked_fasync = drm_fasync,
>  };
>  
>  static int i830_map_buffer(struct drm_buf * buf, struct drm_file *file_priv)
> Index: linux/drivers/char/drm/i830_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/i830_drv.c
> +++ linux/drivers/char/drm/i830_drv.c
> @@ -73,7 +73,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  
>  	.pci_driver = {
> Index: linux/drivers/char/drm/i915_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/i915_drv.c
> +++ linux/drivers/char/drm/i915_drv.c
> @@ -559,7 +559,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  #ifdef CONFIG_COMPAT
>  		 .compat_ioctl = i915_compat_ioctl,
>  #endif
> Index: linux/drivers/char/drm/mga_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/mga_drv.c
> +++ linux/drivers/char/drm/mga_drv.c
> @@ -71,7 +71,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  #ifdef CONFIG_COMPAT
>  		 .compat_ioctl = mga_compat_ioctl,
>  #endif
> Index: linux/drivers/char/drm/r128_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/r128_drv.c
> +++ linux/drivers/char/drm/r128_drv.c
> @@ -66,7 +66,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  #ifdef CONFIG_COMPAT
>  		 .compat_ioctl = r128_compat_ioctl,
>  #endif
> Index: linux/drivers/char/drm/radeon_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/radeon_drv.c
> +++ linux/drivers/char/drm/radeon_drv.c
> @@ -88,7 +88,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  #ifdef CONFIG_COMPAT
>  		 .compat_ioctl = radeon_compat_ioctl,
>  #endif
> Index: linux/drivers/char/drm/savage_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/savage_drv.c
> +++ linux/drivers/char/drm/savage_drv.c
> @@ -53,7 +53,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  
>  	.pci_driver = {
> Index: linux/drivers/char/drm/sis_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/sis_drv.c
> +++ linux/drivers/char/drm/sis_drv.c
> @@ -83,7 +83,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  	.pci_driver = {
>  		 .name = DRIVER_NAME,
> Index: linux/drivers/char/drm/tdfx_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/tdfx_drv.c
> +++ linux/drivers/char/drm/tdfx_drv.c
> @@ -51,7 +51,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  	.pci_driver = {
>  		 .name = DRIVER_NAME,
> Index: linux/drivers/char/drm/via_drv.c
> ===================================================================
> --- linux.orig/drivers/char/drm/via_drv.c
> +++ linux/drivers/char/drm/via_drv.c
> @@ -67,7 +67,7 @@ static struct drm_driver driver = {
>  		 .ioctl = drm_ioctl,
>  		 .mmap = drm_mmap,
>  		 .poll = drm_poll,
> -		 .fasync = drm_fasync,
> +		 .unlocked_fasync = drm_fasync,
>  	},
>  	.pci_driver = {
>  		 .name = DRIVER_NAME,
> 
> 

Re: [PATCH] [9/11] Convert hpet to unlocked_fasync

[Clemens Ladisch]

Andi Kleen wrote:
> Just calls fasync_helper and private structure is protected by
> file reference count.
>
> Cc: clemens@ladisch.de
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: Clemens Ladisch <clemens@ladisch.de>

Re: [PATCH] [2/11] Add unlocked_fasync

[Randy Dunlap]

On Mon, 19 May 2008 14:31:11 +0200 (CEST) Andi Kleen wrote:

> Index: linux/Documentation/filesystems/vfs.txt
> ===================================================================
> --- linux.orig/Documentation/filesystems/vfs.txt
> +++ linux/Documentation/filesystems/vfs.txt
> @@ -755,6 +755,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 *);
> @@ -814,7 +815,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

?                                                  BKL is held.

> +
> +  unlocked_fasync: like fasync, but without BKL
>  
>    lock: called by the fcntl(2) system call for F_GETLK, F_SETLK, and F_SETLKW
>    	commands


---
~Randy

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

Alan Cox wrote:
>> The good news is that none of them I looked at actually needed BKL
>> (or perhaps I just missed it)
> 
> Far better to send an initial patch that simply adds all the lock/unlocks
> then remove them later than worry about it immediately - that makes it
> easier to verify changes and to do testing

I fail to see how that makes it easier than with ->fasync_unlocked.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

Alan Cox wrote:
> On Mon, 19 May 2008 16:43:05 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>>> I'd rather do a properly flag day and move lock_kernel into the
>>> instances instead of adding this _unlocked gunk.
>> I've considered that, but it's a bit too many fasync instances
>> all over the tree, so I chose this method.
> 
> If I can do all the watchdog drivers in one go then we can do all the
> fasync interfaces in one go. It isn't that hard other than making sure
> people didn't hide them in odd places on platforms not usually built.

If it's that easy for you please audit the ones then I didn't cover yet and send 
me a list.

The good news is that none of them I looked at actually needed BKL
(or perhaps I just missed it)

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Alan Cox]

> The good news is that none of them I looked at actually needed BKL
> (or perhaps I just missed it)

Far better to send an initial patch that simply adds all the lock/unlocks
then remove them later than worry about it immediately - that makes it
easier to verify changes and to do testing

Alan

Re: [PATCH] [2/11] Add unlocked_fasync

[Alan Cox]

On Mon, 19 May 2008 16:43:05 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> 
> > I'd rather do a properly flag day and move lock_kernel into the
> > instances instead of adding this _unlocked gunk.
> 
> I've considered that, but it's a bit too many fasync instances
> all over the tree, so I chose this method.

If I can do all the watchdog drivers in one go then we can do all the
fasync interfaces in one go. It isn't that hard other than making sure
people didn't hide them in odd places on platforms not usually built.

Alan

Re: [PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

> I'd rather do a properly flag day and move lock_kernel into the
> instances instead of adding this _unlocked gunk.

I've considered that, but it's a bit too many fasync instances
all over the tree, so I chose this method.

-Andi

Re: [PATCH] [2/11] Add unlocked_fasync

[Christoph Hellwig]

On Mon, May 19, 2008 at 02:31:11PM +0200, Andi Kleen wrote:
> 
> 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.
> 
> There was still the problem of the actual flags change being
> protected against other setters of flags. Instead of using BKL
> for this use the i_mutex now.
> 
> I also added a mutex_lock against one other flags change
> that was lockless and could potentially lose updates.
> 
> There are a couple of potential problems I added comments about on.

I'd rather do a properly flag day and move lock_kernel into the
instances instead of adding this _unlocked gunk.

[PATCH] [2/11] Add unlocked_fasync

[Andi Kleen]

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.

There was still the problem of the actual flags change being
protected against other setters of flags. Instead of using BKL
for this use the i_mutex now.

I also added a mutex_lock against one other flags change
that was lockless and could potentially lose updates.

There are a couple of potential problems I added comments about on.

Signed-off-by: Andi Kleen <ak@suse.de>
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---
 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(-)

Index: linux/fs/fcntl.c
===================================================================
--- linux.orig/fs/fcntl.c
+++ linux/fs/fcntl.c
@@ -227,18 +227,26 @@ static int setfl(int fd, struct file * f
 	if (error)
 		return error;
 
-	lock_kernel();
+	/* AK: potential race here. Since test is unlocked fasync could
+	   be called several times in a row with on. We hope the drivers
+	   deal with it. */
 	if ((arg ^ filp->f_flags) & FASYNC) {
-		if (filp->f_op && filp->f_op->fasync) {
-			error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
-			if (error < 0)
-				goto out;
+		int on = !!(arg & 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();
 		}
+		/* AK: no else error = -EINVAL here? */
+		if (error < 0)
+			return error;
 	}
 
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
- out:
-	unlock_kernel();
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
Index: linux/fs/ioctl.c
===================================================================
--- linux.orig/fs/ioctl.c
+++ linux/fs/ioctl.c
@@ -103,10 +103,13 @@ static int ioctl_fionbio(struct file *fi
 	if (O_NONBLOCK != O_NDELAY)
 		flag |= O_NDELAY;
 #endif
+	/* Protect f_flags */
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	if (on)
 		filp->f_flags |= flag;
 	else
 		filp->f_flags &= ~flag;
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
@@ -122,8 +125,13 @@ static int ioctl_fioasync(unsigned int f
 	flag = on ? FASYNC : 0;
 
 	/* Did FASYNC state change ? */
+	/* AK: potential race here: ->fasync could be called with on two times
+	   in a row. We hope the drivers deal with it. */
 	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();
@@ -133,10 +141,13 @@ static int ioctl_fioasync(unsigned int f
 	if (error)
 		return error;
 
+	/* Protect f_flags */
+	mutex_lock(&filp->f_dentry->d_inode->i_mutex);
 	if (on)
 		filp->f_flags |= FASYNC;
 	else
 		filp->f_flags &= ~FASYNC;
+	mutex_unlock(&filp->f_dentry->d_inode->i_mutex);
 	return error;
 }
 
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h
+++ linux/include/linux/fs.h
@@ -1237,6 +1237,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);
Index: linux/Documentation/filesystems/vfs.txt
===================================================================
--- linux.orig/Documentation/filesystems/vfs.txt
+++ linux/Documentation/filesystems/vfs.txt
@@ -755,6 +755,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 *);
@@ -814,7 +815,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