[PATCH] xattrr: preparation for fine-grained locking

From: Andreas Gruenbacher <agruen@suse.de>

Andrew Morton found that there is lock contention between extended
attribute operations (like reading ACLs, which `ls -l' needs to do)
and other operations on the same files. This is due to the fact that
all extended attribute syscalls take inode->i_sem before calling into
the filesystem code.

To fix this problem, this patch no longer takes inode->i_sem in the
getxattr and listxattr syscalls, and moves the lock taking code into
the file systems. (Another patch improves the locking strategy in
ext2 and ext3.)

Documentation/filesystems/Locking

@@ -68,8 +68,8 @@ setattr:	yes
 permission:	no
 getattr:	no
 setxattr:	yes
-getxattr:	yes
-listxattr:	yes
+getxattr:	no
+listxattr:	no
 removexattr:	yes
 	Additionally, ->rmdir(), ->unlink() and ->rename() have ->i_sem on
 victim.

fs/ext2/xattr.c

@@ -204,11 +204,16 @@ ext2_getxattr(struct dentry *dentry, const char *name,
 {
 	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;
 }
 
 /*
@@ -219,7 +224,13 @@ ext2_getxattr(struct dentry *dentry, const char *name,
 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

@@ -199,11 +199,16 @@ ext3_getxattr(struct dentry *dentry, const char *name,
 {
 	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;
 }
 
 /*
@@ -214,7 +219,13 @@ ext3_getxattr(struct dentry *dentry, const char *name,
 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/jfs/xattr.c

@@ -964,10 +964,17 @@ ssize_t __jfs_getxattr(struct inode *inode, const char *name, void *data,
 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 dentry * dentry, char *data,
+			       size_t buf_size)
 {
 	struct inode *inode = dentry->d_inode;
 	char *buffer;
@@ -1013,6 +1020,17 @@ ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size)
 	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, 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);

fs/xattr.c

@@ -160,9 +160,7 @@ getxattr(struct dentry *d, char *name, void *value, size_t size)
 		error = security_inode_getxattr(d, kname);
 		if (error)
 			goto out;
-		down(&d->d_inode->i_sem);
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
-		up(&d->d_inode->i_sem);
 	}
 
 	if (kvalue && error > 0)
@@ -233,9 +231,7 @@ listxattr(struct dentry *d, char *list, size_t size)
 		error = security_inode_listxattr(d);
 		if (error)
 			goto out;
-		down(&d->d_inode->i_sem);
 		error = d->d_inode->i_op->listxattr(d, klist, size);
-		up(&d->d_inode->i_sem);
 	}
 
 	if (klist && error > 0)

fs/xfs/linux/xfs_iops.c

@@ -642,7 +642,7 @@ linvfs_setxattr(
 }
 
 STATIC ssize_t
-linvfs_getxattr(
+__linvfs_getxattr(
 	struct dentry	*dentry,
 	const char	*name,
 	void		*data,
@@ -697,9 +697,24 @@ 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)
@@ -741,6 +756,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,