reiserfs: eliminate per-super xattr lock
Jeff Mahoney [Mon, 30 Mar 2009 18:02:35 +0000 (14:02 -0400)]
With the switch to using inode->i_mutex locking during lookups/creation
in the xattr root, the per-super xattr lock is no longer needed.

This patch removes it.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

fs/reiserfs/inode.c
fs/reiserfs/namei.c
fs/reiserfs/super.c
fs/reiserfs/xattr.c
fs/reiserfs/xattr_acl.c
include/linux/reiserfs_fs.h
include/linux/reiserfs_fs_sb.h
include/linux/reiserfs_xattr.h

index cd42a86..50a73e7 100644 (file)
@@ -1957,19 +1957,7 @@ int reiserfs_new_inode(struct reiserfs_transaction_handle *th,
        inode->i_nlink = 0;
        th->t_trans_id = 0;     /* so the caller can't use this handle later */
        unlock_new_inode(inode); /* OK to do even if we hadn't locked it */
-
-       /* If we were inheriting an ACL, we need to release the lock so that
-        * iput doesn't deadlock in reiserfs_delete_xattrs. The locking
-        * code really needs to be reworked, but this will take care of it
-        * for now. -jeffm */
-#ifdef CONFIG_REISERFS_FS_POSIX_ACL
-       if (REISERFS_I(dir)->i_acl_default && !IS_ERR(REISERFS_I(dir)->i_acl_default)) {
-               reiserfs_write_unlock_xattrs(dir->i_sb);
-               iput(inode);
-               reiserfs_write_lock_xattrs(dir->i_sb);
-       } else
-#endif
-               iput(inode);
+       iput(inode);
        return err;
 }
 
index c8430f1..ddf1bcd 100644 (file)
@@ -609,9 +609,6 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
 
        reiserfs_write_lock(dir->i_sb);
 
-       if (locked)
-               reiserfs_write_lock_xattrs(dir->i_sb);
-
        retval = journal_begin(&th, dir->i_sb, jbegin_count);
        if (retval) {
                drop_new_inode(inode);
@@ -624,11 +621,6 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
        if (retval)
                goto out_failed;
 
-       if (locked) {
-               reiserfs_write_unlock_xattrs(dir->i_sb);
-               locked = 0;
-       }
-
        inode->i_op = &reiserfs_file_inode_operations;
        inode->i_fop = &reiserfs_file_operations;
        inode->i_mapping->a_ops = &reiserfs_address_space_operations;
@@ -655,8 +647,6 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
        retval = journal_end(&th, dir->i_sb, jbegin_count);
 
       out_failed:
-       if (locked)
-               reiserfs_write_unlock_xattrs(dir->i_sb);
        reiserfs_write_unlock(dir->i_sb);
        return retval;
 }
@@ -686,9 +676,6 @@ static int reiserfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
 
        reiserfs_write_lock(dir->i_sb);
 
-       if (locked)
-               reiserfs_write_lock_xattrs(dir->i_sb);
-
        retval = journal_begin(&th, dir->i_sb, jbegin_count);
        if (retval) {
                drop_new_inode(inode);
@@ -702,11 +689,6 @@ static int reiserfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
                goto out_failed;
        }
 
-       if (locked) {
-               reiserfs_write_unlock_xattrs(dir->i_sb);
-               locked = 0;
-       }
-
        inode->i_op = &reiserfs_special_inode_operations;
        init_special_inode(inode, inode->i_mode, rdev);
 
@@ -736,8 +718,6 @@ static int reiserfs_mknod(struct inode *dir, struct dentry *dentry, int mode,
        retval = journal_end(&th, dir->i_sb, jbegin_count);
 
       out_failed:
-       if (locked)
-               reiserfs_write_unlock_xattrs(dir->i_sb);
        reiserfs_write_unlock(dir->i_sb);
        return retval;
 }
@@ -767,8 +747,6 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
        locked = reiserfs_cache_default_acl(dir);
 
        reiserfs_write_lock(dir->i_sb);
-       if (locked)
-               reiserfs_write_lock_xattrs(dir->i_sb);
 
        retval = journal_begin(&th, dir->i_sb, jbegin_count);
        if (retval) {
@@ -790,11 +768,6 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
                goto out_failed;
        }
 
-       if (locked) {
-               reiserfs_write_unlock_xattrs(dir->i_sb);
-               locked = 0;
-       }
-
        reiserfs_update_inode_transaction(inode);
        reiserfs_update_inode_transaction(dir);
 
@@ -824,8 +797,6 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
        unlock_new_inode(inode);
        retval = journal_end(&th, dir->i_sb, jbegin_count);
       out_failed:
-       if (locked)
-               reiserfs_write_unlock_xattrs(dir->i_sb);
        reiserfs_write_unlock(dir->i_sb);
        return retval;
 }
index fc7cb46..6d10f81 100644 (file)
@@ -1646,10 +1646,6 @@ static int reiserfs_fill_super(struct super_block *s, void *data, int silent)
        REISERFS_SB(s)->s_alloc_options.preallocmin = 0;
        /* Preallocate by 16 blocks (17-1) at once */
        REISERFS_SB(s)->s_alloc_options.preallocsize = 17;
-#ifdef CONFIG_REISERFS_FS_XATTR
-       /* Initialize the rwsem for xattr dir */
-       init_rwsem(&REISERFS_SB(s)->xattr_dir_sem);
-#endif
        /* setup default block allocator options */
        reiserfs_init_alloc_options(s);
 
index 57920a4..62c9882 100644 (file)
  * these are special cases for filesystem ACLs, they are interpreted by the
  * kernel, in addition, they are negatively and positively cached and attached
  * to the inode so that unnecessary lookups are avoided.
+ *
+ * Locking works like so:
+ * The xattr root (/.reiserfs_priv/xattrs) is protected by its i_mutex.
+ * The xattr dir (/.reiserfs_priv/xattrs/<oid>.<gen>) is protected by
+ * inode->xattr_sem.
+ * The xattrs themselves are likewise protected by the xattr_sem.
  */
 
 #include <linux/reiserfs_fs.h>
@@ -392,16 +398,17 @@ reiserfs_delete_xattrs_filler(void *buf, const char *name, int namelen,
 /* This is called w/ inode->i_mutex downed */
 int reiserfs_delete_xattrs(struct inode *inode)
 {
-       struct dentry *dir, *root;
        int err = 0;
+       struct dentry *dir, *root;
+       struct reiserfs_transaction_handle th;
+       int blocks = JOURNAL_PER_BALANCE_CNT * 2 + 2 +
+                    4 * REISERFS_QUOTA_TRANS_BLOCKS(inode->i_sb);
 
        /* Skip out, an xattr has no xattrs associated with it */
        if (IS_PRIVATE(inode) || get_inode_sd_version(inode) == STAT_DATA_V1)
                return 0;
 
-       reiserfs_read_lock_xattrs(inode->i_sb);
        dir = open_xa_dir(inode, XATTR_REPLACE);
-       reiserfs_read_unlock_xattrs(inode->i_sb);
        if (IS_ERR(dir)) {
                err = PTR_ERR(dir);
                goto out;
@@ -416,18 +423,26 @@ int reiserfs_delete_xattrs(struct inode *inode)
        if (err)
                goto out_dir;
 
-       /* Leftovers besides . and .. -- that's not good. */
-       if (dir->d_inode->i_nlink <= 2) {
-               root = open_xa_root(inode->i_sb, XATTR_REPLACE);
-               reiserfs_write_lock_xattrs(inode->i_sb);
+       /* We start a transaction here to avoid a ABBA situation
+        * between the xattr root's i_mutex and the journal lock.
+        * Inode creation will inherit an ACL, which requires a
+        * lookup. The lookup locks the xattr root i_mutex with a
+        * transaction open.  Inode deletion takes teh xattr root
+        * i_mutex to delete the directory and then starts a
+        * transaction inside it. Boom. This doesn't incur much
+        * additional overhead since the reiserfs_rmdir transaction
+        * will just nest inside the outer transaction. */
+       err = journal_begin(&th, inode->i_sb, blocks);
+       if (!err) {
+               int jerror;
+               root = dget(dir->d_parent);
                mutex_lock_nested(&root->d_inode->i_mutex, I_MUTEX_XATTR);
                err = xattr_rmdir(root->d_inode, dir);
+               jerror = journal_end(&th, inode->i_sb, blocks);
                mutex_unlock(&root->d_inode->i_mutex);
-               reiserfs_write_unlock_xattrs(inode->i_sb);
                dput(root);
-       } else {
-               reiserfs_warning(inode->i_sb, "jdm-20006",
-                                "Couldn't remove all entries in directory");
+
+               err = jerror ?: err;
        }
 
 out_dir:
@@ -437,6 +452,9 @@ out:
        if (!err)
                REISERFS_I(inode)->i_flags =
                    REISERFS_I(inode)->i_flags & ~i_has_xattr_dir;
+       else
+               reiserfs_warning(inode->i_sb, "jdm-20004",
+                                "Couldn't remove all xattrs (%d)\n", err);
        return err;
 }
 
@@ -485,9 +503,7 @@ int reiserfs_chown_xattrs(struct inode *inode, struct iattr *attrs)
        if (IS_PRIVATE(inode) || get_inode_sd_version(inode) == STAT_DATA_V1)
                return 0;
 
-       reiserfs_read_lock_xattrs(inode->i_sb);
        dir = open_xa_dir(inode, XATTR_REPLACE);
-       reiserfs_read_unlock_xattrs(inode->i_sb);
        if (IS_ERR(dir)) {
                if (PTR_ERR(dir) != -ENODATA)
                        err = PTR_ERR(dir);
@@ -731,6 +747,11 @@ reiserfs_xattr_get(const struct inode *inode, const char *name, void *buffer,
                goto out;
        }
 
+       /* protect against concurrent access. xattrs are backed by
+        * regular files, but they're not regular files. The updates
+        * must be atomic from the perspective of the user. */
+       mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_XATTR);
+
        isize = i_size_read(dentry->d_inode);
        REISERFS_I(inode)->i_flags |= i_has_xattr_dir;
 
@@ -798,6 +819,7 @@ reiserfs_xattr_get(const struct inode *inode, const char *name, void *buffer,
        }
 
 out_dput:
+       mutex_unlock(&dentry->d_inode->i_mutex);
        dput(dentry);
 
 out:
@@ -834,7 +856,6 @@ int reiserfs_xattr_del(struct inode *inode, const char *name)
 static struct reiserfs_xattr_handler *find_xattr_handler_prefix(const char *);
 /*
  * Inode operation getxattr()
- * Preliminary locking: we down dentry->d_inode->i_mutex
  */
 ssize_t
 reiserfs_getxattr(struct dentry * dentry, const char *name, void *buffer,
@@ -848,9 +869,7 @@ reiserfs_getxattr(struct dentry * dentry, const char *name, void *buffer,
                return -EOPNOTSUPP;
 
        reiserfs_read_lock_xattr_i(dentry->d_inode);
-       reiserfs_read_lock_xattrs(dentry->d_sb);
        err = xah->get(dentry->d_inode, name, buffer, size);
-       reiserfs_read_unlock_xattrs(dentry->d_sb);
        reiserfs_read_unlock_xattr_i(dentry->d_inode);
        return err;
 }
@@ -866,23 +885,13 @@ reiserfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 {
        struct reiserfs_xattr_handler *xah = find_xattr_handler_prefix(name);
        int err;
-       int lock;
 
        if (!xah || !reiserfs_xattrs(dentry->d_sb) ||
            get_inode_sd_version(dentry->d_inode) == STAT_DATA_V1)
                return -EOPNOTSUPP;
 
        reiserfs_write_lock_xattr_i(dentry->d_inode);
-       lock = !has_xattr_dir(dentry->d_inode);
-       if (lock)
-               reiserfs_write_lock_xattrs(dentry->d_sb);
-       else
-               reiserfs_read_lock_xattrs(dentry->d_sb);
        err = xah->set(dentry->d_inode, name, value, size, flags);
-       if (lock)
-               reiserfs_write_unlock_xattrs(dentry->d_sb);
-       else
-               reiserfs_read_unlock_xattrs(dentry->d_sb);
        reiserfs_write_unlock_xattr_i(dentry->d_inode);
        return err;
 }
@@ -902,8 +911,6 @@ int reiserfs_removexattr(struct dentry *dentry, const char *name)
                return -EOPNOTSUPP;
 
        reiserfs_write_lock_xattr_i(dentry->d_inode);
-       reiserfs_read_lock_xattrs(dentry->d_sb);
-
        /* Deletion pre-operation */
        if (xah->del) {
                err = xah->del(dentry->d_inode, name);
@@ -917,7 +924,6 @@ int reiserfs_removexattr(struct dentry *dentry, const char *name)
        mark_inode_dirty(dentry->d_inode);
 
       out:
-       reiserfs_read_unlock_xattrs(dentry->d_sb);
        reiserfs_write_unlock_xattr_i(dentry->d_inode);
        return err;
 }
@@ -966,8 +972,6 @@ reiserfs_listxattr_filler(void *buf, const char *name, int namelen,
 
 /*
  * Inode operation listxattr()
- *
- * Preliminary locking: we down dentry->d_inode->i_mutex
  */
 ssize_t reiserfs_listxattr(struct dentry * dentry, char *buffer, size_t size)
 {
@@ -983,9 +987,7 @@ ssize_t reiserfs_listxattr(struct dentry * dentry, char *buffer, size_t size)
                return -EOPNOTSUPP;
 
        reiserfs_read_lock_xattr_i(dentry->d_inode);
-       reiserfs_read_lock_xattrs(dentry->d_sb);
        dir = open_xa_dir(dentry->d_inode, XATTR_REPLACE);
-       reiserfs_read_unlock_xattrs(dentry->d_sb);
        if (IS_ERR(dir)) {
                err = PTR_ERR(dir);
                if (err == -ENODATA)
@@ -1114,11 +1116,9 @@ static int reiserfs_check_acl(struct inode *inode, int mask)
        int error = -EAGAIN; /* do regular unix permission checks by default */
 
        reiserfs_read_lock_xattr_i(inode);
-       reiserfs_read_lock_xattrs(inode->i_sb);
 
        acl = reiserfs_get_acl(inode, ACL_TYPE_ACCESS);
 
-       reiserfs_read_unlock_xattrs(inode->i_sb);
        reiserfs_read_unlock_xattr_i(inode);
 
        if (acl) {
index 9128e4d..d63b2c5 100644 (file)
@@ -172,6 +172,29 @@ static void *posix_acl_to_disk(const struct posix_acl *acl, size_t * size)
        return ERR_PTR(-EINVAL);
 }
 
+static inline void iset_acl(struct inode *inode, struct posix_acl **i_acl,
+                           struct posix_acl *acl)
+{
+       spin_lock(&inode->i_lock);
+       if (*i_acl != ERR_PTR(-ENODATA))
+               posix_acl_release(*i_acl);
+       *i_acl = posix_acl_dup(acl);
+       spin_unlock(&inode->i_lock);
+}
+
+static inline struct posix_acl *iget_acl(struct inode *inode,
+                                        struct posix_acl **i_acl)
+{
+       struct posix_acl *acl = ERR_PTR(-ENODATA);
+
+       spin_lock(&inode->i_lock);
+       if (*i_acl != ERR_PTR(-ENODATA))
+               acl = posix_acl_dup(*i_acl);
+       spin_unlock(&inode->i_lock);
+
+       return acl;
+}
+
 /*
  * Inode operation get_posix_acl().
  *
@@ -199,11 +222,11 @@ struct posix_acl *reiserfs_get_acl(struct inode *inode, int type)
                return ERR_PTR(-EINVAL);
        }
 
-       if (IS_ERR(*p_acl)) {
-               if (PTR_ERR(*p_acl) == -ENODATA)
-                       return NULL;
-       } else if (*p_acl != NULL)
-               return posix_acl_dup(*p_acl);
+       acl = iget_acl(inode, p_acl);
+       if (acl && !IS_ERR(acl))
+               return acl;
+       else if (PTR_ERR(acl) == -ENODATA)
+               return NULL;
 
        size = reiserfs_xattr_get(inode, name, NULL, 0);
        if (size < 0) {
@@ -229,7 +252,7 @@ struct posix_acl *reiserfs_get_acl(struct inode *inode, int type)
        } else {
                acl = posix_acl_from_disk(value, retval);
                if (!IS_ERR(acl))
-                       *p_acl = posix_acl_dup(acl);
+                       iset_acl(inode, p_acl, acl);
        }
 
        kfree(value);
@@ -300,16 +323,8 @@ reiserfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
 
        kfree(value);
 
-       if (!error) {
-               /* Release the old one */
-               if (!IS_ERR(*p_acl) && *p_acl)
-                       posix_acl_release(*p_acl);
-
-               if (acl == NULL)
-                       *p_acl = ERR_PTR(-ENODATA);
-               else
-                       *p_acl = posix_acl_dup(acl);
-       }
+       if (!error)
+               iset_acl(inode, p_acl, acl);
 
        return error;
 }
@@ -404,9 +419,7 @@ int reiserfs_cache_default_acl(struct inode *inode)
        if (reiserfs_posixacl(inode->i_sb) && !IS_PRIVATE(inode)) {
                struct posix_acl *acl;
                reiserfs_read_lock_xattr_i(inode);
-               reiserfs_read_lock_xattrs(inode->i_sb);
                acl = reiserfs_get_acl(inode, ACL_TYPE_DEFAULT);
-               reiserfs_read_unlock_xattrs(inode->i_sb);
                reiserfs_read_unlock_xattr_i(inode);
                ret = (acl && !IS_ERR(acl));
                if (ret)
@@ -429,9 +442,7 @@ int reiserfs_acl_chmod(struct inode *inode)
                return 0;
        }
 
-       reiserfs_read_lock_xattrs(inode->i_sb);
        acl = reiserfs_get_acl(inode, ACL_TYPE_ACCESS);
-       reiserfs_read_unlock_xattrs(inode->i_sb);
        if (!acl)
                return 0;
        if (IS_ERR(acl))
@@ -442,17 +453,8 @@ int reiserfs_acl_chmod(struct inode *inode)
                return -ENOMEM;
        error = posix_acl_chmod_masq(clone, inode->i_mode);
        if (!error) {
-               int lock = !has_xattr_dir(inode);
                reiserfs_write_lock_xattr_i(inode);
-               if (lock)
-                       reiserfs_write_lock_xattrs(inode->i_sb);
-               else
-                       reiserfs_read_lock_xattrs(inode->i_sb);
                error = reiserfs_set_acl(inode, ACL_TYPE_ACCESS, clone);
-               if (lock)
-                       reiserfs_write_unlock_xattrs(inode->i_sb);
-               else
-                       reiserfs_read_unlock_xattrs(inode->i_sb);
                reiserfs_write_unlock_xattr_i(inode);
        }
        posix_acl_release(clone);
@@ -480,14 +482,9 @@ posix_acl_access_set(struct inode *inode, const char *name,
 static int posix_acl_access_del(struct inode *inode, const char *name)
 {
        struct reiserfs_inode_info *reiserfs_i = REISERFS_I(inode);
-       struct posix_acl **acl = &reiserfs_i->i_acl_access;
        if (strlen(name) != sizeof(POSIX_ACL_XATTR_ACCESS) - 1)
                return -EINVAL;
-       if (!IS_ERR(*acl) && *acl) {
-               posix_acl_release(*acl);
-               *acl = ERR_PTR(-ENODATA);
-       }
-
+       iset_acl(inode, &reiserfs_i->i_acl_access, ERR_PTR(-ENODATA));
        return 0;
 }
 
@@ -533,14 +530,9 @@ posix_acl_default_set(struct inode *inode, const char *name,
 static int posix_acl_default_del(struct inode *inode, const char *name)
 {
        struct reiserfs_inode_info *reiserfs_i = REISERFS_I(inode);
-       struct posix_acl **acl = &reiserfs_i->i_acl_default;
        if (strlen(name) != sizeof(POSIX_ACL_XATTR_DEFAULT) - 1)
                return -EINVAL;
-       if (!IS_ERR(*acl) && *acl) {
-               posix_acl_release(*acl);
-               *acl = ERR_PTR(-ENODATA);
-       }
-
+       iset_acl(inode, &reiserfs_i->i_acl_default, ERR_PTR(-ENODATA));
        return 0;
 }
 
index 6c4af98..e00d240 100644 (file)
@@ -2224,7 +2224,4 @@ int reiserfs_unpack(struct inode *inode, struct file *filp);
 #define reiserfs_write_lock( sb ) lock_kernel()
 #define reiserfs_write_unlock( sb ) unlock_kernel()
 
-/* xattr stuff */
-#define REISERFS_XATTR_DIR_SEM(s) (REISERFS_SB(s)->xattr_dir_sem)
-
 #endif                         /* _LINUX_REISER_FS_H */
index cbb8868..c8aee41 100644 (file)
@@ -403,9 +403,6 @@ struct reiserfs_sb_info {
        spinlock_t bitmap_lock; /* this lock on now only used to protect reserved_blocks variable */
        struct dentry *priv_root;       /* root of /.reiserfs_priv */
        struct dentry *xattr_root;      /* root of /.reiserfs_priv/.xa */
-#ifdef CONFIG_REISERFS_FS_XATTR
-       struct rw_semaphore xattr_dir_sem;
-#endif
        int j_errno;
 #ifdef CONFIG_QUOTA
        char *s_qf_names[MAXQUOTAS];
index 13cdd5e..65c16fa 100644 (file)
@@ -67,45 +67,27 @@ extern struct reiserfs_xattr_handler user_handler;
 extern struct reiserfs_xattr_handler trusted_handler;
 extern struct reiserfs_xattr_handler security_handler;
 
-static inline void reiserfs_write_lock_xattrs(struct super_block *sb)
-{
-       down_write(&REISERFS_XATTR_DIR_SEM(sb));
-}
-static inline void reiserfs_write_unlock_xattrs(struct super_block *sb)
-{
-       up_write(&REISERFS_XATTR_DIR_SEM(sb));
-}
-static inline void reiserfs_read_lock_xattrs(struct super_block *sb)
-{
-       down_read(&REISERFS_XATTR_DIR_SEM(sb));
-}
-
-static inline void reiserfs_read_unlock_xattrs(struct super_block *sb)
-{
-       up_read(&REISERFS_XATTR_DIR_SEM(sb));
-}
-
 static inline void reiserfs_write_lock_xattr_i(struct inode *inode)
 {
-       down_write(&REISERFS_I(inode)->xattr_sem);
+       down_write(&REISERFS_I(inode)->i_xattr_sem);
 }
 static inline void reiserfs_write_unlock_xattr_i(struct inode *inode)
 {
-       up_write(&REISERFS_I(inode)->xattr_sem);
+       up_write(&REISERFS_I(inode)->i_xattr_sem);
 }
 static inline void reiserfs_read_lock_xattr_i(struct inode *inode)
 {
-       down_read(&REISERFS_I(inode)->xattr_sem);
+       down_read(&REISERFS_I(inode)->i_xattr_sem);
 }
 
 static inline void reiserfs_read_unlock_xattr_i(struct inode *inode)
 {
-       up_read(&REISERFS_I(inode)->xattr_sem);
+       up_read(&REISERFS_I(inode)->i_xattr_sem);
 }
 
 static inline void reiserfs_init_xattr_rwsem(struct inode *inode)
 {
-       init_rwsem(&REISERFS_I(inode)->xattr_sem);
+       init_rwsem(&REISERFS_I(inode)->i_xattr_sem);
 }
 
 #else