From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andreas Gruenbacher Subject: xattr iops locking improvement Date: Mon, 7 Jul 2003 19:48:04 +0200 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <200307071948.04703.agruen@suse.de> Mime-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_UJbC/2mqRKwOR+Q" Cc: linux-fsdevel@vger.kernel.org, Andrew Morton , Andrea Arcangeli , Dave Kleikamp , Nathan Scott , Chris Mason , Tim Shimmin , Jeff Mahoney , Steve Best Return-path: Received: from ns.suse.de ([213.95.15.193]:18195 "EHLO Cantor.suse.de") by vger.kernel.org with ESMTP id S264124AbTGGRdf (ORCPT ); Mon, 7 Jul 2003 13:33:35 -0400 To: Marcelo Tossati List-Id: linux-fsdevel.vger.kernel.org --Boundary-00=_UJbC/2mqRKwOR+Q Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 --Boundary-00=_UJbC/2mqRKwOR+Q Content-Type: text/x-diff; charset="us-ascii"; name="xattr-fine-grained-locking-1" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xattr-fine-grained-locking-1" 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 , 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. --Boundary-00=_UJbC/2mqRKwOR+Q Content-Type: text/x-diff; charset="us-ascii"; name="xattr-fine-grained-locking-2" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xattr-fine-grained-locking-2" 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; } /* --Boundary-00=_UJbC/2mqRKwOR+Q Content-Type: text/x-diff; charset="us-ascii"; name="xattr-fine-grained-locking-3" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xattr-fine-grained-locking-3" 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, --Boundary-00=_UJbC/2mqRKwOR+Q--