Skip to contentSkip to conversation

xattr iops locking improvement

PATCH Andreas Gruenbacher 2 participants 2 messages Jul 7, 2003

Subsystems (5)
Documentation VFS XFS filesystem ext2 file system +1 more subsystems
lore
Share & tools
lore

Andreas Gruenbacher

author
#0 lore raw

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:[email protected] Deutschherrnstr. 15-19
http://www.suse.de/ D-90429 Nuernberg, Germany

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 <[email protected]>, SuSE Labs

fs/xattr.c +0 -4
Index: linux-2.4.21/fs/xattr.c
===================================================================
@@ -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)
fs/jfs/xattr.c +20 -3
Index: linux-2.4.21/fs/jfs/xattr.c
===================================================================
@@ -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);
Documentation/filesystems/Locking +2 -2
Index: linux-2.4.21/Documentation/filesystems/Locking
===================================================================
@@ -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.
fs/ext2/xattr.c +14 -2
Index: linux-2.4.21/fs/ext2/xattr.c
===================================================================
@@ -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;
 }
 
 /*
fs/ext3/xattr.c +14 -2
Index: linux-2.4.21/fs/ext3/xattr.c
===================================================================
@@ -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;
 }
 
 /*
fs/reiserfs/xattr.c +23 -8
Index: linux-2.4.21/fs/reiserfs/xattr.c
===================================================================
@@ -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;
fs/xfs/linux/xfs_iops.c +33 -2
Index: linux-2.4.21/fs/xfs/linux/xfs_iops.c
===================================================================
@@ -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,
#1 lore raw
Quoted 8 lines · Andreas Gruenbacher

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?