Skip to contentSkip to conversation

Repost of old VFS BKL patchkit

PATCH In review Andi Kleen 7 participants 26 messages May 19, 2008

Subsystems (13)
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

cover Repost of old VFS BKL patchkit

Since BKL removal seems to be en-vogue again here's the repost of the unapplied parts of my older VFS BKL patchkit.

Main difference is port to 2.6.26-rc2 and more fasync conversins.

- Converts remote_llseek to be BKL less. This will either eliminate or push it down in a couple of network file systems (SMBFS, CIFS, OCFS2, NCPFS, NFS)
- Converts file flags handling to i_mutex
[BTW while looking over the tree I think there are a few races in this regard in there anyways. Not all people who modify that take BKL. Auditing all of them is a separate project. No difference before/after my patch]
- Adds unlocked_fasync
- Converts lots of fasync users to unlocked_fasync

-Andi

author
#16 lore raw

PATCH 4/11 Convert socket fasync to unlocked_fasync

Pretty sure the socket layer has no BKL requirements.

---
 net/socket.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/socket.c +1 -1
Index: 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,
author
#1 lore raw

PATCH 1/11 Remove BKL from remote_llseek

In reply to Andi Kleen

- Replace remote_llseek with remote_llseek_unlocked (to force compilation failures in all users)
- Change all users to either use remote_llseek directly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.

I moved them all over in a single patch to avoid unbisectable sections.

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.

---
 fs/cifs/cifsfs.c   |    2 +-
 fs/gfs2/ops_file.c |    4 ++--
 fs/ncpfs/file.c    |   12 +++++++++++-
 fs/nfs/file.c      |    6 +++++-
 fs/read_write.c    |    7 +++----
 fs/smbfs/file.c    |   11 ++++++++++-
 include/linux/fs.h |    3 ++-
 7 files changed, 34 insertions(+), 11 deletions(-)
fs/cifs/cifsfs.c +1 -1
Index: 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 remote_llseek_unlocked(file, offset, origin);
 }
 
 struct file_system_type cifs_fs_type = {
fs/gfs2/ops_file.c +2 -2
Index: 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 = remote_llseek_unlocked(file, offset, origin);
 			gfs2_glock_dq_uninit(&i_gh);
 		}
 	} else
-		error = remote_llseek(file, offset, origin);
+		error = remote_llseek_unlocked(file, offset, origin);
 
 	return error;
 }
fs/ncpfs/file.c +11 -1
Index: 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 = remote_llseek_unlocked(file, offset, origin);
+	unlock_kernel();
+	return ret;
+}
+
 const struct file_operations ncp_file_operations =
 {
-	.llseek		= remote_llseek,
+	.llseek 	= ncp_remote_llseek,
 	.read		= ncp_file_read,
 	.write		= ncp_file_write,
 	.ioctl		= ncp_ioctl,
fs/read_write.c +3 -4
Index: linux/fs/read_write.c
===================================================================
@@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *
 
 EXPORT_SYMBOL(generic_file_llseek);
 
-loff_t remote_llseek(struct file *file, loff_t offset, int origin)
+loff_t remote_llseek_unlocked(struct file *file, loff_t offset, int origin)
 {
 	loff_t retval;
 
-	lock_kernel();
 	switch (origin) {
 		case SEEK_END:
 			offset += i_size_read(file->f_path.dentry->d_inode);
@@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file, 
 	retval = -EINVAL;
 	if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
 		if (offset != file->f_pos) {
+			/* AK: do we need a lock for those? */
 			file->f_pos = offset;
 			file->f_version = 0;
 		}
 		retval = offset;
 	}
-	unlock_kernel();
 	return retval;
 }
-EXPORT_SYMBOL(remote_llseek);
+EXPORT_SYMBOL(remote_llseek_unlocked);
 
 loff_t no_llseek(struct file *file, loff_t offset, int origin)
 {
fs/smbfs/file.c +10 -1
Index: 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 = remote_llseek_unlocked(file, offset, origin);
+	unlock_kernel();
+	return ret;
+}
+
 const struct file_operations smb_file_operations =
 {
-	.llseek		= remote_llseek,
+	.llseek 	= smb_remote_llseek,
 	.read		= do_sync_read,
 	.aio_read	= smb_file_aio_read,
 	.write		= do_sync_write,
include/linux/fs.h +2 -1
Index: 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 remote_llseek_unlocked(struct file *file, loff_t offset,
+			int origin);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 
fs/nfs/file.c +5 -1
Index: 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 = remote_llseek_unlocked(filp, offset, origin);
+	unlock_kernel();
+	return loff;
 }
 
 /*
author
#20 lore raw

PATCH 8/11 Use unlocked_fasync in random.c

In reply to Andi Kleen

Just uses fasync_helper which does its own locking

drivers/char/random.c +2 -2
Index: 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,
 };
 
author
#17 lore raw

PATCH 5/11 Convert fuse to unlocked_fasync

In reply to Andi Kleen
---
 fs/fuse/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/fuse/dev.c +1 -1
Index: linux/fs/fuse/dev.c
===================================================================
@@ -1040,7 +1040,7 @@ const struct file_operations fuse_dev_op
 	.aio_write	= fuse_dev_write,
 	.poll		= fuse_dev_poll,
 	.release	= fuse_dev_release,
-	.fasync		= fuse_dev_fasync,
+	.unlocked_fasync = fuse_dev_fasync,
 };
 
 static struct miscdevice fuse_miscdevice = {
author
#7 lore raw

PATCH 2/11 Add unlocked_fasync

In reply to 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.

---
 Documentation/filesystems/vfs.txt |    5 ++++-
 fs/fcntl.c                        |    6 +++++-
 fs/ioctl.c                        |    5 ++++-
 include/linux/fs.h                |    1 +
 4 files changed, 14 insertions(+), 3 deletions(-)
fs/fcntl.c +15 -7
Index: 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;
 }
 
fs/ioctl.c +12 -1
Index: 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;
 }
 
include/linux/fs.h +1 -0
Index: 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);
Documentation/filesystems/vfs.txt +4 -1
Index: 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
author
#18 lore raw

PATCH 6/11 Convert bad_inode to unlocked_fasync

In reply to Andi Kleen

Not that it matters much, but it was easy.

---
 fs/bad_inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/bad_inode.c +1 -1
Index: 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,
author
#15 lore raw

PATCH 3/11 Convert pipe over to unlocked_fasync

In reply to Andi Kleen
---
 fs/pipe.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
fs/pipe.c +6 -6
Index: 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)
author
#19 lore raw

PATCH 7/11 Convert DRM to unlocked_fasync

In reply to Andi Kleen

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

drivers/char/drm/i810_dma.c +1 -1
Index: 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)
drivers/char/drm/i810_drv.c +1 -1
Index: 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 = {
drivers/char/drm/i830_dma.c +1 -1
Index: 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)
drivers/char/drm/i830_drv.c +1 -1
Index: 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 = {
drivers/char/drm/i915_drv.c +1 -1
Index: 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
drivers/char/drm/mga_drv.c +1 -1
Index: 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
drivers/char/drm/r128_drv.c +1 -1
Index: 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
drivers/char/drm/radeon_drv.c +1 -1
Index: 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
drivers/char/drm/savage_drv.c +1 -1
Index: 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 = {
drivers/char/drm/sis_drv.c +1 -1
Index: 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,
drivers/char/drm/tdfx_drv.c +1 -1
Index: 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,
drivers/char/drm/via_drv.c +1 -1
Index: 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,
author
#22 lore raw

PATCH 10/11 Use unlocked_fasync in RTC

In reply to Andi Kleen

Just uses fasync_helper

drivers/char/rtc.c +1 -1
Index: 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 = {
author
#21 lore raw

PATCH 9/11 Convert hpet to unlocked_fasync

In reply to Andi Kleen

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

drivers/char/hpet.c +1 -1
Index: 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,
 };
 
author
#24 lore raw

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

In reply to Andi Kleen
Quoted 3 lines · Andi Kleen

Just calls fasync_helper

Sorry that one was missing a quilt refresh. Here's the real patch. Not very exciting.

-Andi

---

Convert uio to fasync_unlocked

Just calls fasync_helper

drivers/uio/uio.c +1 -1
Index: 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)
Acked-by
#25 lore raw

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

Quoted 8 lines · Andi Kleen
Quoted 3 lines · Andi Kleen

Just calls fasync_helper

Sorry that one was missing a quilt refresh. Here's the real patch. Not very exciting.

Patch looks alright to me, but Greg KH should be cc'ed for UIO patches as well.

Quoted 10 lines

-Andi

---

Convert uio to fasync_unlocked

Just calls fasync_helper

Quoted 1 line
Quoted 19 lines

Index: linux/drivers/uio/uio.c
===================================================================

drivers/uio/uio.c +1 -2
@@ -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)
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw

Re: [PATCH] [1/11] Remove BKL from remote_llseek

In reply to Andi Kleen
Quoted 180 lines · Andi Kleen

- Replace remote_llseek with remote_llseek_unlocked (to force compilation failures in all users)
- Change all users to either use remote_llseek directly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.

I moved them all over in a single patch to avoid unbisectable sections.

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.

---
 fs/cifs/cifsfs.c   |    2 +-
 fs/gfs2/ops_file.c |    4 ++--
 fs/ncpfs/file.c    |   12 +++++++++++-
 fs/nfs/file.c      |    6 +++++-
 fs/read_write.c    |    7 +++----
 fs/smbfs/file.c    |   11 ++++++++++-
 include/linux/fs.h |    3 ++-
 7 files changed, 34 insertions(+), 11 deletions(-)

Index: linux/fs/cifs/cifsfs.c
===================================================================

fs/cifs/cifsfs.c +1 -1
@@ -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 remote_llseek_unlocked(file, offset, origin);
 }
 
 struct file_system_type cifs_fs_type = {

Index: linux/fs/gfs2/ops_file.c
===================================================================

fs/gfs2/ops_file.c +2 -2
@@ -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 = remote_llseek_unlocked(file, offset, origin);
 			gfs2_glock_dq_uninit(&i_gh);
 		}
 	} else
-		error = remote_llseek(file, offset, origin);
+		error = remote_llseek_unlocked(file, offset, origin);
 
 	return error;
 }

Index: linux/fs/ncpfs/file.c
===================================================================

fs/ncpfs/file.c +11 -1
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 
 #include <linux/ncp_fs.h>
 #include "ncplib_kernel.h"
@@ -281,9 +282,18 @@ static int ncp_release(struct inode *ino
 	return 0;
 }
 
+static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
+{
+	loff_t ret;
+	lock_kernel();
+	ret = remote_llseek_unlocked(file, offset, origin);
+	unlock_kernel();
+	return ret;
+}
+
 const struct file_operations ncp_file_operations =
 {
-	.llseek		= remote_llseek,
+	.llseek 	= ncp_remote_llseek,
 	.read		= ncp_file_read,
 	.write		= ncp_file_write,
 	.ioctl		= ncp_ioctl,

Index: linux/fs/read_write.c
===================================================================

fs/read_write.c +3 -4
@@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *
 
 EXPORT_SYMBOL(generic_file_llseek);
 
-loff_t remote_llseek(struct file *file, loff_t offset, int origin)
+loff_t remote_llseek_unlocked(struct file *file, loff_t offset, int origin)
 {
 	loff_t retval;
 
-	lock_kernel();
 	switch (origin) {
 		case SEEK_END:
 			offset += i_size_read(file->f_path.dentry->d_inode);
@@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file, 
 	retval = -EINVAL;
 	if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
 		if (offset != file->f_pos) {
+			/* AK: do we need a lock for those? */
 			file->f_pos = offset;
 			file->f_version = 0;
 		}
 		retval = offset;
 	}
-	unlock_kernel();
 	return retval;
 }
-EXPORT_SYMBOL(remote_llseek);
+EXPORT_SYMBOL(remote_llseek_unlocked);
 
 loff_t no_llseek(struct file *file, loff_t offset, int origin)
 {

Index: linux/fs/smbfs/file.c
===================================================================

fs/smbfs/file.c +10 -1
@@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode,
 	return error;
 }
 
+static loff_t smb_remote_llseek(struct file *file, loff_t offset, int origin)
+{
+	loff_t ret;
+	lock_kernel();
+	ret = remote_llseek_unlocked(file, offset, origin);
+	unlock_kernel();
+	return ret;
+}
+
 const struct file_operations smb_file_operations =
 {
-	.llseek		= remote_llseek,
+	.llseek 	= smb_remote_llseek,
 	.read		= do_sync_read,
 	.aio_read	= smb_file_aio_read,
 	.write		= do_sync_write,

Index: linux/include/linux/fs.h
===================================================================

include/linux/fs.h +2 -1
@@ -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 remote_llseek_unlocked(struct file *file, loff_t offset,
+			int origin);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 

Index: linux/fs/nfs/file.c
===================================================================

fs/nfs/file.c +1 -0
@@ -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;

fs/nfs/file.c static loff_t nfs_file_llseek(struct fil · line 184

Quoted 8 lines
fs/nfs/file.c +4 -1
@@ -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 = remote_llseek_unlocked(filp, offset, origin);
+	unlock_kernel();
+	return loff;

The NFS client shouldn't need any special locking around remote_llseek() beyond whatever is required at the VFS level.

The one case in nfs_file_llseek() where we might care is the call to nfs_revalidate_file_size() ('cos I still haven't finished auditing BKL dependencies in the inode attributes). However that case should already be covered without introducing any new lock_kernel/unlock_kernel calls since __nfs_revalidate_inode() already grabs the BKL.

Cheers
Trond

Signature · 6 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
#4 lore raw

Re: [PATCH] [1/11] Remove BKL from remote_llseek

In reply to Andi Kleen
Quoted 9 lines · Andi Kleen

- Replace remote_llseek with remote_llseek_unlocked (to force compilation failures in all users)
- Change all users to either use remote_llseek directly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.

I moved them all over in a single patch to avoid unbisectable sections.

Good idea, but it could be done a little bit nicer still.

Your new remote_llseek_unlocked is the same as generic_file_llseek except for the fact that it doesn't take i_mutex. So I'd rather call it generic_file_llseek_nolock and make generic_file_llseek call it. While you're at it you could also add some kerneldoc documents describing them.

#8 lore raw

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

In reply to Andi Kleen
Quoted 7 lines · 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.

Quoted 8 lines

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.

author
#3 lore raw

Re: [PATCH] [1/11] Remove BKL from remote_llseek

In reply to Trond Myklebust
Quoted 6 lines

The one case in nfs_file_llseek() where we might care is the call to nfs_revalidate_file_size() ('cos I still haven't finished auditing BKL dependencies in the inode attributes). However that case should already be covered without introducing any new lock_kernel/unlock_kernel calls since __nfs_revalidate_inode() already grabs the BKL.

Thanks. I'll keep it for now with the explicit lock_kernel() and when you're finished your audit and the patches are in before you can just remove them yourself. I guess that's the best way to handle it.

-Andi

author
#9 lore raw

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

In reply to Christoph Hellwig
Quoted 3 lines

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

author
#5 lore raw

Re: [PATCH] [1/11] Remove BKL from remote_llseek

In reply to Christoph Hellwig
Quoted 5 lines

Your new remote_llseek_unlocked is the same as generic_file_llseek except for the fact that it doesn't take i_mutex. So I'd rather call it generic_file_llseek_nolock and make generic_file_llseek call it.

Done. There's also default_llseek btw which is nearly similar, but i didn't touch that one.

Quoted 2 lines

While you're at it you could also add some kerneldoc documents describing them.

Off topic for the series, left that to someone else.

-Andi

#10 lore raw

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

In reply to Andi Kleen
Quoted 6 lines · On Mon, 19 May 2008 16:43:05 +0200 Andi Kleen <[email protected]> wrote:
Quoted 3 lines

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

author
#11 lore raw

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

Quoted 11 lines · Alan Cox
Quoted 4 lines · On Mon, 19 May 2008 16:43:05 +0200 Andi Kleen <[email protected]> wrote:
Quoted 2 lines

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

#12 lore raw

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

Quoted 2 lines

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

author
#13 lore raw

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

Quoted 6 lines · Alan Cox
Quoted 2 lines

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

#6 lore raw

Re: [PATCH] [1/11] Remove BKL from remote_llseek

In reply to Andi Kleen

Acked. The cifs change looks fine to me.

Quoted 193 lines · Andi Kleen

- Replace remote_llseek with remote_llseek_unlocked (to force compilation failures in all users)
- Change all users to either use remote_llseek directly or take the BKL around. I changed the file systems who don't use the BKL for anything (CIFS, GFS) to call it directly. NCPFS and SMBFS and NFS take the BKL, but explicitely in their own source now.

I moved them all over in a single patch to avoid unbisectable sections.

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.

---
 fs/cifs/cifsfs.c   |    2 +-
 fs/gfs2/ops_file.c |    4 ++--
 fs/ncpfs/file.c    |   12 +++++++++++-
 fs/nfs/file.c      |    6 +++++-
 fs/read_write.c    |    7 +++----
 fs/smbfs/file.c    |   11 ++++++++++-
 include/linux/fs.h |    3 ++-
 7 files changed, 34 insertions(+), 11 deletions(-)

Index: linux/fs/cifs/cifsfs.c
===================================================================

fs/cifs/cifsfs.c +1 -1
@@ -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 remote_llseek_unlocked(file, offset, origin);
 }
 
 struct file_system_type cifs_fs_type = {

Index: linux/fs/gfs2/ops_file.c
===================================================================

fs/gfs2/ops_file.c +2 -2
@@ -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 = remote_llseek_unlocked(file, offset, origin);
                       gfs2_glock_dq_uninit(&i_gh);
               }
       } else
-               error = remote_llseek(file, offset, origin);
+               error = remote_llseek_unlocked(file, offset, origin);
 
       return error;
 }

Index: linux/fs/ncpfs/file.c
===================================================================

fs/ncpfs/file.c +11 -1
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/sched.h>
+#include <linux/smp_lock.h>
 
 #include <linux/ncp_fs.h>
 #include "ncplib_kernel.h"
@@ -281,9 +282,18 @@ static int ncp_release(struct inode *ino
       return 0;
 }
 
+static loff_t ncp_remote_llseek(struct file *file, loff_t offset, int origin)
+{
+       loff_t ret;
+       lock_kernel();
+       ret = remote_llseek_unlocked(file, offset, origin);
+       unlock_kernel();
+       return ret;
+}
+
 const struct file_operations ncp_file_operations =
 {
-       .llseek         = remote_llseek,
+       .llseek         = ncp_remote_llseek,
       .read           = ncp_file_read,
       .write          = ncp_file_write,
       .ioctl          = ncp_ioctl,

Index: linux/fs/read_write.c
===================================================================

fs/read_write.c +3 -4
@@ -58,11 +58,10 @@ loff_t generic_file_llseek(struct file *
 
 EXPORT_SYMBOL(generic_file_llseek);
 
-loff_t remote_llseek(struct file *file, loff_t offset, int origin)
+loff_t remote_llseek_unlocked(struct file *file, loff_t offset, int origin)
 {
       loff_t retval;
 
-       lock_kernel();
       switch (origin) {
               case SEEK_END:
                       offset += i_size_read(file->f_path.dentry->d_inode);
@@ -73,15 +72,15 @@ loff_t remote_llseek(struct file *file,
       retval = -EINVAL;
       if (offset>=0 && offset<=file->f_path.dentry->d_inode->i_sb->s_maxbytes) {
               if (offset != file->f_pos) {
+                       /* AK: do we need a lock for those? */
                       file->f_pos = offset;
                       file->f_version = 0;
               }
               retval = offset;
       }
-       unlock_kernel();
       return retval;
 }
-EXPORT_SYMBOL(remote_llseek);
+EXPORT_SYMBOL(remote_llseek_unlocked);
 
 loff_t no_llseek(struct file *file, loff_t offset, int origin)
 {

Index: linux/fs/smbfs/file.c
===================================================================

fs/smbfs/file.c +10 -1
@@ -422,9 +422,18 @@ smb_file_permission(struct inode *inode,
       return error;
 }
 
+static loff_t smb_remote_llseek(struct file *file, loff_t offset, int origin)
+{
+       loff_t ret;
+       lock_kernel();
+       ret = remote_llseek_unlocked(file, offset, origin);
+       unlock_kernel();
+       return ret;
+}
+
 const struct file_operations smb_file_operations =
 {
-       .llseek         = remote_llseek,
+       .llseek         = smb_remote_llseek,
       .read           = do_sync_read,
       .aio_read       = smb_file_aio_read,
       .write          = do_sync_write,

Index: linux/include/linux/fs.h
===================================================================

include/linux/fs.h +2 -1
@@ -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 remote_llseek_unlocked(struct file *file, loff_t offset,
+                       int origin);
 extern int generic_file_open(struct inode * inode, struct file * filp);
 extern int nonseekable_open(struct inode * inode, struct file * filp);
 

Index: linux/fs/nfs/file.c
===================================================================

fs/nfs/file.c +5 -1
@@ -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 = remote_llseek_unlocked(filp, offset, origin);
+       unlock_kernel();
+       return loff;
 }
 
 /*
Signature · 3 lines
Thanks,

Steve
#14 lore raw

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

In reply to Andi Kleen
Quoted 13 lines · On Mon, 19 May 2008 14:31:11 +0200 (CEST) Andi Kleen wrote:

Index: linux/Documentation/filesystems/vfs.txt
===================================================================

Documentation/filesystems/vfs.txt +1 -0
@@ -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 *);

Documentation/filesystems/vfs.txt otherwise noted. · line 818

Quoted 5 lines
Documentation/filesystems/vfs.txt +1 -1
@@ -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.

Documentation/filesystems/vfs.txt otherwise noted. · line 823

Quoted 5 lines
Documentation/filesystems/vfs.txt +2 -0
+
+  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