xattr iops locking improvement

xattr iops locking improvement

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

Hi Marcelo,

Andrew Morton has found that taking i_sem for all xattr iops leads to lock 
contention. Particularly annoying is that writers can block operations like 
`ls -l' because both need i_sem. We have bene working on a fix for 2.4 and 
2.5. The solution is a series of patches that are in Andrew's current tree, 
and have been sent to Linus. I will also shortly put them on 
http://acl.bestbits.at/.

For vanilla 2.4 the relevant patch is xattr-fine-grained-locking-1. i_sem is 
no longer taken in the listxattr and getxattr iops. It is still taken for 
setxattr and removexattr. The patch moves the lock taking down to the file 
system level.

I have patches ready that remove the need for i_sem in ext2 and ext3, and I'm 
sure the xfs, jfs, and reiserfs coders will be quick to follow up with 
improved xattr and acl patches as well.

I am attaching xattr-fine-grained-locking-2 and xattr-fine-grained-locking-3 
for reference.


Cheers,
Andreas.

------------------------------------------------------------------
 Andreas Gruenbacher                                SuSE Linux AG
 mailto:agruen@suse.de                     Deutschherrnstr. 15-19
 http://www.suse.de/                   D-90429 Nuernberg, Germany

[-- Attachment #2: xattr-fine-grained-locking-1 --]
[-- Type: text/x-diff, Size: 3138 bytes --]

More fine-grained locking for extended attributes

No longer take inode->i_sem for iops->getxattr and iops->listxattr.
Move taking inode->i_sem down to the file system level.

Andreas Gruenbacher <agruen@suse.de>, SuSE Labs


Index: linux-2.4.21/fs/xattr.c
===================================================================
--- linux-2.4.21.orig/fs/xattr.c	2002-11-29 00:53:15.000000000 +0100
+++ linux-2.4.21/fs/xattr.c	2003-06-25 22:03:05.000000000 +0200
@@ -159,11 +159,9 @@ getxattr(struct dentry *d, char *name, v
 
 	error = -EOPNOTSUPP;
 	if (d->d_inode->i_op && d->d_inode->i_op->getxattr) {
-		down(&d->d_inode->i_sem);
 		lock_kernel();
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
 		unlock_kernel();
-		up(&d->d_inode->i_sem);
 	}
 
 	if (kvalue && error > 0)
@@ -230,11 +228,9 @@ listxattr(struct dentry *d, char *list, 
 
 	error = -EOPNOTSUPP;
 	if (d->d_inode->i_op && d->d_inode->i_op->listxattr) {
-		down(&d->d_inode->i_sem);
 		lock_kernel();
 		error = d->d_inode->i_op->listxattr(d, klist, size);
 		unlock_kernel();
-		up(&d->d_inode->i_sem);
 	}
 
 	if (klist && error > 0)
Index: linux-2.4.21/fs/jfs/xattr.c
===================================================================
--- linux-2.4.21.orig/fs/jfs/xattr.c	2003-06-25 22:00:29.000000000 +0200
+++ linux-2.4.21/fs/jfs/xattr.c	2003-06-25 22:17:53.000000000 +0200
@@ -964,12 +964,18 @@ ssize_t __jfs_getxattr(struct inode *ino
 ssize_t jfs_getxattr(struct dentry *dentry, const char *name, void *data,
 		     size_t buf_size)
 {
-	return __jfs_getxattr(dentry->d_inode, name, data, buf_size);
+	int err;
+	
+	down(&dentry->d_inode->i_sem);
+	err = __jfs_getxattr(dentry->d_inode, name, data, buf_size);
+	up(&dentry->d_inode->i_sem);
+	
+	return err;
 }
 
-ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size)
+static ssize_t __jfs_listxattr(struct inode *inode, char *data,
+			       size_t buf_size)
 {
-	struct inode *inode = dentry->d_inode;
 	char *buffer;
 	ssize_t size = 0;
 	int xattr_size;
@@ -1013,6 +1019,17 @@ ssize_t jfs_listxattr(struct dentry * de
 	return size;
 }
 
+ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size)
+{
+	int err;
+	
+	down(&dentry->d_inode->i_sem);
+	err = __jfs_listxattr(dentry->d_inode, data, buf_size);
+	up(&dentry->d_inode->i_sem);
+	
+	return err;
+}
+
 int jfs_removexattr(struct dentry *dentry, const char *name)
 {
 	return __jfs_setxattr(dentry->d_inode, name, 0, 0, XATTR_REPLACE);
Index: linux-2.4.21/Documentation/filesystems/Locking
===================================================================
--- linux-2.4.21.orig/Documentation/filesystems/Locking	2003-06-25 21:58:40.000000000 +0200
+++ linux-2.4.21/Documentation/filesystems/Locking	2003-06-25 22:03:05.000000000 +0200
@@ -69,8 +69,8 @@ permission:	yes	no		no
 getattr:						(see below)
 revalidate:	no					(see below)
 setxattr:	yes	yes		no
-getxattr:	yes	yes		no
-listxattr:	yes	yes		no
+getxattr:	yes	no		no
+listxattr:	yes	no		no
 removexattr:	yes	yes		no
 	Additionally, ->rmdir() has i_zombie on victim and so does ->rename()
 in case when target exists and is a directory.

[-- Attachment #3: xattr-fine-grained-locking-2 --]
[-- Type: text/x-diff, Size: 2066 bytes --]

Index: linux-2.4.21/fs/ext2/xattr.c
===================================================================
--- linux-2.4.21.orig/fs/ext2/xattr.c	2003-06-25 15:29:13.000000000 +0200
+++ linux-2.4.21/fs/ext2/xattr.c	2003-06-25 15:59:05.000000000 +0200
@@ -290,11 +290,17 @@ ext2_getxattr(struct dentry *dentry, con
 {
 	struct ext2_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
+	ssize_t error;
 
 	handler = ext2_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size);
+
+	down(&inode->i_sem);
+	error = handler->get(inode, name, buffer, size);
+	up(&inode->i_sem);
+
+	return error;
 }
 
 /*
@@ -306,7 +312,13 @@ ext2_getxattr(struct dentry *dentry, con
 ssize_t
 ext2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext2_xattr_list(dentry->d_inode, buffer, size);
+	ssize_t error;
+
+	down(&dentry->d_inode->i_sem);
+	error = ext2_xattr_list(dentry->d_inode, buffer, size);
+	up(&dentry->d_inode->i_sem);
+
+	return error;
 }
 
 /*
Index: linux-2.4.21/fs/ext3/xattr.c
===================================================================
--- linux-2.4.21.orig/fs/ext3/xattr.c	2003-06-25 15:29:13.000000000 +0200
+++ linux-2.4.21/fs/ext3/xattr.c	2003-06-25 15:58:50.000000000 +0200
@@ -289,11 +289,17 @@ ext3_getxattr(struct dentry *dentry, con
 {
 	struct ext3_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
+	ssize_t error;
 
 	handler = ext3_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size);
+
+	down(&inode->i_sem);
+	error = handler->get(inode, name, buffer, size);
+	up(&inode->i_sem);
+
+	return error;
 }
 
 /*
@@ -305,7 +311,13 @@ ext3_getxattr(struct dentry *dentry, con
 ssize_t
 ext3_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext3_xattr_list(dentry->d_inode, buffer, size);
+	ssize_t error;
+
+	down(&dentry->d_inode->i_sem);
+	error = ext3_xattr_list(dentry->d_inode, buffer, size);
+	up(&dentry->d_inode->i_sem);
+
+	return error;
 }
 
 /*

[-- Attachment #4: xattr-fine-grained-locking-3 --]
[-- Type: text/x-diff, Size: 2843 bytes --]

Index: linux-2.4.21/fs/reiserfs/xattr.c
===================================================================
--- linux-2.4.21.orig/fs/reiserfs/xattr.c	2003-06-25 15:29:19.000000000 +0200
+++ linux-2.4.21/fs/reiserfs/xattr.c	2003-06-25 16:04:39.000000000 +0200
@@ -1322,7 +1322,9 @@ reiserfs_getxattr (struct dentry *dentry
         return -EOPNOTSUPP;
     
     reiserfs_read_lock_xattrs (dentry->d_sb);
+    down(&dentry->d_inode->i_sem);
     err = xah->get (dentry->d_inode, name, buffer, size);
+    up(&dentry->d_inode->i_sem);
     reiserfs_read_unlock_xattrs (dentry->d_sb);
     return err;
 }
@@ -1426,14 +1428,9 @@ reiserfs_listxattr_filler (void *buf, co
 
     return 0;
 }
-/*
- * Inode operation listxattr()
- *
- * dentry->d_inode->i_sem down
- * BKL held [before 2.5.x]
- */
-ssize_t
-reiserfs_listxattr (struct dentry *dentry, char *buffer, size_t size)
+
+static ssize_t
+__reiserfs_listxattr (struct dentry *dentry, char *buffer, size_t size)
 {
     struct file *fp;
     struct dentry *dir;
@@ -1485,6 +1482,24 @@ out:
     return err;
 }
 
+/*
+ * Inode operation listxattr()
+ *
+ * dentry->d_inode->i_sem down
+ * BKL held [before 2.5.x]
+ */
+ssize_t
+reiserfs_listxattr (struct dentry *dentry, char *buffer, size_t size)
+{
+    int error;
+
+    down(&dentry->d_inode->i_sem);
+    error = __reiserfs_listxattr(dentry, buffer, size);
+    up(&dentry->d_inode->i_sem);
+
+    return error;
+}
+
 /* This is the implementation for the xattr plugin infrastructure */
 static struct reiserfs_xattr_handler *xattr_handlers;
 static rwlock_t handler_lock = RW_LOCK_UNLOCKED;
Index: linux-2.4.21/fs/xfs/linux/xfs_iops.c
===================================================================
--- linux-2.4.21.orig/fs/xfs/linux/xfs_iops.c	2003-06-25 15:28:43.000000000 +0200
+++ linux-2.4.21/fs/xfs/linux/xfs_iops.c	2003-06-25 16:13:33.000000000 +0200
@@ -644,7 +644,7 @@ linvfs_setxattr(
 }
 
 STATIC ssize_t
-linvfs_getxattr(
+__linvfs_getxattr(
 	struct dentry	*dentry,
 	const char	*name,
 	void		*data,
@@ -699,9 +699,25 @@ linvfs_getxattr(
 	return -EOPNOTSUPP;
 }
 
+STATIC ssize_t
+linvfs_getxattr(
+	struct dentry	*dentry,
+	const char	*name,
+	void		*data,
+	size_t		size)
+{
+	int error;
+
+	down(&dentry->d_inode->i_sem);
+	error = __linvfs_getxattr(dentry, name, data, size);
+	up(&dentry->d_inode->i_sem);
+
+	return error;
+}
+
 
 STATIC ssize_t
-linvfs_listxattr(
+__linvfs_listxattr(
 	struct dentry		*dentry,
 	char			*data,
 	size_t			size)
@@ -743,6 +759,21 @@ linvfs_listxattr(
 	return result;
 }
 
+STATIC ssize_t
+linvfs_listxattr(
+	struct dentry		*dentry,
+	char			*data,
+	size_t			size)
+{
+	int error;
+
+	down(&dentry->d_inode->i_sem);
+	error = __linvfs_listxattr(dentry, data, size);
+	up(&dentry->d_inode->i_sem);
+
+	return error;
+}
+
 STATIC int
 linvfs_removexattr(
 	struct dentry	*dentry,

Re: xattr iops locking improvement

[Christoph Hellwig]

On Mon, Jul 07, 2003 at 07:48:04PM +0200, Andreas Gruenbacher wrote:
> Hi Marcelo,
> 
> Andrew Morton has found that taking i_sem for all xattr iops leads to lock 
> contention. Particularly annoying is that writers can block operations like 
> `ls -l' because both need i_sem. We have bene working on a fix for 2.4 and 
> 2.5. The solution is a series of patches that are in Andrew's current tree, 
> and have been sent to Linus. I will also shortly put them on 
> http://acl.bestbits.at/.

~o(/me remembers having argued for no VFS-level locking on them a while ago)

Can we please get rid of the i_sem on the other two operations, and finally
move all locking to the implementation?